-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
KAFKA-17532: Moved ShareGroupConfig to share module #17176
base: trunk
Are you sure you want to change the base?
Conversation
@@ -954,6 +956,8 @@ project(':share') { | |||
|
|||
dependencies { | |||
implementation project(':server-common') | |||
implementation project(':clients') | |||
implementation project(':coordinator-common') |
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.
Isn't there is a cycle i.e. coordinator-common
requires server
, server
requires share
and share
requires coordinator-common
?
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.
Looks mosty fine. Some comments to address.
@@ -40,6 +40,9 @@ | |||
|
|||
<subpackage name="coordinator"> | |||
<subpackage name="common"> | |||
|
|||
<allow pkg="org.apache.kafka.common" /> |
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.
Line number 34 - 39 above defines common classes. Which common package do we need explitily, can we please include that above.
@@ -33,6 +33,9 @@ | |||
<!-- anyone can use public classes --> | |||
<allow pkg="org.apache.kafka.common" exact-match="true" /> | |||
<allow pkg="org.apache.kafka.common.utils" /> | |||
<allow pkg="org.apache.kafka.common.config" /> | |||
<allow pkg="org.apache.kafka.common.errors" /> | |||
<allow pkg="org.apache.kafka.coordinator.common" /> |
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 not the right section to include coordinator
.
@@ -202,7 +202,7 @@ public void testSupportedNewGroupProtocols(ClusterInstance clusterInstance) { | |||
|
|||
@ClusterTests({ | |||
@ClusterTest(types = {Type.ZK, Type.KRAFT, Type.CO_KRAFT}, serverProperties = { | |||
@ClusterConfigProperty(key = GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, value = "classic"), | |||
@ClusterConfigProperty(key = GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, value = "Classic"), |
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.
Why this change is required?
@@ -72,7 +72,7 @@ abstract class IntegrationTestHarness extends KafkaServerTestHarness { | |||
cfgs.foreach(_.setProperty(KRaftConfigs.MIGRATION_ENABLED_CONFIG, "true")) | |||
} | |||
if (isShareGroupTest()) { | |||
cfgs.foreach(_.setProperty(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, "classic,consumer,share")) | |||
cfgs.foreach(_.setProperty(CoordinatorCommonConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, "Classic,Consumer,Share")) |
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.
Why do we need to change "Classic,Consumer,Share"
?
@@ -43,7 +44,7 @@ class ConsumerGroupHeartbeatRequestTest(cluster: ClusterInstance) { | |||
@ClusterTest( | |||
types = Array(Type.KRAFT), | |||
serverProperties = Array( | |||
new ClusterConfigProperty(key = GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, value = "classic"), | |||
new ClusterConfigProperty(key = CoordinatorCommonConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG, value = "Classic"), |
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.
Agains same is Classic
change is required?
In this PR, the ShareGroupConfig has been moved to the share package. Since the config
GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
fromGroupCoordinatorConfig
is required inShareGroupConfig
, andshare
cannot depend ongroup-coordinator
,GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
has been moved to a separate config file calledCoordinatorCommonConfig
incoordinator-common
Reference - KAFKA-17532