MINOR: Cleanup 'share' from group.coordinator.rebalance.protocols #20466

elmoctarebnou · 2025-09-03T07:05:28Z

Cleanup 'share' from group.coordinator.rebalance.protocols as a valid
value. Share Groups should instead be enabled through the share.version

Reviewers: Andrew Schofield [email protected], jimmy
[email protected], TaiJuWu [email protected], Chia-Ping
Tsai [email protected]

… value. Share Groups should instead be enabled through the share.version

AndrewJSchofield

Thanks for the PR.

.gitignore

AndrewJSchofield · 2025-09-03T14:43:26Z

@elmoctarebnou There are some build failures. Please check the logs and resolve. For example:

Error:  /home/runner/work/kafka/kafka/core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala:1789:9: local var config in method testGroupCoordinatorRebalanceProtocols is never updated: consider using immutable val

…nd test rejecting 'share'

elmoctarebnou · 2025-09-04T13:50:06Z

Thanks, @AndrewJSchofield — I fixed the error you pointed out. I also chatted with @clolov , and he suggested adding a small test to validate that "share" is not included in group.coordinator.rebalance.protocols, which I’ve now added.

elmoctarebnou · 2025-09-05T03:31:21Z

Hi @AndrewJSchofield all tests are passing except for the ones already marked as flaky, which seem unrelated to this change.

JimmyWang6

Thanks for the PR.

JimmyWang6 · 2025-09-05T07:02:57Z

core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala

    props.put(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, "consumer")
    assertThrows(classOf[ConfigException], () => KafkaConfig.fromProps(props))

+


nit: extra blank.

JimmyWang6 · 2025-09-05T07:04:36Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java

        Group.GroupType.CLASSIC.toString(),
        Group.GroupType.CONSUMER.toString(),
        Group.GroupType.STREAMS.toString());
+    public static final List<String> GROUP_COORDINATOR_REBALANCE_PROTOCOLS_ALLOWED = List.of(


I think we could reuse the GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DEFAULT constant since they are exactly the same.

Thanks @JimmyWang6 for the comment. I’ll update so ALLOWED just references DEFAULT.

AndrewJSchofield · 2025-09-05T07:55:37Z

Requesting review from @dajac since he's most knowledgeable about this config. I'm not certain the change is necessary or worthwhile given that this config is possibly being deprecated soon.

TaiJuWu · 2025-09-08T17:19:39Z

core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala


-    // This is OK.
+    // Including "share" should be rejected.
    props.put(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, "classic,consumer,share")


Should we delete share here?

Oh, I miss something, please ignore this comment.

chia7712 · 2025-09-24T16:56:40Z

@elmoctarebnou could you please fix the conflicts?

elmoctarebnou · 2025-09-25T03:42:37Z

Thank you @chia7712 for pointing out the conflict, somehow I missed this.

chia7712 · 2025-09-25T16:18:28Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java

        Group.GroupType.CLASSIC.toString(),
        Group.GroupType.CONSUMER.toString(),
        Group.GroupType.STREAMS.toString());
+    public static final List<String> GROUP_COORDINATOR_REBALANCE_PROTOCOLS_ALLOWED = GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DEFAULT;


why not using GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DEFAULT directly?

@chia7712 thank you for the comment! The GROUP_COORDINATOR_REBALANCE_PROTOCOLS_ALLOWED is just for readability.

chia7712 · 2025-09-25T16:20:23Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java

        // Group coordinator configs
        .define(GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, LIST, GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DEFAULT,
-                ConfigDef.ValidList.in(false, Group.GroupType.documentValidValues()), MEDIUM, GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DOC)
+                ConfigDef.ValidList.in(GROUP_COORDINATOR_REBALANCE_PROTOCOLS_ALLOWED.toArray(new String[0])), 


not sure why share is not controlled by group.coordinator.rebalance.protocols? By contrast, stream and consumer could be disabled by group.coordinator.rebalance.protocols

That's because share groups are controlled by a feature. This config was created before the features were implemented to turn on KIP-848 and KIP-932. I understand the plan is to deprecate and remove it entirely. It's a really bad idea having a broker-level config that controls this because individual brokers might be misconfigured and a cluster is in a mixed state. That's why a feature is so much better.

@AndrewJSchofield Thanks for the response. Do we have a KIP that covers deprecating this config? Since each protocol already maintains its own feature version, it feels like the right time to deprecate it. Otherwise, exposing an incomplete public config seems a bit odd to me

No KIP yet. But then KIP-932 says that share groups are enabled by a feature, and not group.coordinator.rebalance.protocols.

@chia7712 I do understand your point but I was not expecting other group types to use this config after KIP-848. It would be acceptable to me for share groups to honour this config in trunk and AK 4.2. Let me know what you think.

I do understand your point but I was not expecting other group types to use this config after KIP-848. It would be acceptable to me for share groups to honour this config in trunk and AK 4.2. Let me know what you think.

In fact, I’d prefer to raise a KIP to deprecate group.coordinator.rebalance.protocols in trunk and AK 4.2. Let’s leave it as is, since extending a deprecated config would be a bit odd. I’ve opened KAFKA-19740 for this.

@dajac I think you're planning a KIP to complete the new consumer group protocol migration, so I expect this is on your radar.

elmoctarebnou · 2025-09-26T16:28:37Z

Related PR: apache/kafka-site#717

chia7712 · 2025-09-29T15:35:16Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java

    public static final ConfigDef CONFIG_DEF = new ConfigDef()
        // Group coordinator configs
        .define(GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, LIST, GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DEFAULT,
-                ConfigDef.ValidList.in(false, Group.GroupType.documentValidValues()), MEDIUM, GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DOC)


It should be ConfigDef.ValidList.in(false, GROUP_COORDINATOR_REBALANCE_PROTOCOLS_ALLOWED.toArray(new String[0])), right? The empty GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG is illegal

@chia7712 You're right — thanks for catching that! I’ve updated the validation to disallow empty lists

chia7712

LGTM

chia7712 · 2025-10-06T13:05:32Z

@AndrewJSchofield What do you think about this PR?

AndrewJSchofield · 2025-10-07T13:11:37Z

@AndrewJSchofield What do you think about this PR?

It's fine. I slightly prefer to permit and ignore "share" in the config, but I know it's just a matter of taste.

chia7712 · 2025-10-07T14:29:11Z

I slightly prefer to permit and ignore "share" in the config, but I know it's just a matter of taste.

This seems to be related to a compatibility issue. The config group.coordinator.rebalance.protocols=classic,share should not throw an exception, even though share is a no-op

@elmoctarebnou what's your take on this?

elmoctarebnou · 2025-10-07T15:18:50Z

Thanks @chia7712 and @AndrewJSchofield for the feedback — I agree that maintaining compatibility makes sense here. I’ll update the PR so that "share" is accepted in group.coordinator.rebalance.protocols but treated as a no-op (ignored internally with a warning). Let me know if you’re good with that approach, or if you’d prefer a different handling.

chia7712 · 2025-10-07T15:27:09Z

I’ll update the PR so that "share" is accepted in group.coordinator.rebalance.protocols but treated as a no-op (ignored internally with a warning)

sounds good to me 😃

AndrewJSchofield · 2025-10-10T19:15:23Z

I’ll update the PR so that "share" is accepted in group.coordinator.rebalance.protocols but treated as a no-op (ignored internally with a warning). Let me know if you’re good with that approach, or if you’d prefer a different handling.

Works for me too. Thanks.

…g log

chia7712 · 2025-10-13T14:18:50Z

@elmoctarebnou Could you please merge trunk to bring in the recent CI updates?

…rebalance-protocols

AndrewJSchofield

Looks good to me.

Cleanup 'share' from group.coordinator.rebalance.protocols as a valid…

88834ca

… value. Share Groups should instead be enabled through the share.version

github-actions bot added triage PRs from the community core Kafka Broker clients small Small PRs labels Sep 3, 2025

elmoctarebnou mentioned this pull request Sep 3, 2025

Updated the documentation to align with broker changes. apache/kafka-site#717

Open

AndrewJSchofield requested changes Sep 3, 2025

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

AndrewJSchofield added KIP-932 Queues for Kafka ci-approved and removed triage PRs from the community labels Sep 3, 2025

AndrewJSchofield changed the title ~~Cleanup 'share' from group.coordinator.rebalance.protocols~~ MINOR: Cleanup 'share' from group.coordinator.rebalance.protocols Sep 3, 2025

Revert .gitignore change per review feedback

49da1b1

github-actions bot added the tests Test fixes (including flaky tests) label Sep 3, 2025

Add explicit allowed list for group.coordinator.rebalance.protocols a…

bac2627

…nd test rejecting 'share'

github-actions bot added the group-coordinator label Sep 4, 2025

JimmyWang6 reviewed Sep 5, 2025

View reviewed changes

AndrewJSchofield requested a review from dajac September 5, 2025 07:53

Reuse GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DEFAULT for ALLOWED

0072a82

TaiJuWu reviewed Sep 8, 2025

View reviewed changes

Merge branch 'trunk' into remove-share-from-rebalance-protocols

098fa13

chia7712 reviewed Sep 25, 2025

View reviewed changes

chia7712 reviewed Sep 29, 2025

View reviewed changes

Disallow empty group.coordinator.rebalance.protocols

476bdb1

chia7712 approved these changes Oct 6, 2025

View reviewed changes

Accept 'share' in group.coordinator.rebalance.protocols with a warnin…

a10df13

…g log

Merge remote-tracking branch 'upstream/trunk' into remove-share-from-…

decf3a6

…rebalance-protocols

AndrewJSchofield approved these changes Oct 14, 2025

View reviewed changes

chia7712 merged commit 781bc7a into apache:trunk Oct 16, 2025
22 checks passed

		props.put(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, "consumer")
		assertThrows(classOf[ConfigException], () => KafkaConfig.fromProps(props))

MINOR: Cleanup 'share' from group.coordinator.rebalance.protocols #20466

MINOR: Cleanup 'share' from group.coordinator.rebalance.protocols #20466

Uh oh!

Conversation

elmoctarebnou commented Sep 3, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewJSchofield commented Sep 3, 2025

Uh oh!

elmoctarebnou commented Sep 4, 2025

Uh oh!

elmoctarebnou commented Sep 5, 2025

Uh oh!

JimmyWang6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Sep 24, 2025

Uh oh!

elmoctarebnou commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elmoctarebnou commented Sep 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Oct 6, 2025

Uh oh!

AndrewJSchofield commented Oct 7, 2025

Uh oh!

chia7712 commented Oct 7, 2025

Uh oh!

elmoctarebnou commented Oct 7, 2025

Uh oh!

chia7712 commented Oct 7, 2025

Uh oh!

AndrewJSchofield commented Oct 10, 2025

Uh oh!

chia7712 commented Oct 13, 2025

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

elmoctarebnou commented Sep 3, 2025 •

edited by github-actions bot

Loading

elmoctarebnou commented Sep 25, 2025 •

edited

Loading