lh0156 commented on code in PR #22935:
URL: https://github.com/apache/kafka/pull/22935#discussion_r3687587050
##########
core/src/test/scala/unit/kafka/server/ControllerConfigurationValidatorTest.scala:
##########
@@ -184,6 +184,41 @@ class ControllerConfigurationValidatorTest {
validator.validate(new ConfigResource(GROUP, "group"), config, emptyMap())
}
+ @Test
+ def testGroupConfigChangeIgnoresUnchangedOutOfRangeSessionTimeout(): Unit = {
+ val existingConfig = new util.TreeMap[String, String]()
Review Comment:
Addressed in 15e4b42. The affected tests now use `oldConfig`, matching the
alter-validation parameter naming.
##########
core/src/test/scala/unit/kafka/server/ControllerConfigurationValidatorTest.scala:
##########
@@ -184,6 +184,41 @@ class ControllerConfigurationValidatorTest {
validator.validate(new ConfigResource(GROUP, "group"), config, emptyMap())
}
+ @Test
+ def testGroupConfigChangeIgnoresUnchangedOutOfRangeSessionTimeout(): Unit = {
+ val existingConfig = new util.TreeMap[String, String]()
+ existingConfig.put(GroupConfig.CONSUMER_SESSION_TIMEOUT_MS_CONFIG, "90000")
+
+ val newConfig = new util.TreeMap[String, String](existingConfig)
+ newConfig.put(GroupConfig.CONSUMER_HEARTBEAT_INTERVAL_MS_CONFIG, "6000")
+
+ validator.validate(new ConfigResource(GROUP, "group"), newConfig,
existingConfig)
+ }
+
+ @Test
+ def testGroupConfigChangeIgnoresUnchangedOutOfRangeHeartbeatInterval(): Unit
= {
Review Comment:
Addressed in 15e4b42. This test now covers the evaluated-old-session-timeout
case, while `testGroupConfigChangeIgnoresUnchangedOutOfRangeSessionTimeout`
covers the unchanged-value path; the cases exercise different effective
configurations.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfig.java:
##########
@@ -489,6 +492,86 @@ public static void validate(
);
}
+ /**
+ * Check a group configuration after an alter operation.
+ *
+ * <p>Only values changed by the operation are validated against the
current broker-level
+ * bounds. Values that were already stored may be outside those bounds
after a broker
+ * configuration change, but they must not prevent an unrelated group
configuration from
+ * being updated. Cross-field validation still considers the effective
value of every
+ * involved setting.</p>
+ *
+ * @param newGroupConfig The complete group config after the alter
operation.
+ * @param existingGroupConfig The complete group config before the
alter operation.
+ * @param groupCoordinatorConfig The group coordinator config.
+ * @param shareGroupConfig The share group config.
+ */
+ public static void validate(
+ Map<String, String> newGroupConfig,
+ Map<String, String> existingGroupConfig,
Review Comment:
Addressed in 15e4b42. The local variable is now named `oldGroupConfig`
consistently with the validation API and Javadoc.
##########
core/src/test/scala/unit/kafka/server/ControllerConfigurationValidatorTest.scala:
##########
@@ -184,6 +184,41 @@ class ControllerConfigurationValidatorTest {
validator.validate(new ConfigResource(GROUP, "group"), config, emptyMap())
}
+ @Test
+ def testGroupConfigChangeIgnoresUnchangedOutOfRangeSessionTimeout(): Unit = {
+ val existingConfig = new util.TreeMap[String, String]()
+ existingConfig.put(GroupConfig.CONSUMER_SESSION_TIMEOUT_MS_CONFIG, "90000")
Review Comment:
Addressed in 15e4b42. The tests now document the current valid
session-timeout and heartbeat-interval ranges next to the boundary values.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfig.java:
##########
Review Comment:
I rechecked this against the current tree: the three-argument
`GroupConfig.validate` method is still used by the existing `GroupConfigTest`
validation cases, so it is not unused and should remain.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
