showuon commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813598311
##########
File path:
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
##########
@@ -377,10 +383,21 @@ private void logAll() {
}
/**
- * Log warnings for any unused configurations
+ * Log infos for any unused configurations
*/
public void logUnused() {
- for (String key : unused())
+ Set<String> unusedKeys = unused();
+ //Printing unusedKeys to users should remove unknownKeys
+ unusedKeys.removeAll(unknown());
Review comment:
Could we move this `remove unknown` keys into `unUsed` method, and let
`logUnused` only do the `log` thing?
##########
File path:
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java
##########
@@ -2849,6 +2850,20 @@ public void testUnusedConfigs() {
}
}
+ @Test
+ public void testUnknownConfigs() {
+ Map<String, Object> props = new HashMap<>();
+ props.put(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999");
+ props.put(unknownTestConfig, "my_value");
+ ConsumerConfig config = new
ConsumerConfig(ConsumerConfig.appendDeserializerToConfig(props, new
StringDeserializer(), new StringDeserializer()));
+
+ assertTrue(config.unknown().contains(unknownTestConfig));
Review comment:
I think we should also verify the `config.unknown()` has only 1 element,
that is: `unknownTestConfig`. In current verification, there could be many
`unknown` in the configs, but still pass the test. Same as below.
##########
File path:
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java
##########
@@ -2849,6 +2850,20 @@ public void testUnusedConfigs() {
}
}
+ @Test
+ public void testUnknownConfigs() {
Review comment:
I think we should also test the mix of unknown and unused configs case,
because we have some logic for them, right?
##########
File path:
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
##########
@@ -813,6 +813,7 @@ public KafkaConsumer(Map<String, Object> configs,
this.kafkaConsumerMetrics = new KafkaConsumerMetrics(metrics,
metricGrpPrefix);
config.logUnused();
+ config.logUnknown();
Review comment:
It's a little weird and easy to forget to call these 2 methods after
client initialization. Could we create a `public logUnknownAndUnused` method,
and make both `logUnused` and `logUnknown` as private, so that users will
always call `logUnknownAndUnused` to log both. WDYT?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
