You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/02/24 02:37:20 UTC
[GitHub] [kafka] RivenSun2 opened a new pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
RivenSun2 opened a new pull request #11800:
URL: https://github.com/apache/kafka/pull/11800
Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050676944
@showuon
Sorry, I made a mistake, the `map` returned by `config.valuesWithPrefixOverride(prefix)` is not a normal map, it is of type `AbstractConfig.RecordingMap`.
`RecordingMap#get(Object key)` will call the `AbstractConfig#ignore(String key)` method to change the `AbstractConfig.used` variable.
Forgive my mistakes.
However, after `valuesWithPrefixOverride.get("sasl.mechanism")` add `assertFalse(config.unknown().contains("prefix.sasl.mechanism"));`,
testCase will fail to verify; because unknownKeys is only affected by originals and values, the value is originalKeys.removeAll(valueKeys).
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813663958
##########
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:
ok, I agree with you.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r820261017
##########
File path: clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
##########
@@ -376,11 +384,24 @@ private void logAll() {
log.info(b.toString());
}
+ public void logUnusedAndUnknown() {
Review comment:
Did we consider the fact that we don't know the config names for pluggable components?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050656591
In fact, in the testcases we discussed, `config.unused()` and `config.unknown()` will not change after `TestSecurityConfig config` is initialized.
The following `config.valuesWithPrefixOverride(prefix)` and `valuesWithPrefixOverride.get(xxxx)` will not cause the return value of `unused()` and `unknown()` to change
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813607110
##########
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:
Sure.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813606372
##########
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:
I don't want to change the existing behavior of unused() method, otherwise many testCases in AbstractConfigTest will fail.
Only when printing unusedKeys to the user, tell the user exactly which configurations are known but unused
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051842848
@showuon
okay, I agree with you. This is probably better than what I said above.
I will resubmit later, thanks for your suggestion.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 edited a comment on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 edited a comment on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051822201
@showuon I totally agree with you now. It was I forgot to modify a code that caused it to not meet our expectations.
In the methods `AbstractConfig#ignore(String key)` and `AbstractConfig#get(String key)`, add the following logic before `used.add(key)` :
`if (!unknown().contains(key))`
`used.add(key);`
What do you think?
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050652426
@showuon
you misunderstood me
1. A configuration is in unknown(), then it must not be in unused()
2. A configuration is not in unknown(), then it may be in unused()
3. A configuration is in unused(), then it must not be in unknown()
4. A configuration may be neither in unused() nor unknown() because it is **usedKey**.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813644961
##########
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:
Before this PR, the `unUsed` set contains both `unUsed` and `unKnown` configs, because we didn't separate them.
After this PR, we hope we can separate the `unUsed` and `unKnown`, so we created a new method `unKnown`. Under this situation, I don't think it makes sense to make `unUsed` set contains both `unUsed` and `unKnown`. That will just confuse other developers.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050632493
> Hi @showuon Thank you for your reply. In fact, in the AbstractConfigTest class, I have asserted `unknown()` after `config.valuesWithPrefixOverride(prefix)`. `valuesWithPrefixOverride` is just a normal map, which is returned by `config.valuesWithPrefixOverride(prefix)`. valuesWithPrefixOverride.get(xxxx) no longer affects config.
@RivenSun2 , are you saying there will be some configs that are not in `unused` configs, but will be in `unknown` configs? Does that make sense? Some configs are `unknown` but are `used` in Kafka? It doesn't sound correct to me.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 edited a comment on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 edited a comment on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050656591
In fact, in the testcases we discussed in `AbstractConfigTest`, `config.unused()` and `config.unknown()` will not change after `TestSecurityConfig config` is initialized.
The following `config.valuesWithPrefixOverride(prefix)` and `valuesWithPrefixOverride.get(xxxx)` will not cause the return value of `unused()` and `unknown()` to change
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813610827
##########
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 agree with you,thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813643248
##########
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:
Sure,I will add.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1061319723
@RivenSun2 , thanks for the help.
> Will the new PR title start with MINOR or still start with [KAFKA-13689](https://issues.apache.org/jira/browse/KAFKA-13689)?
Let's start with `KAFKA-13689`.
> For the configuration of some pluggable components of KafkaClients, will Kafka not know the specific configuration name? could you give an example?
Something like [KIP-519](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=128650952). Let's discuss it in KIP discussion. Thanks.
Thank you.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r820324135
##########
File path: clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
##########
@@ -376,11 +384,24 @@ private void logAll() {
log.info(b.toString());
}
+ public void logUnusedAndUnknown() {
Review comment:
TBH, no, I've never considered that! Thanks for the reminder.
@RivenSun2 , sorry, I forgot the `AbstractConfig` is a public API. But since the v3.2.0 KIP freeze has passed, and I think this KIP might need more discussion, could you submit another PR to revert this change first? After KIP discussion/voting passed, we can start to work on it again. If you don't have time to submit a revert PR within this week, please let me know. Thank you.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1061303039
Hi @showuon
I will create a new PR later to revert this code change.
Will the new PR title start with MINOR or still start with KAFKA-13689?
For the configuration of some pluggable components of KafkaClients, will Kafka not know the specific configuration name? could you give an example?
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050632493
> Hi @showuon Thank you for your reply. In fact, in the AbstractConfigTest class, I have asserted `unknown()` after `config.valuesWithPrefixOverride(prefix)`. `valuesWithPrefixOverride` is just a normal map, which is returned by `config.valuesWithPrefixOverride(prefix)`. valuesWithPrefixOverride.get(xxxx) no longer affects config.
@RivenSun2 , are you saying there will be some configs that are not in `unused` configs, but will be in `unknown` configs? Does that make sense? Some configs are `unknown` but are `used` in Kafka?
Please fix it. Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050660740
> In fact, in the testcases we discussed, `config.unused()` and `config.unknown()` will not change after `TestSecurityConfig config` is initialized. The following `config.valuesWithPrefixOverride(prefix)` and `valuesWithPrefixOverride.get(xxxx)` will not cause the return value of `unused()` and `unknown()` to change
Thanks for the explanation. But I don't think it is correct. If we check the original test below in `testValuesWithPrefixOverride`
```java
// prefix overrides global
assertTrue(config.unused().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"));
```
We can see the `unused` was expected to contain those 2 configs. Then after `valuesWithPrefixOverride.get("sasl.mechanism")`, they are used now, that is, not containing in `unused` set.
So, what I mean is that since we've changed the 1st assert into `unknown`:
```
assertTrue(config.unknown().contains("prefix.sasl.mechanism"));
```
Theh, we should change the last assert to assert unknown there:
```
assertFalse(config.unknown().contains("prefix.sasl.mechanism"));
```
Does that make sense?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 edited a comment on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 edited a comment on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050656591
In fact, in the testcases we discussed in `AbstractConfigTest`, `config.unused()` and `config.unknown()` will not change after `TestSecurityConfig config` is initialized.
The following `config.valuesWithPrefixOverride(prefix)` and `valuesWithPrefixOverride.get(xxxx)` will not cause the return value of `unused()` and `unknown()` to change.
The main purpose of these testCases is to test whether the Map value returned by AbstractConfig#valuesWithPrefixOverride meets our expectations under different conditions.
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 edited a comment on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 edited a comment on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1049673076
@showuon
Please help to check when you are available.
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813640006
##########
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 can add the mix of unknown and unused test case, no matter we want to change unused logics or not. That is, like the previous `testUnusedConfigs` test, we can add an unknown configs into it, and verify the unUsed and unknown results are as what we expected.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r814419358
##########
File path: clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java
##########
@@ -2866,6 +2866,25 @@ public void testUnknownConfigs() {
}
}
+ @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());
Review comment:
We should also assert the unUsed().size() == 1.
##########
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:
here, we should change to assert `unknown`, right?
Same as below.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r820159332
##########
File path: clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
##########
@@ -376,11 +384,24 @@ private void logAll() {
log.info(b.toString());
}
+ public void logUnusedAndUnknown() {
Review comment:
Oh... Thanks for the reminder. @RivenSun2 , could you summit a small KIP for this change? Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813618411
##########
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:
For KafkaClient, unknownKeys also belong to unusedKeys, what do you think?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1049673076
@showuon
Please check when you are available.
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051835536
> if (!unknown().contains(key)) {
used.add(key);
}
The reason we call `ignore` is because we indeed **use** this config key. In your logic, it'll make the **used** config keys not existed in `used` set (when it's unknown). It will confuse other developers, even though we can make the test pass.
I think we should remove all `used` keys in `unknown` method. That will exclude all `used` keys when retrieving `unknown` config keys. 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 edited a comment on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 edited a comment on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051822201
@showuon I totally agree with you now. It was I forgot to modify a code that caused it to not meet our expectations.
In the methods `AbstractConfig#ignore(String key)` and `AbstractConfig#get(String key)`, add the following logic before `used.add(key)` :
`if (!unknown().contains(key))`
`used.add(key);`
What do you think?
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051822201
@showuon I totally agree with you now. It was I forgot to modify a code that caused it to not meet our expectations.
In the methods `AbstractConfig#ignore(String key)` and `AbstractConfig#get(String key)`, add the following logic before `used.add(key)` :
`if (!unknown().contains(key))
used.add(key);`
What do you think?
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051847026
@showuon
I agree with you now, thank you for your suggestion.
Thanks again.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813609339
##########
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:
In fact, I don't recommend changing the current logic of the unused method, because for KafkaClient, unknownKeys also belong to unusedKeys, what do you think?
Of course, I would add mixed test cases.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1050597788
Hi @showuon Thank you for your reply.
In fact, in the AbstractConfigTest class, I have asserted `unknown()` after `config.valuesWithPrefixOverride(prefix)`.
`valuesWithPrefixOverride` is just a normal map, which is returned by `config.valuesWithPrefixOverride(prefix)`.
valuesWithPrefixOverride.get(xxxx) no longer affects 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051554030
> However, after valuesWithPrefixOverride.get("sasl.mechanism") add assertFalse(config.unknown().contains("prefix.sasl.mechanism"));,
testCase will fail to verify; because unknownKeys is only affected by originals and values, the value is originalKeys.removeAll(valueKeys).
So, could we also remove `used` config keys for `unknown` configs? Otherwise, there would be this strange case that a config is `used`, but `unknown` to kafka.
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051735512
Hi @showuon
Currently the logic of the `unknown()` method is exactly what you would expect.
`unknownKeys=originalKeys.removeAll(valueKeys)`
`unknownKeys` contains neither `usedKeys` nor `unusedKeys`.
Only configKeys that are not officially recognized by kafka will be put into `unknownKeys`.
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051784959
> unknownKeys contains neither usedKeys nor unusedKeys.
> Only configKeys that are not officially recognized by kafka will be put into unknownKeys.
Yes, this is what I expected. Thanks.
But that also confuses me: if `unknown` keys doesn't contain `used` and `unused`, why this test will fail after we assert `unknown`?
> However, after valuesWithPrefixOverride.get("sasl.mechanism") add assertFalse(config.unknown().contains("prefix.sasl.mechanism"));,
testCase will fail to verify; because unknownKeys is only affected by originals and values, the value is originalKeys.removeAll(valueKeys).
In the above test case, we originally expect that `prefix.sasl.mechanism` is `used` and is not in `unused` keys. After this PR, we should expected that `prefix.sasl.mechanism` is `used`, and not in `unused` and **NOT** in `unknown` keys, right? Did I miss anything here?
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1049437682
Hi @showuon @guozhangwang
Please help to review the PR .
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r813609339
##########
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:
In fact, I don't recommend changing the current logic of the unused method, because for KafkaClient, unknownKeys also belong to unusedKeys, what do you think?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051843328
> unknownKeys = originals.keySet().removeAll(values.keySet())
unknownKeys should not be changed after AbstractConfig is initialized.
That looks good, but I think that doesn't work in some cases, like the `prefix` case.
I'm not very familiar with the prefix logic, but I think the prefix is dynamic added to the `config key`. So that would make your assumption (unknownKeys should not be changed after AbstractConfig is initialized) failed. Could you make sure if your logic works in dynamic `prefix` case? (i.e. the `testValuesWithPrefixOverride` test case or other similar one)
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1051834698
Or we add an instance variable `AbstractConfig.unknownKeys`, we can complete the assignment to the variable `unknownKeys` when `AbstractConfig` is initialized.
`unknownKeys = originals.keySet().removeAll(values.keySet())`
`unknownKeys` should not be changed after `AbstractConfig` is initialized.
In this way, in the `unknown()` method, we directly return `unknownKeys` to avoid recalculating unknownKeys each time.
What do you think?
Thanks.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1049587798
@showuon
Thank you for your review, please check when you are available.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] RivenSun2 commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
RivenSun2 commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r814473563
##########
File path: clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java
##########
@@ -2866,6 +2866,25 @@ public void testUnknownConfigs() {
}
}
+ @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());
Review comment:
In fact, unused.size is not 1, I will add assert for it later.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#discussion_r820125031
##########
File path: clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
##########
@@ -376,11 +384,24 @@ private void logAll() {
log.info(b.toString());
}
+ public void logUnusedAndUnknown() {
Review comment:
This class is public and hence a kip is required for this https://javadoc.io/doc/org.apache.kafka/kafka-clients/latest/org/apache/kafka/common/config/AbstractConfig.html
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11800:
URL: https://github.com/apache/kafka/pull/11800#issuecomment-1059718434
@RivenSun2 , thanks for the contribution!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon merged pull request #11800: KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys
Posted by GitBox <gi...@apache.org>.
showuon merged pull request #11800:
URL: https://github.com/apache/kafka/pull/11800
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org