Switch from ZK to Kafka Admin Client #1569

tomncooper · 2021-05-19T16:35:40Z

This PR partially resolves #1415.

To keep code review easier I am proposing to make changes in stages, starting with the KafkaTopicConfigProvider and then moving on to the other classes which use AdminZkClient.

tomncooper · 2021-05-19T16:47:13Z

@Lincong @efeg I have started the work on removing the ZkAdminClient, currently (to keep the size of the code review down) this just deals with the KafkaTopicConfigProvider. If you are happy with the general approach I can either add to this PR or open separate ones?

tomncooper · 2021-05-19T16:50:34Z

This converts the default KafkaTopicConfigProvider to use the Kafka Admin Client and creates a new TopicConfigProvider implementation that uses the ZkAdminClient so a user could switch back to the old Provider logic if they need to?

efeg · 2021-05-19T19:05:41Z

@tomncooper Thanks for the PR! I will try to review it within this week.

efeg

@tomncooper Thanks for the PR!
Made a first pass and left a few comments.

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/KafkaCruiseControlUtils.java

...ontrol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaZkTopicConfigProvider.java

...-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaTopicConfigProvider.java

...rol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaAdminTopicConfigProvider.java

config/cruisecontrol.properties

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/constants/MonitorConfig.java

docs/wiki/User Guide/Configurations.md

...rol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaAdminTopicConfigProvider.java

...-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaTopicConfigProvider.java

efeg

Thanks for the changes! Left more comments.

efeg

Thanks for the update!
Left a few initial comments, will take a closer look at KafkaAdminTopicConfigProvider.java later.

...-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaTopicConfigProvider.java

...rol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaAdminTopicConfigProvider.java

...-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaTopicConfigProvider.java

efeg

Thanks for the updates! Left comments.

...rol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaAdminTopicConfigProvider.java

efeg · 2021-06-09T01:38:23Z

@tomncooper the build seems to have failed due to style issues:

> Task :cruise-control:checkstyleMain
[ant:checkstyle] [ERROR] /home/circleci/workspace/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/JsonFileTopicConfigProvider.java:52: Multiple empty lines after this line [RegexpMultiline]
[ant:checkstyle] [ERROR] /home/circleci/workspace/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/TopicConfigProvider.java:12:1: Extra separation in import group before 'org.apache.kafka.common.annotation.InterfaceStability' [CustomImportOrder]

tomncooper · 2021-06-10T08:59:07Z

@tomncooper the build seems to have failed due to style issues:

Sorry about that. I keep hitting checkstyle issues (I need to tweak my IDE). I have made a pre-commit hook to run checkstyle before each commit. I submitted a PR (#1584) in case any other devs might find that useful.

efeg

Left a few nits. Once they are addressed, the PR looks good to me.

...rol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaAdminTopicConfigProvider.java

...-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaTopicConfigProvider.java

efeg · 2021-06-29T17:56:24Z

Hi @tomncooper -- do you think we can address the remaining items this week to merge this PR?

tomncooper · 2021-06-29T18:01:21Z

Hi @efeg, yes, I got sidetracked with some other things. I will try and address these this week.

tomncooper · 2021-07-03T08:58:22Z

Hi @efeg, I think I addressed all your comments. Thanks.

Signed-off-by: Thomas Cooper <code@tomcooper.dev>

* Changed ZK based provider to use old class name and gave new Kafka Admin provider new name * Added deprecated annotation to old ZK provider and additional javadoc explaining the deprecation. * Switched to using the configured admin client timeout from the main config class * Fixed Copy Right date

… parent class Signed-off-by: Thomas Cooper <code@tomcooper.dev>

…nstance from LoadMonitor Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Signed-off-by: Thomas Cooper <code@tomcooper.dev>

…n the KafkaAdminTopicConfigProvider Signed-off-by: Thomas Cooper <code@tomcooper.dev>

…common config key to abstract parent class Signed-off-by: Thomas Cooper <code@tomcooper.dev>

…afka Admin Topic Config Provider Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Signed-off-by: Thomas Cooper <code@tomcooper.dev>

efeg

Thanks for the updates! LGTM.

…provider class (linkedin#1569)

efeg self-requested a review May 19, 2021 19:05

efeg linked an issue May 19, 2021 that may be closed by this pull request

Replace AdminZkClient with AdminClient #1415

Closed

efeg reviewed May 22, 2021

View reviewed changes

tomncooper force-pushed the zk-admin-removal branch 2 times, most recently from deb24a2 to 80fe8b6 Compare May 24, 2021 17:01

tomncooper requested a review from efeg May 24, 2021 17:01

efeg reviewed May 25, 2021

View reviewed changes

efeg requested changes May 25, 2021

View reviewed changes

tomncooper force-pushed the zk-admin-removal branch 4 times, most recently from f6319f5 to 95e8ef1 Compare June 3, 2021 18:01

tomncooper requested a review from efeg June 3, 2021 18:41

tomncooper force-pushed the zk-admin-removal branch from cfa4dcb to 858b2a9 Compare June 3, 2021 18:42

tomncooper marked this pull request as ready for review June 3, 2021 18:43

efeg reviewed Jun 4, 2021

View reviewed changes

efeg reviewed Jun 5, 2021

View reviewed changes

...rol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaAdminTopicConfigProvider.java Outdated Show resolved Hide resolved

...rol/src/main/java/com/linkedin/kafka/cruisecontrol/config/KafkaAdminTopicConfigProvider.java Outdated Show resolved Hide resolved

tomncooper force-pushed the zk-admin-removal branch 2 times, most recently from 83ecc95 to ac760f7 Compare June 7, 2021 17:48

tomncooper force-pushed the zk-admin-removal branch from ac760f7 to 7463236 Compare June 10, 2021 08:57

tomncooper requested a review from efeg June 11, 2021 08:43

efeg reviewed Jun 14, 2021

View reviewed changes

tomncooper added 10 commits July 3, 2021 10:05

Created TopicConfigProvider using Kafka Admin Client

64fa381

Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Moved JSON config loading method for TopicConfigProviders to abstract…

fde2168

… parent class Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Switched KafkaAdminTopicConfigProvider to use provided Admin Client i…

d7d093b

…nstance from LoadMonitor Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Reduced ERROR level logging in KafkaAdminTopicConfigProvider

0bcc1f3

Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Switch from using .all() to .values() for alltopics config fetching i…

4b26a60

…n the KafkaAdminTopicConfigProvider Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Added deprecated javadoc to ZK based topic config provider and moved …

9705e01

…common config key to abstract parent class Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Simplified the future processing in the all topic configs method of K…

bf009b3

…afka Admin Topic Config Provider Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Added new method to TopicConfigProvider and refactored implementations

4e45b49

Signed-off-by: Thomas Cooper <code@tomcooper.dev>

Addressed reviewer comments

4cbfd8f

Signed-off-by: Thomas Cooper <code@tomcooper.dev>

tomncooper force-pushed the zk-admin-removal branch from 84ed9c9 to 4cbfd8f Compare July 3, 2021 09:10

tomncooper requested a review from efeg July 6, 2021 08:33

efeg approved these changes Jul 15, 2021

View reviewed changes

efeg merged commit 30ecf7b into linkedin:migrate_to_kafka_2_5 Jul 15, 2021

efeg pushed a commit to efeg/cruise-control that referenced this pull request Jul 15, 2021

Add support to switch from ZK to Kafka Admin Client for topic config …

ea9e62c

…provider class (linkedin#1569)

efeg pushed a commit to efeg/cruise-control that referenced this pull request Jul 15, 2021

Add support to switch from ZK to Kafka Admin Client for topic config …

17cd6f1

…provider class (linkedin#1569)

efeg mentioned this pull request Jul 20, 2021

Enable users to monitor the min.insync.replicas of all topics #1622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from ZK to Kafka Admin Client #1569

Switch from ZK to Kafka Admin Client #1569

tomncooper commented May 19, 2021 •

edited

Loading

tomncooper commented May 19, 2021 •

edited

Loading

tomncooper commented May 19, 2021

efeg commented May 19, 2021

efeg left a comment

efeg left a comment

efeg left a comment

efeg left a comment

efeg commented Jun 9, 2021

tomncooper commented Jun 10, 2021 •

edited

Loading

efeg left a comment

efeg commented Jun 29, 2021

tomncooper commented Jun 29, 2021

tomncooper commented Jul 3, 2021

efeg left a comment

Switch from ZK to Kafka Admin Client #1569

Switch from ZK to Kafka Admin Client #1569

Conversation

tomncooper commented May 19, 2021 • edited Loading

tomncooper commented May 19, 2021 • edited Loading

tomncooper commented May 19, 2021

efeg commented May 19, 2021

efeg left a comment

Choose a reason for hiding this comment

efeg left a comment

Choose a reason for hiding this comment

efeg left a comment

Choose a reason for hiding this comment

efeg left a comment

Choose a reason for hiding this comment

efeg commented Jun 9, 2021

tomncooper commented Jun 10, 2021 • edited Loading

efeg left a comment

Choose a reason for hiding this comment

efeg commented Jun 29, 2021

tomncooper commented Jun 29, 2021

tomncooper commented Jul 3, 2021

efeg left a comment

Choose a reason for hiding this comment

tomncooper commented May 19, 2021 •

edited

Loading

tomncooper commented May 19, 2021 •

edited

Loading

tomncooper commented Jun 10, 2021 •

edited

Loading