AndrewJSchofield commented on code in PR #17046:
URL: https://github.com/apache/kafka/pull/17046#discussion_r1753746159


##########
core/src/test/java/kafka/admin/ConfigCommandIntegrationTest.java:
##########
@@ -620,6 +699,17 @@ private void deleteAndVerifyGroupConfigValue(Admin client,
         verifyGroupConfig(client, groupName, defaultConfigs);
     }
 
+    private void deleteAndVerifyClientMetricsConfigValue(Admin client,
+                                                         String 
clientMetricsName,
+                                                         Map<String, String> 
defaultConfigs) throws Exception {

Review Comment:
   Ah, there are multiple points here. Thanks for your patience.
   
   If you do `--describe --entity-type topics`, it lists the configs for all 
topics. If you do `--describe --entity-type client-metrics`, it lists configs 
for all client metrics resources. If you do `--describe --entity-type groups`, 
it would ideally list all of the group config resources, but there is no RPC 
capable of doing that so instead it lists the consumer groups with configs.
   
   The `--entity-default` situation is tricky. None of topics, groups or 
client-metrics works with `--entity-default`. Making it required to specify an 
entity name for these types would break the ability to list the configs for all 
resources. `--entity-default` should only be permitted for clients, users, ips 
and brokers, but there's no explicit check for this.
   
   I will revert my change for group with no entity name, and add a check for 
`--entity-default` to make sure it's only specified with entity types that 
genuinely support it.
   
   There is also the point about default values for the configs themselves. So, 
if I have a group G1 with just `consumer.heartbeat.interval.ms` defined, then 
this is the output:
   
   ```
   $ bin/kafka-configs.sh --bootstrap-server localhost:9092 --describe 
--entity-type groups --entity-name G1
   Dynamic configs for group G1 are:
     consumer.heartbeat.interval.ms=5000 sensitive=false 
synonyms={DYNAMIC_GROUP_CONFIG:consumer.heartbeat.interval.ms=5000}
   
   $ bin/kafka-configs.sh --bootstrap-server localhost:9092 --describe 
--entity-type groups --entity-name G1 --all
   All configs for group G1 are:
     consumer.heartbeat.interval.ms=5000 sensitive=false 
synonyms={DYNAMIC_GROUP_CONFIG:consumer.heartbeat.interval.ms=5000}
     consumer.session.timeout.ms=45000 sensitive=false synonyms={}
   ```
   
   Currently, client-metrics does not behave the same (and it should). If a 
client-metrics resource defines a subset of the allowed configs, the `--all` 
does not display the default values for the missing configs. I'll address that 
in KAFKA-17516.
   



-- 
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]

Reply via email to