You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "chirag-wadhwa5 (via GitHub)" <gi...@apache.org> on 2024/09/12 10:46:55 UTC
[PR] KAFKA-17532: Moved ShareGroupConfig to share module [kafka]
chirag-wadhwa5 opened a new pull request, #17176:
URL: https://github.com/apache/kafka/pull/17176
In this PR, the ShareGroupConfig has been moved to the share package. Since the config `GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG` from `GroupCoordinatorConfig` is required in `ShareGroupConfig`, and `share` cannot depend on `group-coordinator`, `GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG` has been moved to a separate config file called `CoordinatorCommonConfig` in `coordinator-common`
Reference - [KAFKA-17532](https://issues.apache.org/jira/browse/KAFKA-17532)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17532: Moved ShareGroupConfig to share module [kafka]
Posted by "chirag-wadhwa5 (via GitHub)" <gi...@apache.org>.
chirag-wadhwa5 closed pull request #17176: KAFKA-17532: Moved ShareGroupConfig to share module
URL: https://github.com/apache/kafka/pull/17176
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17532: Moved ShareGroupConfig to share module [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #17176:
URL: https://github.com/apache/kafka/pull/17176#discussion_r1756733579
##########
build.gradle:
##########
@@ -954,6 +956,8 @@ project(':share') {
dependencies {
implementation project(':server-common')
+ implementation project(':clients')
+ implementation project(':coordinator-common')
Review Comment:
Isn't there is a cycle i.e. `coordinator-common` requires `server`, `server` requires `share` and `share` requires `coordinator-common`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17532: Moved ShareGroupConfig to share module [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #17176:
URL: https://github.com/apache/kafka/pull/17176#discussion_r1756736312
##########
checkstyle/import-control-coordinator-common.xml:
##########
@@ -40,6 +40,9 @@
<subpackage name="coordinator">
<subpackage name="common">
+
+ <allow pkg="org.apache.kafka.common" />
Review Comment:
Line number 34 - 39 above defines common classes. Which common package do we need explitily, can we please include that above.
##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -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"),
Review Comment:
Why this change is required?
##########
checkstyle/import-control-share.xml:
##########
@@ -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" />
Review Comment:
Seems not the right section to include `coordinator`.
##########
core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala:
##########
@@ -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"))
Review Comment:
Why do we need to change `"Classic,Consumer,Share"`?
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala:
##########
@@ -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"),
Review Comment:
Agains same is `Classic` change is required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org