KAFKA-10090 Misleading warnings: The configuration was supplied but i… by chia7712 · Pull Request #8826 · apache/kafka

chia7712 · 2020-06-07T14:02:27Z

https://issues.apache.org/jira/browse/KAFKA-10090

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

abbccdda

One meta comment, in the ticket we stated that we want to fix the warning for used instead of unknown. Does it make sense to just change the logging? And in terms of misleading, does this log confuse the user by any chance?

junrao · 2020-11-04T21:35:18Z

@chia7712 : Thanks for the PR.

I ran the following command with the PR.
bin/kafka-topics.sh --bootstrap-server localhost:9092 --command-config kafka.properties --create --topic test1

kafka.properties

security.protocol=SSL
ssl.protocol=TLS

I still saw the WARN.

[2020-11-04 13:32:16,059] WARN The configuration 'ssl.protocol' was supplied but isn't a known config. (org.apache.kafka.clients.admin.AdminClientConfig)

chia7712 · 2020-11-05T04:07:28Z

@junrao Thanks for your response. fixed.

junrao · 2020-11-06T00:36:44Z

@chia7712 : Thanks for the updated PR. Did that fix the issue since I still saw the same WARN when running kafka-topics.sh.

chia7712 · 2020-11-06T03:07:09Z

Did that fix the issue since I still saw the same WARN when running kafka-topics.sh.

my bad :(

junrao

@chia7712 : Thanks for the updated PR. It does seem to fix the WARN in admin tool.

I tried running console-producer with/without this PR. It doesn't seem to WARN any unused SSL configs in either test. Do you know why?

junrao · 2020-11-10T01:21:34Z

Does this cover the case when listenerName is not null? I guess that can only happen on the server side and since we don't log unused configs on the server, so maybe this is ok for now?

if (listenerName == null) parsedConfigs = (Map<String, Object>) config.values(); else parsedConfigs = config.valuesWithPrefixOverride(listenerName.configPrefix());

the method config.valuesWithPrefixOverride also returns ```RecordingMap so it is ok.

chia7712 · 2020-11-10T05:24:57Z

I tried running console-producer with/without this PR. It doesn't seem to WARN any unused SSL configs in either test. Do you know why?

@junrao this is the root cause.

Hmm, why is this necessary since we reset used to empty in the next line?

ConfigDef#parse (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java#L468) tries to get all elements from input maps so all gets are recorded. In order to avoid recording, we pass a copy instead of RecordingMap.

Ok. I guess the issue is in the following, where we pass in a RecordingMap to construct ProducerConfig.

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L384

However, that code seems no longer necessary since we are now setting clientId in ProducerConfig.postProcessParsedConfig(). Could we just avoid constructing ProducerConfig there?

However, that code seems no longer necessary since we are now setting clientId in ProducerConfig.postProcessParsedConfig(). Could we just avoid constructing ProducerConfig there?

I don't think so. The configs passed to configurable object is origins so the generated "client.id" is not included. However, your feedback inspires me that we don't need to create a new ProducerConfig. Instead, we can use overrideConfig to set generated client.id to those configurable object. Will update it later.

Hmm, I am still a bit confused. My understanding is that with the latest change, ProducerConfig will only be instantiated once and thus the passed in originals will never be a RecordingMap. But it seems this is still needed? Could you explain a bit more why this is the case?

But it seems this is still needed?

It is not necessary with the latest change. I kept it as a total solution (if someone pass RecordingMap in the future). However, I'm going to remove it to make this PR simpler.

chia7712 · 2020-11-11T11:54:41Z

org.apache.kafka.streams.integration.StreamTableJoinTopologyOptimizationIntegrationTest.shouldDoStreamTableJoinWithDifferentNumberOfPartitions

unrelated error. rebase to trigger QA

junrao

@chia7712 : Thanks for the updated PR. A few more comments below.

junrao · 2020-11-18T22:19:33Z

Hmm, why is this necessary since we reset used to empty in the next line?

junrao · 2020-11-19T17:56:33Z

When will the input configs not be recording?

Currently, the use cases of non-RecordingMap happens on tests. However, it seems to me we don't give a good definition of Configurable#configure. It is hard to say what we should pass to it. immutable map, mutable map and RecordingMap are alternatives. I want to keep flexibility but it is ok to me to rewrite related tests to make sure all pass are RecordingMap

junrao · 2020-11-19T18:04:48Z

consumer is unused.

It tests the specify config is recorded when constructing KafkaConsumer

junrao · 2020-11-19T18:11:56Z

Should we use the private static constructor in this class? Ditto below.

junrao · 2020-11-19T18:13:29Z

producer is unused.

It tests the specify config is recorded when constructing KafkaProducer

junrao

@chia7712 : Thanks for the updated PR. One more comment below.

junrao · 2020-11-20T21:13:25Z

Ok. I guess the issue is in the following, where we pass in a RecordingMap to construct ProducerConfig.

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L384

However, that code seems no longer necessary since we are now setting clientId in ProducerConfig.postProcessParsedConfig(). Could we just avoid constructing ProducerConfig there?

junrao

@chia7712 : Thanks for the updated PR. A few more comments below.

junrao · 2020-12-01T00:48:34Z

Hmm, I am still a bit confused. My understanding is that with the latest change, ProducerConfig will only be instantiated once and thus the passed in originals will never be a RecordingMap. But it seems this is still needed? Could you explain a bit more why this is the case?

junrao · 2020-12-01T01:31:39Z

Do we need to instantiate again?

We need a new RecordingMap to test different key without listener prefix. Otherwise, the key may be used by previous test.

junrao · 2020-12-01T01:46:14Z

Is this test necessary? Do we still have a case where we pass in a RecordingMap to ProducerConfig?

I'm going to remove this test assertTrue(new ProducerConfig(config.originals(), false).unused().co...

junrao

@chia7712 : Thanks for the latest PR. LGTM. I triggered a run of system tests. Will share the results later. If there are no issues with the system tests, you can merged the PR.

junrao · 2020-12-02T04:32:57Z

@chia7712 : There are 6 failures in the system test run. Are they related to this PR?

http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2020-12-01--001.1606874236--chia7712--KAFKA-10090--4da062adc/report.html

chia7712 · 2020-12-02T06:42:18Z

There are 6 failures in the system test run. Are they related to this PR?

They are unrelated to this PR and I have opened a PR to fix them (#9673)

junrao

@chia7712 : Thanks for the investigation. LGTM

…sn't a known config

chia7712 · 2020-12-02T15:49:33Z

rebase to trigger QA again

…t-for-generated-requests * apache-github/trunk: MINOR: Fix flaky test shouldQueryOnlyActivePartitionStoresByDefault (apache#9681) KAFKA-10799 AlterIsr utilizes ReplicaManager ISR metrics (apache#9677) MINOR: Fix KTable-KTable foreign-key join example (apache#9683) KAFKA-10473: Add docs on partition size-on-disk, and other log-related metrics (apache#9276) KAFKA-10739; Replace EpochEndOffset with automated protocol (apache#9630) KAFKA-10460: ReplicaListValidator format checking is incomplete (apache#9326) KAFKA-10554; Perform follower truncation based on diverging epochs in Fetch response (apache#9382) MINOR: Align the UID inside/outside container (apache#9652) KAFKA-10794 Replica leader election is too slow in the case of too many partitions (apache#9675) KAFKA-10090 Misleading warnings: The configuration was supplied but i… (apache#8826) clients/src/main/java/org/apache/kafka/common/requests/OffsetsForLeaderEpochResponse.java clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java core/src/test/scala/unit/kafka/server/epoch/util/ReplicaFetcherMockBlockingSend.scala

abbccdda reviewed Jun 27, 2020

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/network/ChannelBuilders.java Outdated

chia7712 force-pushed the KAFKA-10090 branch from f1c6811 to c09467a Compare June 27, 2020 17:02

chia7712 force-pushed the KAFKA-10090 branch from c09467a to a2a4a39 Compare November 5, 2020 04:07

chia7712 force-pushed the KAFKA-10090 branch from a2a4a39 to b510a38 Compare November 6, 2020 03:06

junrao reviewed Nov 10, 2020

View reviewed changes

chia7712 force-pushed the KAFKA-10090 branch from b510a38 to cb0aa27 Compare November 10, 2020 05:24

chia7712 commented Nov 10, 2020

View reviewed changes

chia7712 force-pushed the KAFKA-10090 branch from cb0aa27 to 2f5d4e3 Compare November 11, 2020 11:55

junrao reviewed Nov 19, 2020

View reviewed changes

chia7712 force-pushed the KAFKA-10090 branch from 2f5d4e3 to 648fdc7 Compare November 20, 2020 07:27

junrao reviewed Nov 21, 2020

View reviewed changes

chia7712 force-pushed the KAFKA-10090 branch from 648fdc7 to 2415a0d Compare November 21, 2020 09:27

junrao reviewed Dec 1, 2020

View reviewed changes

chia7712 force-pushed the KAFKA-10090 branch from 2415a0d to 4da062a Compare December 1, 2020 06:10

junrao reviewed Dec 1, 2020

View reviewed changes

junrao approved these changes Dec 2, 2020

View reviewed changes

chia7712 added 7 commits December 2, 2020 23:49

KAFKA-10090 Misleading warnings: The configuration was supplied but i…

b994ee1

…sn't a known config

fix SslFactory configs

7298cfa

channelBuilderConfigs should return RecordedMap

df37807

address review comments; fix another bug

93465cc

refactor KafkaProducerTest

29e36ae

apply generated client id to all configurable objects

a2b52fd

remove unnecessary tests and changes

bee0513

chia7712 force-pushed the KAFKA-10090 branch from 4da062a to bee0513 Compare December 2, 2020 15:49

chia7712 merged commit e63f591 into apache:trunk Dec 3, 2020

chia7712 deleted the KAFKA-10090 branch March 25, 2024 15:22

Conversation

chia7712 commented Jun 7, 2020

Committer Checklist (excluded from commit message)

Uh oh!

abbccdda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

junrao commented Nov 4, 2020

Uh oh!

chia7712 commented Nov 5, 2020

Uh oh!

junrao commented Nov 6, 2020

Uh oh!

chia7712 commented Nov 6, 2020

Uh oh!

junrao 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!

Uh oh!

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!

chia7712 commented Nov 11, 2020

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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