squah-confluent commented on code in PR #22302:
URL: https://github.com/apache/kafka/pull/22302#discussion_r3258384351
##########
server/src/main/java/org/apache/kafka/server/config/AbstractKafkaConfig.java:
##########
@@ -634,8 +634,14 @@ public Long logRetentionTimeMillis() {
public Map<String, Object> extractGroupConfigMap() {
Review Comment:
Since the method no longer returns the full broker contribution to group
configs, could we add a javadoc for this method explaining that it's only
suitable for use by `DescribeConfigs`?
##########
server/src/main/java/org/apache/kafka/server/config/AbstractKafkaConfig.java:
##########
@@ -634,8 +634,14 @@ public Long logRetentionTimeMillis() {
public Map<String, Object> extractGroupConfigMap() {
Map<String, Object> defaults = new HashMap<>();
- GroupConfig.ALL_GROUP_CONFIG_SYNONYMS.forEach((groupConfigName,
brokerConfigName) ->
- brokerConfigName.ifPresent(name -> defaults.put(groupConfigName,
get(name)))
+ Map<String, Object> brokerOriginals = originals();
+ GroupConfig.configNames().forEach(groupConfigName ->
+
GroupConfig.brokerSynonym(groupConfigName).ifPresent(brokerConfigName -> {
+ // Skip internal configs unless they are explicitly configured
via the broker synonym.
+ if (!GroupConfig.isInternal(groupConfigName) ||
brokerOriginals.containsKey(brokerConfigName)) {
Review Comment:
Should we also skip internal broker configs? What does `DescribeConfigs` do
for dynamic, internal broker configs right now?
##########
server/src/test/java/org/apache/kafka/server/config/AbstractKafkaConfigTest.java:
##########
@@ -52,4 +65,51 @@ public void testPopulateSynonymsOnMapWithoutBrokerId() {
expectedOutput.put(KRaftConfigs.NODE_ID_CONFIG, "4");
assertEquals(expectedOutput,
AbstractKafkaConfig.populateSynonyms(input));
}
+
+ @Test
+ public void testInternalConfigWithDefaultSynonymIsSkipped() {
+ Map<String, Object> config = mockInternalGroupConfigMap(Map.of(),
true);
+
+ assertFalse(config.containsKey(TEST_INTERNAL_GROUP_CONFIG));
+ }
+
+ @Test
+ public void testInternalConfigWithSetSynonymIsIncluded() {
+ Map<String, Object> config =
mockInternalGroupConfigMap(Map.of(TEST_INTERNAL_GROUP_CONFIG_BROKER_SYNONYM,
"override-value"), true);
+
+ assertTrue(config.containsKey(TEST_INTERNAL_GROUP_CONFIG));
+ assertEquals("override-value", config.get(TEST_INTERNAL_GROUP_CONFIG));
+ }
+
+ @Test
+ public void testNonInternalConfigIsIncluded() {
Review Comment:
`testExtractGroupConfigMapIncludesNonInternalConfig`?
##########
server/src/test/java/org/apache/kafka/server/config/AbstractKafkaConfigTest.java:
##########
@@ -52,4 +65,51 @@ public void testPopulateSynonymsOnMapWithoutBrokerId() {
expectedOutput.put(KRaftConfigs.NODE_ID_CONFIG, "4");
assertEquals(expectedOutput,
AbstractKafkaConfig.populateSynonyms(input));
}
+
+ @Test
+ public void testInternalConfigWithDefaultSynonymIsSkipped() {
Review Comment:
Can we make it clear that these new methods test `extractGroupConfigMap`
through the naming?
`testExtractGroupConfigMapExcludesInternalConfigWithUnconfiguredBrokerSynonym`?
##########
server/src/test/java/org/apache/kafka/server/config/AbstractKafkaConfigTest.java:
##########
@@ -52,4 +65,51 @@ public void testPopulateSynonymsOnMapWithoutBrokerId() {
expectedOutput.put(KRaftConfigs.NODE_ID_CONFIG, "4");
assertEquals(expectedOutput,
AbstractKafkaConfig.populateSynonyms(input));
}
+
+ @Test
+ public void testInternalConfigWithDefaultSynonymIsSkipped() {
+ Map<String, Object> config = mockInternalGroupConfigMap(Map.of(),
true);
+
+ assertFalse(config.containsKey(TEST_INTERNAL_GROUP_CONFIG));
+ }
+
+ @Test
+ public void testInternalConfigWithSetSynonymIsIncluded() {
Review Comment:
`testExtractGroupConfigMapIncludesInternalConfigWithConfiguredBrokerSynonym`?
##########
server/src/test/java/org/apache/kafka/server/config/AbstractKafkaConfigTest.java:
##########
@@ -52,4 +65,51 @@ public void testPopulateSynonymsOnMapWithoutBrokerId() {
expectedOutput.put(KRaftConfigs.NODE_ID_CONFIG, "4");
assertEquals(expectedOutput,
AbstractKafkaConfig.populateSynonyms(input));
}
+
+ @Test
+ public void testInternalConfigWithDefaultSynonymIsSkipped() {
+ Map<String, Object> config = mockInternalGroupConfigMap(Map.of(),
true);
+
+ assertFalse(config.containsKey(TEST_INTERNAL_GROUP_CONFIG));
+ }
+
+ @Test
+ public void testInternalConfigWithSetSynonymIsIncluded() {
+ Map<String, Object> config =
mockInternalGroupConfigMap(Map.of(TEST_INTERNAL_GROUP_CONFIG_BROKER_SYNONYM,
"override-value"), true);
+
+ assertTrue(config.containsKey(TEST_INTERNAL_GROUP_CONFIG));
+ assertEquals("override-value", config.get(TEST_INTERNAL_GROUP_CONFIG));
+ }
+
+ @Test
+ public void testNonInternalConfigIsIncluded() {
+ Map<String, Object> config = mockInternalGroupConfigMap(Map.of(),
false);
+
+ assertTrue(config.containsKey(TEST_INTERNAL_GROUP_CONFIG));
+ assertEquals("default-value", config.get(TEST_INTERNAL_GROUP_CONFIG));
+ }
+
+ private static Map<String, Object> mockInternalGroupConfigMap(Map<String,
Object> overrides, boolean isInternal) {
Review Comment:
It wasn't clear to me that `mockInternalGroupConfigMap` returned the
extracted group config. Perhaps we can rename it.
```suggestion
private static Map<String, Object> extractGroupConfigMap(Map<String,
Object> brokerProps, boolean isInternal) {
```
--
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]
