-
Notifications
You must be signed in to change notification settings - Fork 14.9k
KAFKA-19173: Add Feature for "streams" group
#19509
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
Conversation
chia7712
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjsax thanks for this patch. Please take a look at my two small questions. thanks!
tools/src/test/java/org/apache/kafka/tools/FeatureCommandTest.java
Outdated
Show resolved
Hide resolved
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java
Show resolved
Hide resolved
server-common/src/main/java/org/apache/kafka/server/common/StreamsGroupVersion.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/org/apache/kafka/server/common/StreamsGroupVersion.java
Outdated
Show resolved
Hide resolved
server-common/src/main/java/org/apache/kafka/server/common/StreamsGroupVersion.java
Outdated
Show resolved
Hide resolved
|
Updated this PR. |
metadata/src/test/java/org/apache/kafka/metadata/storage/FormatterTest.java
Outdated
Show resolved
Hide resolved
...n-tests/src/test/java/org/apache/kafka/streams/integration/InternalTopicIntegrationTest.java
Show resolved
Hide resolved
dc5bc03 to
43e212a
Compare
| * IT CANNOT BE CHANGED.</strong> | ||
| */ | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_4_0_IV3; | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_4_0_IV3; // when do we update this to `IBP_4_1_IV2`+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side question...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is still on 4_0 though -- when should it get bumped to 4_1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets bumped to 4_1 when MV 4_1-IV0 is production ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this comment?
Yes :)
It gets bumped to 4_1 when MV 4_1-IV0 is production ready.
@jolshan -- from our discussion, my understanding is, that we would only add a MV if something is stable (and IBP_4_1_IV1 added for QfK should actually be removed)? Is ELR not production ready yet? And if not, why was a new MV added for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We add the MV version when we create the feature, but don't mark the MV as LATEST_PRODUCTION until the feature should be enabled by default.
I think we could remove 4_1 from QfK.
For ELR, the first MV (4.0) was due to a dependency on a new MV record value. The second one (4.1) will be marked as stable soon I believe. But Calvin would know better than me. :)
lucasbru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but a review from somebody with more background on features would be good.
| // Enables share groups. Note, share groups are for preview only in 4.1. (KIP-932). | ||
| IBP_4_1_IV1(27, "4.1", "IV1", false), | ||
|
|
||
| // Enables "streams" groups. Note, streams groups are for early access only in 4.1. (KIP-1071). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need two MV versions if we aren't adding any new metadata records for this feature. If you don't want to enable it by default, we can remove this one and just keep ibp 4.1-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Seems like the share groups one was also done incorrectly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jolshan Does that mean EligibleLeaderReplicasVersion is also incorrect. I used that as the basis of ShareVersion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one is different because it requires an update to the MV log. (See the "true" for the first MV under didMetadataChange)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndrewJSchofield : Added a clarification question in https://github.com/apache/kafka/pull/19293/files#r2064970001 regarding IBP_4_1_IV1. We could follow up there.
| SV_0(0, MetadataVersion.MINIMUM_VERSION, Map.of()), | ||
|
|
||
| // Version 1 enables "streams" groups (KIP-1071). | ||
| // Using metadata version IBP_4_2_IV1 disables it in AK 4.1 release, and enables it in AK 4.2 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, we don't need a MV to "disable it". We just don't update the LATEST_PRODUCTION in this file while it is still unstable, and we don't mark the MV listed here as stable until we want it to be set by default.
jolshan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
junrao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjsax : Thanks for the PR. Added a few comments.
| // *** STREAMS GROUPS BECOME PRODUCTION-READY IN THE FUTURE. ITS DEFINITION ALLOWS A STREAMS *** | ||
| // *** GROUPS FEATURE TO BE DEFINED IN 4.1 BUT TURNED OFF BY DEFAULT, ABLE TO BE TURNED ON *** | ||
| // *** DYNAMICALLY TO TRY OUT THE EARLY ACCESS CAPABILITY. *** | ||
| IBP_4_2_IV1(29, "4.2", "IV1", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit awkward to add an unstable MV for each unstable feature level. Could we introduce a single unstable MV that's shared between Qfk and streams MV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did discuss this with @jolshan and her recommendation was to use two individual versions. I'll let her comment about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is easier to understand 1 new MV per feature. That way when your feature is ready you can mark it and you don't have to worry about other folks being ready etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, @jolshan. Sound good to me then.
| * IT CANNOT BE CHANGED.</strong> | ||
| */ | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_4_0_IV3; | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_4_0_IV3; // when do we update this to `IBP_4_1_IV2`+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this comment?
| // Enables share groups. Note, share groups are for preview only in 4.1. (KIP-932). | ||
| IBP_4_1_IV1(27, "4.1", "IV1", false), | ||
|
|
||
| // Enables "streams" groups. Note, streams groups are for early access only in 4.1. (KIP-1071). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndrewJSchofield : Added a clarification question in https://github.com/apache/kafka/pull/19293/files#r2064970001 regarding IBP_4_1_IV1. We could follow up there.
Add new StreamsGroupFeature, disabled by default, and add "streams" as default value to `group.coordinator.rebalance.protocols`.
ac715e8 to
9a14f8b
Compare
Add new StreamsGroupFeature, disabled by default, and add "streams" as default value to `group.coordinator.rebalance.protocols`. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, David Jacot <david.jacot@gmail.com>, Lucas Brutschy <lbrutschy@confluent.io>, Justine Olshan <jolshan@confluent.io>, Andrew Schofield <aschofield@confluent.io>, Jun Rao <jun@confluent.io>
Add new StreamsGroupFeature, disabled by default, and add "streams" as
default value to
group.coordinator.rebalance.protocols.Reviewers: Chia-Ping Tsai chia7712@gmail.com, David Jacot
david.jacot@gmail.com, Lucas Brutschy lbrutschy@confluent.io,
Justine Olshan jolshan@confluent.io, Andrew Schofield
aschofield@confluent.io, Jun Rao jun@confluent.io