-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
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
@@ -147,7 +158,7 @@ public enum MetadataVersion { | |||
* <strong>Think carefully before you update this value. ONCE A METADATA VERSION IS PRODUCTION, | |||
* 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. :)
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.
@@ -117,6 +117,9 @@ public enum MetadataVersion { | |||
// 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.
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!
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.
@@ -147,7 +158,7 @@ public enum MetadataVersion { | |||
* <strong>Think carefully before you update this value. ONCE A METADATA VERSION IS PRODUCTION, | |||
* 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?
@@ -117,6 +117,9 @@ public enum MetadataVersion { | |||
// 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