KAFKA-16894: Exploit share feature [3/N] #19542
Conversation
| @ClusterConfigProperty(key = "group.coordinator.rebalance.protocols", value = "classic,consumer,share"), | ||
| @ClusterConfigProperty(key = "group.share.enable", value = "true"), |
There was a problem hiding this comment.
testComplexShareConsumer in this file also has both properties specifed, shall we remove them as well?
There was a problem hiding this comment.
Good catch. Fixed.
| if (config.shareGroupConfig.isShareGroupEnabled && | ||
| config.shareGroupConfig.shareGroupPersisterClassName().nonEmpty) { |
There was a problem hiding this comment.
So irrespective of feature flag or config the share-coordinator thread will run now. I think this is what you mentioned to be fixed in further PRs to start using feature listeners, correct?
| private def isShareGroupProtocolEnabled: Boolean = { | ||
| config.shareGroupConfig.isShareGroupEnabled | ||
| config.shareGroupConfig.isShareGroupEnabled || shareVersion().supportsShareGroups |
There was a problem hiding this comment.
Query: So we have removed the config isShareGroupEnabled usage from BokerServer and tests but still uses in KafkaApis, why?
There was a problem hiding this comment.
In the tests, the non-production features are automatically enabled. As a result, tests generally do not need to enable the feature. However, if you make a broker outside the test infrastructure, it will not have share groups enabled.
In the period of time that the feature is not enabled by default, having an internal config (group.share.enable) as a simple way to turn on share groups without using the feature is helpful. For situations in which creating a broker using config is automated or scripted, just adding group.share.enable=true to the config is very simple. The alternative using a feature is a bit more fiddly (either enable the feature when formatting the broker storage, or start the broker and then enable the feature with a second command)
There was a problem hiding this comment.
The other part of this of course is that BrokerServer unconditionally initialises the SharePartitionManager and share coordinator.
There was a problem hiding this comment.
Thanks for explaining.
| val delta = new MetadataDelta(MetadataImage.EMPTY); | ||
| delta.replay(new FeatureLevelRecord() | ||
| .setName(MetadataVersion.FEATURE_NAME) | ||
| .setFeatureLevel(MetadataVersion.MINIMUM_VERSION.featureLevel()) | ||
| ) | ||
| delta.replay(new FeatureLevelRecord() | ||
| .setName(ShareVersion.FEATURE_NAME) | ||
| .setFeatureLevel(ShareVersion.SV_1.featureLevel()) | ||
| ) | ||
| cache.setImage(delta.apply(MetadataProvenance.EMPTY)) |
There was a problem hiding this comment.
nit: Seems repeating at a lot of instance, should we have a method to enable share groups which can be called in all the methods?
There was a problem hiding this comment.
Looks good, are we fixing this comment as well: #19542 (comment)?
Thinking about it. Might make it a follow-on. That's a very large file and there's more tidying up that could usefully be done I think. |
This PR uses the v1 of the ShareVersion feature to enable share groups for KIP-932. Previously, there were two potential configs which could be used - `group.share.enable=true` and including "share" in `group.coordinator.rebalance.protocols`. After this PR, the first of these is retained, but the second is not. Instead, the preferred switch is the ShareVersion feature. The `group.share.enable` config is temporarily retained for testing and situations in which it is inconvenient to set the feature, but it should really not be necessary, especially when we get to AK 4.2. The aim is to remove this internal config at that point. No tests should be setting `group.share.enable` any more, because they can use the feature (which is enabled in test environments by default because that's how features work). For tests which need to disable share groups, they now set the share feature to v0. The majority of the code changes were related to correct initialisation of the metadata cache in tests now that a feature is used. Reviewers: Apoorv Mittal <[email protected]>
…#19633) Currently, the quorum uses kraft by default, so there's no need to specify it explicitly. For kip932 and isShareGroupTest, they are no longer used after #19542 . Reviewers: PoAn Yang <[email protected]>, Ken Huang <[email protected]>, Chia-Ping Tsai <[email protected]>
This PR uses the v1 of the ShareVersion feature to enable share groups
for KIP-932.
Previously, there were two potential configs which could be used -
group.share.enable=trueand including "share" ingroup.coordinator.rebalance.protocols. After this PR, the first ofthese is retained, but the second is not. Instead, the preferred switch
is the ShareVersion feature.
The
group.share.enableconfig is temporarily retained for testing andsituations in which it is inconvenient to set the feature, but it should
really not be necessary, especially when we get to AK 4.2. The aim is to
remove this internal config at that point.
No tests should be setting
group.share.enableany more, because theycan use the feature (which is enabled in test environments by default
because that's how features work). For tests which need to disable share
groups, they now set the share feature to v0. The majority of the code
changes were related to correct initialisation of the metadata cache in
tests now that a feature is used.