Skip to content

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

Merged
merged 8 commits into from
Apr 30, 2025

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented Apr 18, 2025

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

@mjsax mjsax added streams core Kafka Broker KIP-1071 PRs related to KIP-1071 labels Apr 18, 2025
Copy link
Member

@chia7712 chia7712 left a 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!

@mjsax
Copy link
Member Author

mjsax commented Apr 24, 2025

Updated this PR.

@mjsax mjsax force-pushed the kafka-19173-kip-1071-feature-flag branch from dc5bc03 to 43e212a Compare April 25, 2025 04:01
@@ -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`+
Copy link
Member Author

Choose a reason for hiding this comment

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

Side question...

Copy link
Member

Choose a reason for hiding this comment

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

See my comment above.

Copy link
Member Author

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 ?

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Member

@jolshan jolshan Apr 29, 2025

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. :)

Copy link
Member

@lucasbru lucasbru left a 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).
Copy link
Member

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

Copy link
Member

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.)

Copy link
Member

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.

Copy link
Member

@jolshan jolshan Apr 28, 2025

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)

Copy link
Contributor

@junrao junrao Apr 28, 2025

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.
Copy link
Member

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.

Copy link
Member

@jolshan jolshan left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@junrao junrao left a 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);
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

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`+
Copy link
Contributor

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).
Copy link
Contributor

@junrao junrao Apr 28, 2025

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.

mjsax added 7 commits April 28, 2025 16:30
Add new StreamsGroupFeature, disabled by default,
and add "streams" as default value to `group.coordinator.rebalance.protocols`.
@mjsax mjsax force-pushed the kafka-19173-kip-1071-feature-flag branch from ac715e8 to 9a14f8b Compare April 29, 2025 01:14
@mjsax mjsax merged commit b0a26bc into apache:trunk Apr 30, 2025
21 checks passed
@mjsax mjsax deleted the kafka-19173-kip-1071-feature-flag branch April 30, 2025 05:51
shmily7829 pushed a commit to shmily7829/kafka that referenced this pull request May 7, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Kafka Broker KIP-1071 PRs related to KIP-1071 kraft streams tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants