Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR:Upgrade guide updates for KIP-479 #7550

Merged
merged 6 commits into from Oct 22, 2019
Merged

Conversation

bbejeck
Copy link
Contributor

@bbejeck bbejeck commented Oct 17, 2019

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 17, 2019

Copy link
Contributor

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a few suggestions but feel free to take or leave any/all of them

@@ -76,8 +76,13 @@ <h3><a id="streams_api_changes_240" href="#streams_api_changes_240">Streams API

<!-- Placeholder KIP-213 -->
<!-- Placeholder KIP-307 -->
<!-- Placeholder KIP-479 -->
<p>
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a Stream-Stream join. If users added DSL operations upstream of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: stream-stream join (or else KStream-KStream) here and below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack


Another feature delivered by <code>StreamJoined</code> that will be of interest to users is that you can now configure the type of state store used in the join. Users can now elect to use in-memory stores or custom state stores for a Stream-Stream join. Users should note that the provided stores will not be available for searching via Interactive Queries. With the addition of <code>StreamJoined</code> Stream-Stream join operations using <code>Joined</code> have been deprecated, and users are encouraged to switch over to Stream-Stream join methods using the new overloaded methods. You can get more details from <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join">KIP-479</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: (single line so can't refer to specific location)

  1. "searching via IQ" -> "querying via IQ"?
  2. The sentence beginning "With the addition..." reads a bit awkwardly to me. Maybe something like "stream-stream joins accepting a Joined parameter have been deprecated; users are encouraged to migrate to the new StreamJoined parameter (is that an accurate description?)
  3. I would also maybe refactor the first sentence a bit, something like "With the addition of the StreamJoined object, you can now configure the type of state store..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

Copy link
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @bbejeck.

As always, some wording nits are impossible to avoid, but it looks good to me overall!

@@ -76,8 +76,13 @@ <h3><a id="streams_api_changes_240" href="#streams_api_changes_240">Streams API

<!-- Placeholder KIP-213 -->
<!-- Placeholder KIP-307 -->
<!-- Placeholder KIP-479 -->
<p>
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a Stream-Stream join. If users added DSL operations upstream of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a Stream-Stream join. If users added DSL operations upstream of the
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a Stream-Stream join. If users added DSL operations before the

Super subtle difference, but the only thing that affects the names is the order in which you call the DSL operators, so you can actually add upstream operations, as long as you add the new DSL calls after the original ones. I wouldn't get into it in the docs, but it's easy to swap "before" for "upstream" and be technically correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

<!-- Placeholder KIP-479 -->
<p>
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a Stream-Stream join. If users added DSL operations upstream of the
join, the internal names of the state stores would shift, requiring an application reset when redeploying. Now in the 2.4.0 release Kafka Streams adds the <code>StreamJoined</code>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
join, the internal names of the state stores would shift, requiring an application reset when redeploying. Now in the 2.4.0 release Kafka Streams adds the <code>StreamJoined</code>
join, the internal names of the state stores would shift, requiring an application reset when redeploying. In the 2.4.0 release, Kafka Streams adds the <code>StreamJoined</code>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack


Another feature delivered by <code>StreamJoined</code> that will be of interest to users is that you can now configure the type of state store used in the join. Users can now elect to use in-memory stores or custom state stores for a Stream-Stream join. Users should note that the provided stores will not be available for searching via Interactive Queries. With the addition of <code>StreamJoined</code> Stream-Stream join operations using <code>Joined</code> have been deprecated, and users are encouraged to switch over to Stream-Stream join methods using the new overloaded methods. You can get more details from <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join">KIP-479</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Another feature delivered by <code>StreamJoined</code> that will be of interest to users is that you can now configure the type of state store used in the join. Users can now elect to use in-memory stores or custom state stores for a Stream-Stream join. Users should note that the provided stores will not be available for searching via Interactive Queries. With the addition of <code>StreamJoined</code> Stream-Stream join operations using <code>Joined</code> have been deprecated, and users are encouraged to switch over to Stream-Stream join methods using the new overloaded methods. You can get more details from <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join">KIP-479</a>
Another feature delivered by <code>StreamJoined</code> is that you can now configure the type of state store used in the join. You can now elect to use in-memory stores or custom state stores for a Stream-Stream join. Note that the provided stores will not be available for searching via Interactive Queries. With the addition of <code>StreamJoined</code>, Stream-Stream join operations using <code>Joined</code> have been deprecated. Please switch over to Stream-Stream join methods using the new overloaded methods. You can get more details from <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join">KIP-479</a>

Several suggestions for this line:

  • The audience of this documentation is the user, so it makes more sense to say "you" than "the user". If you look back over it, you'll see we mix both forms of address here and elsewhere. We should stick to "you", it's easier and more natural to read that way.
  • I also shortened up some phrases I felt were overly qualified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 18, 2019

\cc @mjsax

@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 18, 2019

Thanks, @ableegoldman and @vvcephei for the review! I've updated per comments.


Another feature delivered by <code>StreamJoined</code> is that you can now configure the type of state store used in the join. You can now elect to use in-memory stores or custom state stores for a stream-stream join. Note that the provided stores will not be available for querying via Interactive Queries. With the addition of <code>StreamJoined</code>, stream-stream join operations using <code>Joined</code> have been deprecated. Please switch over to stream-stream join methods using the new overloaded methods. You can get more details from <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join">KIP-479</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long...

<p>
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a stream-stream join. If users added DSL operations before of the
join, the internal names of the state stores would shift, requiring an application reset when redeploying. In the 2.4.0 release, Kafka Streams adds the <code>StreamJoined</code>
object, which gives users the ability to name the join processor, repartition topic(s) (if a repartition is required), and the state stores involved in the join. Also, by naming the state stores, the changelog topics backing the state stores are named as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing <br /> markup ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it <br> or <br />? I've seen both in the docs and both seem to work, but what's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

<!-- Placeholder KIP-479 -->
<p>
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a stream-stream join. If users added DSL operations before of the
join, the internal names of the state stores would shift, requiring an application reset when redeploying. In the 2.4.0 release, Kafka Streams adds the <code>StreamJoined</code>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added DSL operations before of the join -- sound confusing.

If users changed their topology and added a operator before a join,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

<p>
Before the 2.4.0 version of Kafa Streams, users of the DSL could not name the state stores involved in a stream-stream join. If users added DSL operations before of the
join, the internal names of the state stores would shift, requiring an application reset when redeploying. In the 2.4.0 release, Kafka Streams adds the <code>StreamJoined</code>
object, which gives users the ability to name the join processor, repartition topic(s) (if a repartition is required), and the state stores involved in the join. Also, by naming the state stores, the changelog topics backing the state stores are named as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit object -> class

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line is still quite long.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

join, the internal names of the state stores would shift, requiring an application reset when redeploying. In the 2.4.0 release, Kafka Streams adds the <code>StreamJoined</code>
object, which gives users the ability to name the join processor, repartition topic(s) (if a repartition is required), and the state stores involved in the join. Also, by naming the state stores, the changelog topics backing the state stores are named as well.
<br/>
Another feature delivered by <code>StreamJoined</code> is that you can now configure the type of state store used in the join. You can now elect to use in-memory stores or custom state stores for a stream-stream join. Note that the provided stores will not be available for querying via Interactive Queries. With the addition of <code>StreamJoined</code>, stream-stream join operations using <code>Joined</code> have been deprecated. Please switch over to stream-stream join methods using the new overloaded methods. You can get more details from
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can now elect -> You can elect (the now is repetitive as it's used in the sentence before already)

Also, this line is too long

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the provided stores will not be available for querying via Interactive Queries -> this is also true if the store in only named (might be worth to mention in the paragraph above already)?

object, which gives users the ability to name the join processor, repartition topic(s) (if a repartition is required), and the state stores involved in the join. Also, by naming the state stores, the changelog topics backing the state stores are named as well.
<br/>
Another feature delivered by <code>StreamJoined</code> is that you can now configure the type of state store used in the join. You can now elect to use in-memory stores or custom state stores for a stream-stream join. Note that the provided stores will not be available for querying via Interactive Queries. With the addition of <code>StreamJoined</code>, stream-stream join operations using <code>Joined</code> have been deprecated. Please switch over to stream-stream join methods using the new overloaded methods. You can get more details from
<a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-479%3A+Add+StreamJoined+config+object+to+Join">KIP-479</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. Missing . at the end of the sentence

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@mjsax
Copy link
Member

mjsax commented Oct 21, 2019

@bbejeck This needs to be rebased to resolve merged conflicts, too.

@bbejeck
Copy link
Contributor Author

bbejeck commented Oct 21, 2019

@mjsax updated and rebased this

@mjsax mjsax merged commit c7a3d1b into apache:trunk Oct 22, 2019
mjsax pushed a commit that referenced this pull request Oct 22, 2019
Reviewers: A. Sophie Blee-Goldmann <sophie@confluent.io>, John Roesler <john@confluent.io>, Matthias J. Sax <matthias@confluent.io>
@mjsax
Copy link
Member

mjsax commented Oct 22, 2019

Merged to trunk and cherry-picked to 2.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants