KAFKA-9046: Use top-level worker configs for connector admin clients by C0urante · Pull Request #7525 · apache/kafka

C0urante · 2019-10-15T23:39:48Z

The changes here are meant to find a healthy compromise between the pre- and post-KIP-458 functionality of Connect workers when configuring admin clients for use with DLQs. Before KIP-458, admin clients were configured using the top-level worker configs; after KIP-458, they are configured using worker configs with a prefix of admin. and then optionally overridden by connector configs with a prefix of admin.override.. The behavior proposed here is to use, in ascending order of precedence, the top-level worker configs, worker configs prefixed with admin., and connector configs prefixed with admin.override.; essentially, use the pre-KIP-458 behavior by default but allow it to be overridden by the post-KIP-458 behavior.

Committer Checklist (excluded from commit message)

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

ncliang · 2019-10-22T21:28:44Z

        connConfig.put("metadata.max.age.ms", "10000");

        Map<String, String> expectedConfigs = new HashMap<>();
+        expectedConfigs.putAll(props);


The admin configs after overrides will contain both "admin.metadata.max.age.ms", and "metadata.max.age.ms", with different values. I feel like that might be confusing to debug. Could we include just the configs that don't start with "admin." prefix so they would not be overridden?

Part of this test is to ensure that overrides happen as expected; leaving out to-be-overridden configs would leave that case uncovered. If anything, it might make sense to expand the tests here to make sure that three distinct sources of configs for the admin client are recognized and handled with the expected precedence. In ascending order, that would be: from the worker config with no prefix, from the worker config with a (stripped) prefix of admin., and from the connector config with a (stripped) prefix of admin.override..

Or are you suggesting changes in the actual Worker class as opposed to this test class?

Oh, yeah, the comment was not about the test, just happen to realize this when looking at the testcase. Could we have something like originalsWithoutPrefix on AbstractConfig and use it to pre-populate adminProps with just original configs not containing override ones? Or even just calling originalsWithPrefix without stripping the prefix and removing those keys from adminProps

Yeah, that should be fine. Probably not worth it to alter the AbstractConfig since that's public API but engaging in some effort to make sure that both admin.foo and foo aren't present in the props given to the admin client seems like a good idea.

C0urante · 2019-10-23T20:17:05Z

Thanks, @ncliang. I've incorporated your suggestion of not including admin.-prefixed properties twice (both with and without the prefix) in admin client configs; LMK what you think

ncliang

LGTM!

ncliang · 2019-10-23T21:42:49Z

        Map<String, String> expectedConfigs = new HashMap<>();
+        expectedConfigs.putAll(props);
+        expectedConfigs.remove("admin.client.id");
+        expectedConfigs.remove("admin.metadata.max.age.ms");


nit: these 2 are added by the testcase just a few lines above. You can construct expectedConfigs from workerProps directly.

kkonstantine

LGTM overall, this is an important fix.
I agree with the recommendation to exclude producer. consumer. prefixed properties and of course give always precedence to admin. prefixed properties.

C0urante · 2019-11-13T19:53:28Z

Thanks for the discussion, @rhauch and @kkonstantine. Adding a check to prevent producer and consumer properties from being passed to the admin client seems safe enough, so I've added one in my latest commit. LMK what you think.

rhauch · 2019-11-13T20:34:56Z

Just to be thoroughly transparent, in AK 2.2 the Worker class passed the WorkerConfig and the producer.* properties to DeadLetterQueueReporter.createAndSetup(...) method, which then created an admin client with the WorkerConfig properties and created a producer with the supplied producer.* properties in the worker config.

In AK 2.3 this changed to compute the admin properties just from the admin.* properties and any connector-specific overrides and pass them to the DeadLetterQueueReporter.createAndSetup(...) method, which then created an admin client with the supplied admin.* properties and created a producer with the supplied producer.* properties in the worker config.

This change reverts the behavior so that the worker config properties (minus any producer. or consumer. properties), overwritten with any admin.* properties and any connector-specific admin property overrides, are passed as the properties for the admin client to the DeadLetterQueueReporter.createAndSetup(...) method. This restores the AK 2.2 behavior yet is still compatible with the AK 2.3 behavior.

After merging, this should be backported to the 2.3 branch.

rhauch

LGTM. Thanks for identifying this problem and providing the fix, @C0urante!

Waiting for a green build before merging.

C0urante · 2019-11-14T00:30:03Z

@rhauch the only failure on the JDK8/Scala 2.11 branch appears to be unrelated:

kafka.api.SslAdminClientIntegrationTest > testCreatePartitions FAILED
     java.lang.AssertionError: validateOnly expected:<3> but was:<1>

[Jira](https://issues.apache.org/jira/browse/KAFKA-9046) The changes here are meant to find a healthy compromise between the pre- and post-KIP-458 functionality of Connect workers when configuring admin clients for use with DLQs. Before KIP-458, admin clients were configured using the top-level worker configs; after KIP-458, they are configured using worker configs with a prefix of `admin.` and then optionally overridden by connector configs with a prefix of `admin.override.`. The behavior proposed here is to use, in ascending order of precedence, the top-level worker configs, worker configs prefixed with `admin.`, and connector configs prefixed with `admin.override.`; essentially, use the pre-KIP-458 behavior by default but allow it to be overridden by the post-KIP-458 behavior. Author: Chris Egerton <chrise@confluent.io> Reviewers: Konstantine Karantasis <konstantine@confluent.io>, Randall Hauch <rhauch@gmail.com>, Nigel Liang <nigel@nigelliang.com> Closes #7525 from C0urante/kafka-9046 (cherry picked from commit 38d243b) Signed-off-by: Manikumar Reddy <manikumar@confluent.io>

omkreddy · 2019-11-14T08:41:13Z

Merged the PR to trunk, 2.4, 2.3 branches

C0urante · 2019-11-14T19:08:44Z

Thanks, @omkreddy!

KAFKA-9046: Use top-level worker configs for connector admin clients

5f31ce2

ncliang reviewed Oct 22, 2019

View reviewed changes

KAFKA-9046: Address review comments

575f9e0

ncliang approved these changes Oct 23, 2019

View reviewed changes

KAFKA-9046: Address review comments

6ef7a4b

rhauch reviewed Nov 13, 2019

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

kkonstantine approved these changes Nov 13, 2019

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java

KAFKA-9046: Address review comments

ae232d2

rhauch approved these changes Nov 13, 2019

View reviewed changes

omkreddy closed this in 38d243b Nov 14, 2019

C0urante deleted the kafka-9046 branch November 14, 2019 19:08

kkonstantine added the connect label Oct 16, 2020

Conversation

C0urante commented Oct 15, 2019

Committer Checklist (excluded from commit message)

Uh oh!

ncliang Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

C0urante Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

C0urante Oct 23, 2019 • 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!

ncliang Oct 23, 2019 • 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!

C0urante Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

C0urante commented Oct 23, 2019

Uh oh!

ncliang left a comment

Choose a reason for hiding this comment

Uh oh!

ncliang Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kkonstantine left a comment • 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!

Uh oh!

C0urante commented Nov 13, 2019

Uh oh!

rhauch commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhauch left a comment • 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!

C0urante commented Nov 14, 2019

Uh oh!

omkreddy commented Nov 14, 2019

Uh oh!

C0urante commented Nov 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

C0urante Oct 23, 2019 •

edited

Loading

ncliang Oct 23, 2019 •

edited

Loading

kkonstantine left a comment •

edited

Loading

rhauch commented Nov 13, 2019 •

edited

Loading

rhauch left a comment •

edited

Loading