KAFKA-14511: extend AlterIncrementalConfigs API to support group config by DL1231 · Pull Request #15067 · apache/kafka

DL1231 · 2023-12-24T08:25:56Z

This PR add resources to store and handle consumer group config. jira

Changes include:

Adding GRUOP to resource type
Corresponding DYNAMIC consumer group configurations in resources.
Changes to support dynamic loading of configuration on changes.
Test cases for the changes

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

AndrewJSchofield

KIP-848 introduces the new GROUP resource type with the intent that it will be used for more than just consumer groups in the future.

# Conflicts: # core/src/main/scala/kafka/zk/AdminZkClient.scala # core/src/test/scala/unit/kafka/server/KafkaApisTest.scala

DL1231 · 2024-01-10T03:10:34Z

@AndrewJSchofield, I've updated the PR. Please take a look again. Thanks.

AndrewJSchofield

This PR is looking pretty good now. Thanks.

# Conflicts: # core/src/main/scala/kafka/server/KafkaConfig.scala

DL1231 · 2024-01-14T03:12:21Z

Thanks @AndrewJSchofield for the feedback, I have addressed comments.

AndrewJSchofield

lgtm

DL1231 · 2024-01-17T12:00:10Z

@dajac, PTAL, thanks in advance.

# Conflicts: # core/src/main/scala/kafka/server/BrokerServer.scala # core/src/main/scala/kafka/server/KafkaConfig.scala # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java # group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java

github-actions · 2024-05-21T03:33:42Z

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

AndrewJSchofield · 2024-06-06T15:39:22Z

@DL1231 I'm interested in getting group configs working. This PR is quite out of date, in particular because there's been a lot of refactoring of configs in Kafka recently. Would you like to rebase it and get it working again? Alternatively, I'm happy to take the work on instead.

# Conflicts: # core/src/main/scala/kafka/security/authorizer/AclEntry.scala # core/src/main/scala/kafka/server/BrokerServer.scala # core/src/main/scala/kafka/server/ConfigHandler.scala # core/src/main/scala/kafka/server/KafkaConfig.scala # core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala # core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java # group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java # group-coordinator/src/test/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntimeTest.java

DL1231 · 2024-06-15T06:33:15Z

@AndrewJSchofield I've resolved the conflict, PTAL, thanks in advance.

AndrewJSchofield · 2024-06-17T07:52:24Z

@DL1231 Yes, I'll give the updated code a detailed review in the next few days. Thanks for rebasing it.

AndrewJSchofield

Thanks for the updated PR. I left a few comments.

# Conflicts: # core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala

DL1231 · 2024-06-20T02:11:02Z

@AndrewJSchofield I've updated the PR. Please take a look again. Thanks.

AndrewJSchofield

lgtm. Thanks for the PR.

omkreddy

@DL1231 Thanks for the PR. changes LGTM.

will wait for @dajac review

dajac

@DL1231 Thanks for the update. I just made another pass on the patch and I left some comments for consideration.

dajac · 2024-07-02T11:26:04Z

+
+  @ParameterizedTest
+  @ValueSource(strings = Array("kraft+kip848"))
+  def testIncrementalAlterDefaultGroupConfig(quorum: String): Unit = {


nit: testDynamicGroupConfigChangeWithInvalidValue?

Consider aligning with testIncrementalAlterDefaultTopicConfig

dajac · 2024-07-02T11:27:51Z

+      val resource = new ConfigResource(ConfigResource.Type.GROUP, "")
+      val op = new AlterConfigOp(new ConfigEntry(GroupConfig.CONSUMER_SESSION_TIMEOUT_MS_CONFIG, "200000"), OpType.SET)
+      val future = admin.incrementalAlterConfigs(Map(resource -> List(op).asJavaCollection).asJava).all
+      TestUtils.assertFutureExceptionTypeEquals(future, classOf[InvalidRequestException])


I wonder if it should rather be a InvalidConfigurationException exception. What's the reasoning for using InvalidRequestException?

Consider aligning with testIncrementalAlterDefaultTopicConfig

dajac · 2024-07-02T12:02:13Z

+        super(CONFIG, props, false);
+    }
+
+    public static Set<String> configNames() {


configNames is only used in validateNames. Should we remove it?

It is also used in testFromPropsInvalid.

# Conflicts: # core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala

dajac · 2024-07-05T09:38:05Z

I'll be away until 7/29. I will continue reviewing this PR when I come back.

DL1231 · 2024-07-07T07:34:30Z

@dajac Sorry for the delay, I've updated the PR, PTAL when you get a chance.

# Conflicts: # group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java # group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTestContext.java

dajac · 2024-07-30T13:30:34Z

@DL1231 I am back. Thanks for the update. I will review your PR asap.

dajac

@DL1231 Thanks for the update and please excuse me for the delay on this one. I went thought it again and left a few minor comments. I think that we can merge it when they are addressed.

dajac · 2024-08-08T07:44:40Z

+        CLIENT_METRICS((byte) 16),
+        BROKER_LOGGER((byte) 8),
+        BROKER((byte) 4),
+        GROUP((byte) 3),


I disagree with this change. I think that GROUP should use 32 in order to stay consistent with the existing values.

dajac · 2024-08-08T07:57:12Z

+    }
+
+    /**
+     * Create a group config instance using the given properties and defaults


nit: Let's add a . at the end of the sentence for consistency.

dajac · 2024-08-08T08:06:45Z

+  /**
+   * Copy the subset of properties that are relevant to consumer group.
+   */
+  def extractGroupConfigMap: java.util.Map[String, Int] = {
+    val groupProps = new java.util.HashMap[String, Int]()
+    groupProps.put(GroupConfig.CONSUMER_SESSION_TIMEOUT_MS_CONFIG, groupCoordinatorConfig.consumerGroupSessionTimeoutMs)
+    groupProps.put(GroupConfig.CONSUMER_HEARTBEAT_INTERVAL_MS_CONFIG, groupCoordinatorConfig.consumerGroupHeartbeatIntervalMs)
+    groupProps
+  }


Would it make sense to move this method to GroupCoordinatorConfig? GroupCoordinatorConfig has changed quite a lot since this PR was started. It seems to me that keeping all the group related logic there would make sense. What do you think?

If we do this, we could also simplify GroupConfigManager.validate.

Yes it makes sense to me. Moved this method to GroupCoordinatorConfig.

# Conflicts: # core/src/main/scala/kafka/server/BrokerServer.scala # core/src/test/scala/unit/kafka/server/ControllerConfigurationValidatorTest.scala

DL1231 · 2024-08-08T14:27:01Z

Hi @dajac. Thanks a lot for the review. I have made the required changes in the last commit, PTAL when you get a chance.

dajac

LGTM. Thanks for the patch, @DL1231! I will merge it when the build completes.

dajac · 2024-08-09T11:40:02Z

I spoke too quickly. @DL1231 There are related failed tests. For instance:

testInvalidProps() – org.apache.kafka.coordinator.group.GroupConfigTest
Could you please check?

dajac · 2024-08-09T12:40:04Z

@DL1231 The last build failed with compilation errors. Could you please check? I would also advice to not rebase and force-push from now on. It will be easier for me to follow the fixes. Otherwise, I have to go through the entire PR.

KAFKA-14511: extend AlterIncrementalConfigs API to support group config

3aeb05a

dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Dec 24, 2023

AndrewJSchofield requested changes Jan 9, 2024

View reviewed changes

DL1231 added 2 commits January 10, 2024 10:43

replace 'consumer group' with 'group'

325e07e

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

853f91a

# Conflicts: # core/src/main/scala/kafka/zk/AdminZkClient.scala # core/src/test/scala/unit/kafka/server/KafkaApisTest.scala

AndrewJSchofield reviewed Jan 12, 2024

View reviewed changes

Comment thread clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Outdated

Comment thread core/src/main/scala/kafka/server/ConfigHandler.scala

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfig.java Outdated

DL1231 added 2 commits January 14, 2024 11:03

Addressing review comments

4f5b3ac

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

a8aa862

# Conflicts: # core/src/main/scala/kafka/server/KafkaConfig.scala

AndrewJSchofield approved these changes Jan 17, 2024

View reviewed changes

github-actions Bot added the stale Stale PRs label May 21, 2024

github-actions Bot removed the stale Stale PRs label Jun 7, 2024

DL1231 force-pushed the KAFKA-14511 branch 2 times, most recently from 0657ace to ab95c05 Compare June 15, 2024 07:36

fix conflict

3110031

DL1231 force-pushed the KAFKA-14511 branch from ab95c05 to 3110031 Compare June 16, 2024 07:27

AndrewJSchofield requested changes Jun 18, 2024

View reviewed changes

Comment thread core/src/main/scala/kafka/server/KafkaApis.scala

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java

DL1231 added 2 commits June 20, 2024 10:00

Addressing review comments

29d28c6

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

34d26bf

# Conflicts: # core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala

AndrewJSchofield approved these changes Jun 24, 2024

View reviewed changes

omkreddy approved these changes Jun 26, 2024

View reviewed changes

fix conflict

8a4a1f2

dajac reviewed Jul 2, 2024

View reviewed changes

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

2107cd2

# Conflicts: # core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala

DL1231 added 2 commits July 7, 2024 15:16

Addressing review comments

e556e2a

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

5a0bf70

pranavrth reviewed Jul 26, 2024

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/config/ConfigResource.java

DL1231 added 2 commits July 30, 2024 19:44

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

6a58c25

# Conflicts: # group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java # group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTestContext.java

fix conflict

b2af892

DL1231 requested a review from dajac July 30, 2024 12:25

dajac reviewed Aug 8, 2024

View reviewed changes

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

76376f3

# Conflicts: # core/src/main/scala/kafka/server/BrokerServer.scala # core/src/test/scala/unit/kafka/server/ControllerConfigurationValidatorTest.scala

DL1231 force-pushed the KAFKA-14511 branch from ee9c38e to 4a827fc Compare August 8, 2024 14:15

DL1231 force-pushed the KAFKA-14511 branch 5 times, most recently from 767e756 to 10b9d24 Compare August 9, 2024 07:50

dajac approved these changes Aug 9, 2024

View reviewed changes

Addressing review comments

2c764c4

DL1231 force-pushed the KAFKA-14511 branch from 10b9d24 to 2c764c4 Compare August 9, 2024 12:08

DL1231 added 2 commits August 9, 2024 21:11

Merge remote-tracking branch 'origin/trunk' into KAFKA-14511

20c1c2a

fix conflict

259751a

dajac approved these changes Aug 12, 2024

View reviewed changes

dajac merged commit bbdf79e into apache:trunk Aug 12, 2024

Conversation

DL1231 commented Dec 24, 2023

Committer Checklist (excluded from commit message)

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DL1231 commented Jan 10, 2024

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DL1231 commented Jan 14, 2024

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

DL1231 commented Jan 17, 2024

Uh oh!

github-actions Bot commented May 21, 2024

Uh oh!

AndrewJSchofield commented Jun 6, 2024

Uh oh!

DL1231 commented Jun 15, 2024

Uh oh!

AndrewJSchofield commented Jun 17, 2024

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DL1231 commented Jun 20, 2024

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

omkreddy left a comment

Choose a reason for hiding this comment

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

DL1231 Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac commented Jul 5, 2024

Uh oh!

DL1231 commented Jul 7, 2024

Uh oh!

Uh oh!

dajac commented Jul 30, 2024

Uh oh!

DL1231 Jul 7, 2024 •

edited

Loading