KAFKA-13689: Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys by RivenSun2 · Pull Request #11800 · apache/kafka

RivenSun2 · 2022-02-24T02:37:05Z

Optimized the printing of AbstractConfig logs, and stripped unknownKeys from unusedKeys

… cpu and traffic on the broker side increase sharply JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

…sets method JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

2. Optimize the import of package Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

add test Method "testForceMetadataDeleteForPatternSubscriptionDuringRebalance()" Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Author: RivenSun2 riven.sun@zoom.us

� Conflicts: � clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java

…ped unknownKeys from unusedKeys

RivenSun2 · 2022-02-24T02:39:34Z

Hi @showuon @guozhangwang
Please help to review the PR .
Thanks.

showuon

@RivenSun2 , thanks for the PR. Left some comments.

showuon · 2022-02-24T07:03:45Z

-        for (String key : unused())
+        Set<String> unusedKeys = unused();
+        //Printing unusedKeys to users should remove unknownKeys
+        unusedKeys.removeAll(unknown());


Could we move this remove unknown keys into unUsed method, and let logUnused only do the log thing?

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

For KafkaClient, unknownKeys also belong to unusedKeys, what do you think?

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.

ok, I agree with you.

showuon · 2022-02-24T07:07:44Z

+        props.put(unknownTestConfig, "my_value");
+        ConsumerConfig config = new ConsumerConfig(ConsumerConfig.appendDeserializerToConfig(props, new StringDeserializer(), new StringDeserializer()));
+
+        assertTrue(config.unknown().contains(unknownTestConfig));


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.

I agree with you,thanks.

showuon · 2022-02-24T07:08:34Z

    }

+    @Test
+    public void testUnknownConfigs() {


I think we should also test the mix of unknown and unused configs case, because we have some logic for them, right?

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?

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.

Sure,I will add.

showuon · 2022-02-24T07:11:48Z

            config.logUnused();
+            config.logUnknown();


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?

RivenSun2 · 2022-02-24T08:00:00Z

@showuon
Thank you for your review, please check when you are available.

RivenSun2 · 2022-02-24T09:48:53Z

@showuon
Please help to check when you are available.
Thanks.

showuon

@RivenSun2 , thanks for the update. Left some comments.

showuon · 2022-02-25T02:02:18Z

+
+        assertTrue(config.unused().contains(SslConfigs.SSL_PROTOCOL_CONFIG));
+        assertTrue(config.unknown().contains(unknownTestConfig));
+        assertEquals(1, config.unknown().size());


We should also assert the unUsed().size() == 1.

In fact, unused.size is not 1, I will add assert for it later.

showuon · 2022-02-25T02:17:59Z

        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"));


here, we should change to assert unknown, right?
Same as below.

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?

showuon

Thanks for the update. Left some comments.

showuon · 2022-02-25T06:55:28Z

+        assertEquals(1, config.unknown().size());
+        assertEquals(3, config.unused().size());


I these assert here is not meaningful. We can remove them and keep the asserts after consumer created below.

showuon · 2022-02-25T06:56:14Z

+        assertTrue(config.unused().contains(SslConfigs.SSL_PROTOCOL_CONFIG));
+        assertTrue(config.unknown().contains(unknownTestConfig));
+        assertEquals(1, config.unknown().size());
+        assertEquals(4, config.unused().size());


Same here, we can just keep the asserts after consumer created

showuon · 2022-02-25T06:56:44Z

+        assertEquals(1, config.unknown().size());
+        assertEquals(3, config.unused().size());


Same as here, and other places

showuon · 2022-02-25T06:58:09Z

        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"));


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?

showuon · 2022-02-25T07:00:27Z

        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"));


same here and below, we should assert false to (config.unknown().contains("prefix.sasl.kerberos.kinit.cmd") after using that config.

RivenSun2 · 2022-02-25T07:18:31Z

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.

showuon · 2022-02-25T08:17:51Z

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.

RivenSun2 · 2022-02-25T08:45:58Z

@showuon
you misunderstood me

A configuration is in unknown(), then it must not be in unused()
A configuration is not in unknown(), then it may be in unused()
A configuration is in unused(), then it must not be in unknown()
A configuration may be neither in unused() nor unknown() because it is usedKey.

RivenSun2 · 2022-02-25T08:52:03Z

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.

showuon · 2022-02-25T08:57:39Z

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

       // 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?

RivenSun2 · 2022-02-25T09:18:32Z

@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).

showuon · 2022-02-26T03:49:12Z

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.

RivenSun2 · 2022-02-26T07:14:41Z

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.

showuon · 2022-02-26T07:55:30Z

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.

RivenSun2 · 2022-02-26T08:26:10Z

@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.

RivenSun2 · 2022-02-26T08:36:28Z

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.

showuon · 2022-02-26T08:37:09Z

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?

RivenSun2 · 2022-02-26T08:43:12Z

@showuon
okay, I agree with you. This is probably better than what I said above.
I will resubmit later, thanks for your suggestion.

showuon · 2022-02-26T08:43:37Z

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.

RivenSun2 · 2022-02-26T08:46:38Z

@showuon
I agree with you now, thank you for your suggestion.
Thanks again.

showuon

LGTM! Thanks for the PR! I'll wait for another week to see if there are other people also want to have a review. Thanks.

showuon · 2022-03-05T08:11:39Z

@RivenSun2 , thanks for the contribution!

ijuma · 2022-03-05T16:48:17Z

        log.info(b.toString());
    }

+    public void logUnusedAndUnknown() {


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

Oh... Thanks for the reminder. @RivenSun2 , could you summit a small KIP for this change? Thanks.

Did we consider the fact that we don't know the config names for pluggable components?

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.

RivenSun2 · 2022-03-08T01:09:23Z

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.

showuon · 2022-03-08T01:43:13Z

@RivenSun2 , thanks for the help.

Will the new PR title start with MINOR or still start with 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. Let's discuss it in KIP discussion. Thanks.

Thank you.

RivenSun2 and others added 27 commits September 19, 2021 17:59

fix issue : KafkaConsumer cannot jump out of the poll method, and the…

4169106

… cpu and traffic on the broker side increase sharply JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

fix this issue for AUTO_PATTERN and AUTO_TOPICS in cleanUpConsumedOff…

cd84dbc

…sets method JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

1. fix NPE issue

39ee67a

2. Optimize the import of package Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit changeCodes by showuon review

9aa7bd7

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

fix package auto-import issue by IDEA Editor

5e09d03

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit reviewed codeChange

8b1382a

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit reviewed codeChange

7346e88

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit test code

0a77af1

add test Method "testForceMetadataDeleteForPatternSubscriptionDuringRebalance()" Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

fix merge conflicts

9e544b2

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Merge branch 'apache:trunk' into trunk

3334bc5

commit code changes for review

a5f6f81

Author: RivenSun2 riven.sun@zoom.us

commit code changes for showuon's review

12359c2

commit code changes for showuon's review

359af7d

commit code changes for guozhangwang's review

82b3585

fix issue for ConsumerBounceTest testClose case failed

a98bd87

fix issue for KafkaConsumerTest testCase failed

efef844

fix issue for testCloseManualAssignment case failed

12fb32c

LeaveGroupResponse should be prepared conditionally

996b8d9

fix issue for testInvalidGroupMetadata case

de1580c

Merge branch 'apache:trunk' into trunk

587a2bd

Merge branch 'trunk' of https://github.com/apache/kafka into trunk

c5e7595

� Conflicts: � clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java

branch conflicts resolved

f78969c

in order to retrigger jenkins tests

e0ab2e5

Merge branch 'apache:trunk' into trunk

8b8c78f

Merge branch 'apache:trunk' into trunk

282692f

Merge branch 'apache:trunk' into trunk

e7c66e0

KAFKA-13689: Optimized the printing of AbstractConfig logs, and strip…

0c63859

…ped unknownKeys from unusedKeys

showuon reviewed Feb 24, 2022

View reviewed changes

KAFKA-13689: commit changes for showuon's review

b8a36e0

commit code changes for review

5e4cf50

showuon reviewed Feb 25, 2022

View reviewed changes

commit code changes

6031c3b

showuon reviewed Feb 25, 2022

View reviewed changes

commit code change for showuon's review

153323b

commit code for review

30cb734

showuon approved these changes Feb 26, 2022

View reviewed changes

showuon merged commit 0dac4b4 into apache:trunk Mar 5, 2022

ijuma reviewed Mar 5, 2022

View reviewed changes

showuon mentioned this pull request Mar 8, 2022

KAFKA-13689: Revert AbstractConfig code changes #11863

Merged

		assertEquals(1, config.unknown().size());
		assertEquals(3, config.unused().size());

Conversation

RivenSun2 commented Feb 24, 2022

Uh oh!

RivenSun2 commented Feb 24, 2022

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RivenSun2 Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RivenSun2 commented Feb 24, 2022

Uh oh!

RivenSun2 commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RivenSun2 commented Feb 25, 2022

Uh oh!

showuon commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RivenSun2 commented Feb 25, 2022

Uh oh!

RivenSun2 commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon commented Feb 25, 2022

Uh oh!

RivenSun2 commented Feb 25, 2022

Uh oh!

showuon commented Feb 26, 2022

RivenSun2 Feb 24, 2022 •

edited

Loading

RivenSun2 commented Feb 24, 2022 •

edited

Loading

showuon commented Feb 25, 2022 •

edited

Loading

RivenSun2 commented Feb 25, 2022 •

edited

Loading

RivenSun2 commented Feb 26, 2022 •

edited

Loading