KAFKA-9432 automated protocol for DescribeConfigs by tombentley · Pull Request #8312 · apache/kafka

tombentley · 2020-03-18T17:47:52Z

Change the DescribeConfigs request and response to use the generated protocol messages.

tombentley · 2020-03-18T17:49:01Z

@mimaison here's another one, grateful if you could review.

tombentley · 2020-04-09T12:38:54Z

Rebased for conflict.

tombentley · 2020-04-09T14:58:40Z

Rebased for conflict.

tombentley · 2020-04-28T09:04:49Z

Rebased for conflict.

tombentley · 2020-05-12T10:33:18Z

Rebased for conflict.

mimaison · 2020-05-22T11:00:31Z

retest this please

mimaison

@tombentley Thanks for the PR. I've made a first pass and left a bunch of comments

tombentley · 2020-05-27T14:38:41Z

@mimaison I've fixed those, if you want to make a 2nd pass. I guess I should add a test to KafkaAdminClientTest too.

mimaison · 2020-05-28T08:42:17Z

retest this please

mimaison · 2020-06-04T09:53:45Z

Let's get #8311 merged first and then rebase this one on top

tombentley · 2020-06-08T15:26:00Z

@mimaison I've rebased and added some more tests in KafkaAdminClientTest. In so doing I noticed that describeConfigs() would require two requests to each broker when both BROKER and BROKER_LOGGER config resources for a broker were requested. I've fixed that, but alterConfigs() and incrementalAlterConfigs() do the same thing. WDYT about changing them too?

tombentley · 2020-06-08T15:36:15Z

This PR will also fix KAFKA-6962

mimaison · 2020-06-08T20:58:10Z

retest this please

mimaison

Thanks for the updates. It looks good. I just left a few minor comments.

tombentley · 2020-06-11T18:43:47Z

@mimaison done.

ableegoldman · 2020-06-13T00:53:37Z

-    public DescribeConfigsResponse(int throttleTimeMs, Map<ConfigResource, Config> configs) {
-        this.throttleTimeMs = throttleTimeMs;
-        this.configs = Objects.requireNonNull(configs, "configs");


Hey @mimaison @tombentley, removing this constructor seems to have broken the build (it was used in TopicAdminTest). Can we get a hotfix for this to unblock other PRs?
edit: nevermind, a PR was just opened for it (pull/8866)

tombentley · 2020-06-13T08:13:19Z

@ableegoldman sorry about that. A fix has now been committed to trunk. Thanks to @chia7712.

* 'trunk' of github.com:apache/kafka: (42 commits) HOTFIX: Fix compile error in TopicAdminTest (apache#8866) KAFKA-10144: clean up corrupted standby tasks before attempting a commit (apache#8849) KAFKA-10157: Fix broken tests due to InterruptedException from FinalizedFeatureChangeListener (apache#8857) KAFKA-9432: automated protocol for DescribeConfigs (apache#8312) KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764) KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680) KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787) KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818) KAFKA-9374: Make connector interactions asynchronous (apache#8069) MINOR: reduce sizeInBytes for percentiles metrics (apache#8835) KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829) KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828) KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455) KAFKA-7833: Add missing test (apache#8847) KAFKA-9066: Retain metrics for failed tasks (apache#8502) KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453) KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663) KAFKA-9441: remove prepareClose() to simplify task management (apache#8833) KAFKA-7833: Add Global/StateStore name conflict check (apache#8825) KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630) ...

…)" This reverts commit 9a4f00f.

omkreddy · 2020-07-18T12:51:29Z

@tombentley After this change, adminClient.describeConnfigs() is not backward compatible with older kafka broker versions. It seems, older brokers are receiving empty configNames here. Older brokers does not handle empty configKeys. Due to this old brokers are returning empty config list. We may need to explicitly set setConfigurationKeys(null) here.

We also need to update ClientCompatibilityTest, to catch these kind of errors.
https://github.com/apache/kafka/blob/trunk/tools/src/main/java/org/apache/kafka/tools/ClientCompatibilityTest.java
https://github.com/apache/kafka/blob/trunk/tests/kafkatest/tests/client/client_compatibility_features_test.py

cc @mimaison

…` to make it backward compatible with older Kafka versions. - After #8312, older brokers are returning empty configs, with latest `adminClient.describeConfigs`. Old brokers are receiving empty configNames in `AdminManageer.describeConfigs()` method. Older brokers does not handle empty configKeys. Due to this old brokers are filtering all the configs. - Update ClientCompatibilityTest to verify describe configs - Add test case to test describe configs with empty configuration Keys Author: Manikumar Reddy <manikumar.reddy@gmail.com> Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com> Closes #9046 from omkreddy/KAFKA-9432

…` to make it backward compatible with older Kafka versions. - After apache#8312, older brokers are returning empty configs, with latest `adminClient.describeConfigs`. Old brokers are receiving empty configNames in `AdminManageer.describeConfigs()` method. Older brokers does not handle empty configKeys. Due to this old brokers are filtering all the configs. - Update ClientCompatibilityTest to verify describe configs - Add test case to test describe configs with empty configuration Keys Author: Manikumar Reddy <manikumar.reddy@gmail.com> Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com> Closes apache#9046 from omkreddy/KAFKA-9432

tombentley · 2020-07-28T07:37:09Z

@omkreddy sorry about that, I was away. I see that you fixed it in the meantime.

tombentley · 2020-07-28T08:10:05Z

@omkreddy, also thanks for the pointer to that compatibility test, I wasn't previously aware of it, but I'll use it now that I am.

tombentley mentioned this pull request Mar 20, 2020

KAFKA-9494: Include data type of the config in ConfigEntry #8034

Closed

3 tasks

tombentley force-pushed the KAFKA-9432-DescribeConfigs-automated-protocol branch from ed6a9ae to 0fce630 Compare March 20, 2020 15:59

tombentley force-pushed the KAFKA-9432-DescribeConfigs-automated-protocol branch from 0fce630 to 8f2c22f Compare April 9, 2020 12:38

tombentley force-pushed the KAFKA-9432-DescribeConfigs-automated-protocol branch from 8f2c22f to 4eb1f06 Compare April 9, 2020 14:47

tombentley force-pushed the KAFKA-9432-DescribeConfigs-automated-protocol branch from 4eb1f06 to c60c196 Compare April 28, 2020 09:04

tombentley force-pushed the KAFKA-9432-DescribeConfigs-automated-protocol branch from c60c196 to cb1f307 Compare May 12, 2020 10:33

mimaison requested changes May 22, 2020

View reviewed changes

mimaison mentioned this pull request Jun 3, 2020

KAFKA-9627: Replace ListOffset request/response with automated protocol #8295

Merged

3 tasks

tombentley added 6 commits June 8, 2020 11:42

KAFKA-9432 automated protocol for DescribeConfigs

141ca72

rebase

ca2e5e8

Review comments

2962bf7

Fix rebase

55783db

Better test

c89ec96

Use single request when getting BROKER & BROKER_LOGGER from same broker

b5b3006

tombentley force-pushed the KAFKA-9432-DescribeConfigs-automated-protocol branch from 10c21a1 to b5b3006 Compare June 8, 2020 15:20

mimaison requested changes Jun 11, 2020

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated

Review comments

5a9c778

mimaison approved these changes Jun 12, 2020

View reviewed changes

mimaison merged commit 9a4f00f into apache:trunk Jun 12, 2020

ableegoldman reviewed Jun 13, 2020

View reviewed changes

stevenpyzhang added a commit to confluentinc/kafka that referenced this pull request Jul 18, 2020

Revert "KAFKA-9432: automated protocol for DescribeConfigs (apache#8312…

a11f563

…)" This reverts commit 9a4f00f.

stevenpyzhang mentioned this pull request Jul 20, 2020

Cannot create stream/table specifying only partitions and not replicas confluentinc/ksql#5847

Closed

omkreddy mentioned this pull request Jul 20, 2020

KAFKA-9432:(follow-up) Set configKeys to null in describeConfigs() to make it backward compatible with older Kafka versions. #9046

Closed

3 tasks

Uh oh!

Conversation

tombentley commented Mar 18, 2020

Uh oh!

tombentley commented Mar 18, 2020

Uh oh!

tombentley commented Apr 9, 2020

Uh oh!

tombentley commented Apr 9, 2020

Uh oh!

tombentley commented Apr 28, 2020

Uh oh!

tombentley commented May 12, 2020

Uh oh!

mimaison commented May 22, 2020

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tombentley commented May 27, 2020

Uh oh!

mimaison commented May 28, 2020

Uh oh!

mimaison commented Jun 4, 2020

Uh oh!

tombentley commented Jun 8, 2020

Uh oh!

tombentley commented Jun 8, 2020

Uh oh!

mimaison commented Jun 8, 2020

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tombentley commented Jun 11, 2020

Uh oh!

ableegoldman Jun 13, 2020 • 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!

tombentley commented Jun 13, 2020

Uh oh!

omkreddy commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tombentley commented Jul 28, 2020

Uh oh!

tombentley commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ableegoldman Jun 13, 2020 •

edited

Loading

omkreddy commented Jul 18, 2020 •

edited

Loading