showuon commented on a change in pull request #11800: URL: https://github.com/apache/kafka/pull/11800#discussion_r814517759
########## File path: clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java ########## @@ -2849,6 +2850,45 @@ 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)); + assertEquals(1, config.unknown().size()); + assertEquals(3, config.unused().size()); Review comment: I these assert here is not meaningful. We can remove them and keep the asserts after consumer created below. ########## File path: clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java ########## @@ -2849,6 +2850,45 @@ 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)); + assertEquals(1, config.unknown().size()); + assertEquals(3, config.unused().size()); + + try (KafkaConsumer<byte[], byte[]> consumer = new KafkaConsumer<>(config, null, null)) { + assertTrue(config.unknown().contains(unknownTestConfig)); + assertEquals(1, config.unknown().size()); + assertEquals(0, config.unused().size()); + } + } + + @Test + public void testUnusedAndUnknownConfigs() { + Map<String, Object> props = new HashMap<>(); + props.put(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + props.put(SslConfigs.SSL_PROTOCOL_CONFIG, "TLS"); + props.put(unknownTestConfig, "my_value"); + ConsumerConfig config = new ConsumerConfig(ConsumerConfig.appendDeserializerToConfig(props, new StringDeserializer(), new StringDeserializer())); + + assertTrue(config.unused().contains(SslConfigs.SSL_PROTOCOL_CONFIG)); + assertTrue(config.unknown().contains(unknownTestConfig)); + assertEquals(1, config.unknown().size()); + assertEquals(4, config.unused().size()); Review comment: Same here, we can just keep the asserts after consumer created ########## File path: clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java ########## @@ -1852,6 +1853,49 @@ public void testUnusedConfigs() { } } + @Test + public void testUnknownConfigs() { + Map<String, Object> props = new HashMap<>(); + props.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + props.put(unknownTestConfig, "my_value"); + ProducerConfig config = new ProducerConfig(ProducerConfig.appendSerializerToConfig(props, + new StringSerializer(), new StringSerializer())); + + assertTrue(config.unknown().contains(unknownTestConfig)); + assertEquals(1, config.unknown().size()); + assertEquals(3, config.unused().size()); Review comment: Same as here, and other places ########## File path: clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java ########## @@ -102,21 +104,21 @@ public void testValuesWithPrefixOverride() { Map<String, Object> valuesWithPrefixOverride = config.valuesWithPrefixOverride(prefix); // prefix overrides global - assertTrue(config.unused().contains("prefix.sasl.mechanism")); + assertTrue(config.unknown().contains("prefix.sasl.mechanism")); assertTrue(config.unused().contains("sasl.mechanism")); assertEquals("GSSAPI", valuesWithPrefixOverride.get("sasl.mechanism")); assertFalse(config.unused().contains("sasl.mechanism")); assertFalse(config.unused().contains("prefix.sasl.mechanism")); Review comment: Should we add `assertFalse(config.unknown().contains("prefix.sasl.mechanism"));` here? Because we assert unknown before using `prefix.sasl.mechanism` config (i.e. `valuesWithPrefixOverride.get("sasl.mechanism")`), we should expect `prefix.sasl.mechanism` is not unknown after using it, right? ########## File path: clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java ########## @@ -102,21 +104,21 @@ public void testValuesWithPrefixOverride() { Map<String, Object> valuesWithPrefixOverride = config.valuesWithPrefixOverride(prefix); // prefix overrides global - assertTrue(config.unused().contains("prefix.sasl.mechanism")); + assertTrue(config.unknown().contains("prefix.sasl.mechanism")); assertTrue(config.unused().contains("sasl.mechanism")); assertEquals("GSSAPI", valuesWithPrefixOverride.get("sasl.mechanism")); assertFalse(config.unused().contains("sasl.mechanism")); assertFalse(config.unused().contains("prefix.sasl.mechanism")); // prefix overrides default - assertTrue(config.unused().contains("prefix.sasl.kerberos.kinit.cmd")); + assertTrue(config.unknown().contains("prefix.sasl.kerberos.kinit.cmd")); assertFalse(config.unused().contains("sasl.kerberos.kinit.cmd")); assertEquals("/usr/bin/kinit2", valuesWithPrefixOverride.get("sasl.kerberos.kinit.cmd")); assertFalse(config.unused().contains("sasl.kerberos.kinit.cmd")); assertFalse(config.unused().contains("prefix.sasl.kerberos.kinit.cmd")); Review comment: same here and below, we should assert false to `(config.unknown().contains("prefix.sasl.kerberos.kinit.cmd")` after using that config. -- 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