KAFKA-20409: Don't expose internal group configs unless they are user-defined#22302
Conversation
github-actions
Bot
added
triage
|
This PR resolves the issue discussed here: #21926. Apologies for the delay in getting this done! @chia7712 @dajac @squah-confluent Could you please take a look and review it when you have a moment? Thanks! |
squah-confluent
left a comment
squah-confluent
left a comment
There was a problem hiding this comment.
Thanks for addressing the bug!
| @@ -634,8 +634,14 @@ public Long logRetentionTimeMillis() { | |||
|
|
|||
| public Map<String, Object> extractGroupConfigMap() { | |||
There was a problem hiding this 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?
There was a problem hiding this comment.
Thanks for the suggestion, updated!
| } | ||
|
|
||
| @Test | ||
| public void testInternalConfigWithDefaultSynonymIsSkipped() { |
There was a problem hiding this comment.
Can we make it clear that these new methods test extractGroupConfigMap through the naming?
testExtractGroupConfigMapExcludesInternalConfigWithUnconfiguredBrokerSynonym?
| } | ||
|
|
||
| @Test | ||
| public void testInternalConfigWithSetSynonymIsIncluded() { |
There was a problem hiding this comment.
testExtractGroupConfigMapIncludesInternalConfigWithConfiguredBrokerSynonym?
| } | ||
|
|
||
| @Test | ||
| public void testNonInternalConfigIsIncluded() { |
There was a problem hiding this comment.
testExtractGroupConfigMapIncludesNonInternalConfig?
| assertEquals("default-value", config.get(TEST_INTERNAL_GROUP_CONFIG)); | ||
| } | ||
|
|
||
| private static Map<String, Object> mockInternalGroupConfigMap(Map<String, Object> overrides, boolean isInternal) { |
There was a problem hiding this comment.
It wasn't clear to me that mockInternalGroupConfigMap returned the extracted group config. Perhaps we can rename it.
| private static Map<String, Object> mockInternalGroupConfigMap(Map<String, Object> overrides, boolean isInternal) { | |
| private static Map<String, Object> extractGroupConfigMap(Map<String, Object> brokerProps, boolean isInternal) { |
|
Thanks for the review and suggestions, @squah-confluent ! I've made the updates. Please take another look when you have a chance. |
squah-confluent
left a comment
squah-confluent
left a comment
There was a problem hiding this comment.
Thanks for the updates!
…-defined (apache#22302) Previously, `internal group configs` with a `broker-level synonym` were always included in the group config map, exposing them even when the user had never explicitly set them. This change skips such configs unless the user has configured them either via the broker synonym or directly at the group level. Reviewers: Sean Quah <squah@confluent.io>
|
|
||
| AbstractKafkaConfig kafkaConfig = new AbstractKafkaConfig(configDef, new HashMap<>(brokerProps), Map.of(), false) { | ||
| @Override | ||
| public void addReconfigurable(Reconfigurable reconfigurable) { } |
There was a problem hiding this comment.
@majialoong It seems the current abstraction is a bit suboptimal and forces unnecessary dummy implementation in tests. would you mind providing empty implementation for these two methods to clean up the codebase
…d removeReconfigurable in AbstractKafkaConfig (#22342) Ref: #22302 (comment). Convert `addReconfigurable` and `removeReconfigurable` from abstract to default no-op methods so tests no longer need to supply dummy overrides. Reviewers: Ken Huang <s7133700@gmail.com>, Murali Basani <muralidhar.basani@aiven.io>, Chia-Ping Tsai <chia7712@gmail.com>
…d removeReconfigurable in AbstractKafkaConfig (apache#22342) Ref: apache#22302 (comment). Convert `addReconfigurable` and `removeReconfigurable` from abstract to default no-op methods so tests no longer need to supply dummy overrides. Reviewers: Ken Huang <s7133700@gmail.com>, Murali Basani <muralidhar.basani@aiven.io>, Chia-Ping Tsai <chia7712@gmail.com>
4 participants
Previously,
internal group configswith abroker-level synonymwerealways included in the group config map, exposing them even when the
user had never explicitly set them. This change skips such configs
unless the user has configured them either via the broker synonym or
directly at the group level.
Reviewers: Sean Quah squah@confluent.io