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: add Kafka Streams upgrade notes for 2.3 release #6758

Merged
merged 4 commits into from May 24, 2019

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented May 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)

@mjsax mjsax added the streams label May 17, 2019
@mjsax
Copy link
Member Author

mjsax commented May 17, 2019

Call for review @guozhangwang @bbejeck @vvcephei @ableegoldman @cadonna @abbccdda

Tried to cover all KIP for this release. Hope I did not forget anything.

@ableegoldman
Copy link
Contributor

Should we mention upgrading Rocks somewhere? Not an API change exactly but it exposes some new configs

Users should implement this method to release any memory used by RocksDB config objects, by closing those objects.
For more details please read <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-453%3A+Add+close%28%29+method+to+RocksDBConfigSetter">KIP-453</a>.
</p>

<h3><a id="streams_api_changes_230" href="#streams_api_changes_230">Streams API changes in 2.3.0</a></h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

This was moved to above and should be deleted

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups. Overlooked this.

</p>

<p>
In previous release, Kafka Streams only shipped with a built-in in-memory key-value store.
Copy link
Contributor

Choose a reason for hiding this comment

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

These are mentioned again below (line 76), you should remove the other comment

@mjsax
Copy link
Member Author

mjsax commented May 19, 2019

Updated this.

Copy link
Contributor

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

LGTM

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.

A couple of small suggestions. Thanks!

<p>
To improve operator semantics, new store types are added that allow storing an additional timestamp per key-value pair or window.
Some DSL operators (for example KTables) are using those new stores.
Hence, you can now receive the last update timestamp via Interactive Queries if you change the <code>QueryableStoreType</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
Hence, you can now receive the last update timestamp via Interactive Queries if you change the <code>QueryableStoreType</code>.
Hence, you can now retrieve the last update timestamp via Interactive Queries if you specify <code>TimestampedWindowStoreType</code> or <code>TimestampedKeyValueStoreType</code>.

To improve operator semantics, new store types are added that allow storing an additional timestamp per key-value pair or window.
Some DSL operators (for example KTables) are using those new stores.
Hence, you can now receive the last update timestamp via Interactive Queries if you change the <code>QueryableStoreType</code>.
While this change is mainly transparent to the user, there are some corner cases that may require code changes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a nit, but I feel it's a bit strange to call someone "the user" when you're talking directly to them.

Suggested change
While this change is mainly transparent to the user, there are some corner cases that may require code changes:
While this change is mainly transparent, there are some corner cases that may require code changes:

Some DSL operators (for example KTables) are using those new stores.
Hence, you can now receive the last update timestamp via Interactive Queries if you change the <code>QueryableStoreType</code>.
While this change is mainly transparent to the user, there are some corner cases that may require code changes:
<strong>Caution: If you receive an untyped store and use a cast, you might need to update your code to cast to the correct type.</strong>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can drop in the actual exception, to improve the time-to-discover the reason, if someone is just searching:
java.lang.ClassCastException: class org.apache.kafka.streams.state.ValueAndTimestamp cannot be cast to class ${actual value type} upon getting a value from the store.

@mjsax
Copy link
Member Author

mjsax commented May 21, 2019

Updated this.

Copy link
Contributor

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Just one very minor nit, otherwise LGTM


<p>
Kafka Streams used to set the configuration parameter <code>max.poll.interval.ms</code> to <code>Interger.MAX_VALUE</code>.
In default value is removed and Kafka Streams uses the consumer default value now.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 'In->Its`

@mjsax mjsax merged commit 7df79e1 into apache:trunk May 24, 2019
@mjsax mjsax deleted the minor-kip-258-upgrade-notes branch May 24, 2019 06:19
@mjsax
Copy link
Member Author

mjsax commented May 24, 2019

Merged to trunk and cherry-picked to 2.3

mjsax added a commit that referenced this pull request May 24, 2019
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
Pengxiaolong pushed a commit to Pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants