squah-confluent commented on code in PR #22935:
URL: https://github.com/apache/kafka/pull/22935#discussion_r3652147114
##########
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:
nit: Can we call this `oldConfig` to match the `validate()` parameter name?
Same for the other tests in this PR.
##########
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:
Could you add a short comment stating the valid range of
`CONSUMER_SESSION_TIMEOUT_MS_CONFIG`?
Same for the other tests in this PR.
##########
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,
+ GroupCoordinatorConfig groupCoordinatorConfig,
+ ShareGroupConfig shareGroupConfig
+ ) {
Review Comment:
I'm not a fan of duplicating the validation logic. What if we overlay the
changed group configs on top of the "evaluate"d old group config and run that
through validation instead?
##########
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:
What does this test cover that isn't covered by
`testGroupConfigChangeIgnoresUnchangedOutOfRangeSessionTimeout`?
##########
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:
nit: Can we name this `oldGroupConfig`?
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfig.java:
##########
Review Comment:
This method is now unused.
--
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]
