MINOR: Cleanup 'share' from group.coordinator.rebalance.protocols #20466
Conversation
… value. Share Groups should instead be enabled through the share.version
github-actions
bot
added
triage
.gitignore
Outdated
| @@ -31,6 +31,8 @@ TAGS | |||
| Vagrantfile.local | |||
| /logs | |||
| .DS_Store | |||
| .bloop/ | |||
There was a problem hiding this comment.
Please revert this change.
There was a problem hiding this comment.
Thanks for the review, @AndrewJSchofield. I’ve reverted the .gitignore change. Please let me know if there’s anything else I should adjust.
AndrewJSchofield
added
KIP-932
AndrewJSchofield
changed the title
|
@elmoctarebnou There are some build failures. Please check the logs and resolve. For example: |
…nd test rejecting 'share'
|
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. |
|
Hi @AndrewJSchofield all tests are passing except for the ones already marked as flaky, which seem unrelated to this change. |
| @@ -1784,15 +1784,15 @@ class KafkaConfigTest { | |||
| props.put(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, "consumer") | |||
| assertThrows(classOf[ConfigException], () => KafkaConfig.fromProps(props)) | |||
|
|
|||
|
|
|||
| @@ -69,6 +69,11 @@ public class GroupCoordinatorConfig { | |||
| Group.GroupType.CLASSIC.toString(), | |||
| Group.GroupType.CONSUMER.toString(), | |||
| Group.GroupType.STREAMS.toString()); | |||
| public static final List<String> GROUP_COORDINATOR_REBALANCE_PROTOCOLS_ALLOWED = List.of( | |||
There was a problem hiding this comment.
I think we could reuse the GROUP_COORDINATOR_REBALANCE_PROTOCOLS_DEFAULT constant since they are exactly the same.
There was a problem hiding this comment.
Thanks @JimmyWang6 for the comment. I’ll update so ALLOWED just references DEFAULT.
|
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. |
| assertEquals(Set(GroupType.CLASSIC, GroupType.CONSUMER), config.groupCoordinatorRebalanceProtocols) | ||
|
|
||
| // This is OK. | ||
| // Including "share" should be rejected. | ||
| props.put(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, "classic,consumer,share") |
There was a problem hiding this comment.
Should we delete share here?
There was a problem hiding this comment.
Oh, I miss something, please ignore this comment.
Cleanup 'share' from group.coordinator.rebalance.protocols as a valid
value. Share Groups should instead be enabled through the share.version
Reviewers: Christo
Lolov[email protected]