Skip to content

KAFKA-8219: add doc changes for static membership release#6790

Merged
guozhangwang merged 4 commits into
apache:trunkfrom
abbccdda:web_doc
May 27, 2019
Merged

KAFKA-8219: add doc changes for static membership release#6790
guozhangwang merged 4 commits into
apache:trunkfrom
abbccdda:web_doc

Conversation

@abbccdda

Copy link
Copy Markdown

This is a doc-change only PR to add upgrade notes for static membership in 2.3

@abbccdda

abbccdda commented May 22, 2019

Copy link
Copy Markdown
Author

@guozhangwang @hachikuji @mjsax Let me know if there is anything missing. Thank you!

@abbccdda

Copy link
Copy Markdown
Author

Retest this please

1 similar comment
@abbccdda

Copy link
Copy Markdown
Author

Retest this please

@abbccdda

Copy link
Copy Markdown
Author

work?

Comment thread docs/streams/upgrade-guide.html Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

KStream is not the standard term we recommend to call here. We should just use Kafka Streams or Streams API.

Comment thread docs/streams/upgrade-guide.html Outdated
Comment thread docs/streams/upgrade-guide.html Outdated
Comment thread docs/upgrade.html Outdated

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My comments on Streams upgrade docs also apply to the main docs -- just did not add them twice.

For the things, that I marked as "do we need this", it might be better to add a proper section about static group membership and explain how it works. And we could link to this section. In future versions, people won't read older release notes and thus this information get buried -- having a proper section about static group membership avoid this issue.

Comment thread docs/streams/upgrade-guide.html Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should use KStream only to refer to a stream (ie, KStream vs KTable) and use Kafka Streams otherwise to avoid confusion.

Comment thread docs/streams/upgrade-guide.html Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

membership, [it is required] to upgrade your Kafka cluster (i.e., brokers) and your Kafka Streams application to 2.3.0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to include This involves a necessity to change <code>inter.broker.protocol.version</code> to 2.3 as well. -- Maybe just link to the main upgrade section instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I guess declaring the requirement here is important to make sure user won't choose to only upgrade one broker for coordinator as a shortcut.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but this information is in the broker upgrade section already -- it seems better to not scatter information, but link to the corresponding upgrade sections? Or maybe I am missing your point?

Comment thread docs/streams/upgrade-guide.html Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then you need to set the config -> You also need to set the config

Comment thread docs/streams/upgrade-guide.html Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need it for user to trouble shooting when they set group.instance.id on client where broker doesn't support.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but as mentioned below, I would prefer to have this information is a proper "static group membership" sections. Having it in the upgrade guide does not seem to be the best place.

Comment thread docs/streams/upgrade-guide.html Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We need to document the expected mechanism for dealing with duplicate instance id case.

@abbccdda

Copy link
Copy Markdown
Author

Working on a central page for static membership.

@abbccdda
abbccdda force-pushed the web_doc branch 3 times, most recently from 4099ce1 to 049312b Compare May 24, 2019 05:23

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update! I like the new section a lot!

Comment thread docs/design.html Outdated
Comment thread docs/design.html Outdated
Comment thread docs/design.html Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add one bullet point for Streams:
<li>For Kafka Streams applications, it is sufficient to set a unique <code>ConsumerConfig#GROUP_INSTANCE_ID_CONFIG</code> per KafkaStreams instance, independent of the number of used threads for an instance.</li>

Side question: what happens if one set's the config for a consumer with no group.id -- this case could easily happen for Kafka Streams restore and global consumer?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@mjsax Static membership only affects group based consumer app. Standalone consumers are not affected (I.E. we won't assign them group.instance.id

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let me rephrase: What happens if a user configures no group.id but does configure group.instance.id. Do the consumer crash? Or is the group.instance.id config just ignored?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My understanding is that these two configs should not affect each other. But will try to add a unit test to check.

Comment thread docs/design.html Outdated
Comment thread docs/design.html Outdated
@abbccdda

Copy link
Copy Markdown
Author

@guozhangwang Could you take another look?

@guozhangwang
guozhangwang merged commit 27943e3 into apache:trunk May 27, 2019
guozhangwang pushed a commit that referenced this pull request May 27, 2019
* add doc changes for static membership release

* address comments

* address comments for a separate page

* address Matthias' comment
@guozhangwang

Copy link
Copy Markdown
Contributor

Cherry-picked to 2.3 as well.

haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
* add doc changes for static membership release

* address comments

* address comments for a separate page

* address Matthias' comment
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
* add doc changes for static membership release

* address comments

* address comments for a separate page

* address Matthias' comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants