KAFKA-18530 Remove ZooKeeperInternals by m1a2st · Pull Request #18641 · apache/kafka

m1a2st · 2025-01-20T13:34:49Z

Jira: https://issues.apache.org/jira/browse/KAFKA-18530

Since Kafka 4.0 has deprecated ZooKeeper, we should also remove the ZooKeeperInternals class. However, we need to maintain backward compatibility for metric names.

Committer Checklist (excluded from commit message)

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

ijuma · 2025-01-20T15:21:30Z

 package org.apache.kafka.server.config;

-public class ZooKeeperInternals {
+public class KraftInternals {


I don't think this name makes sense - there's nothing specific to kraft in this code. Can we move this constant to one of the quota classes and have the others reference it from there?

Removing this would affect the default username and default client-id. Since this is a breaking change, I suggest we maintain consistency by keeping these default values. WDYT?

@m1a2st Could you please clean up the usage of ZooKeeperInternals.DEFAULT_STRING first? We are currently using ZooKeeperInternals.DEFAULT_STRING as a specific string for compatibility with the zk handler (QuotaConfigHandler). Since #18617 will remove QuotaConfigHandler, the kraft ClientQuotaMetadataManager no longer needs to use ZooKeeperInternals.DEFAULT_STRING.

After addressing above comment, we can move <default> to be a internal constant of ClientQuotaManager to ensure the compatibility of metrics kafka.server:type={Produce|Fetch},user=([-.\w]+),client-id=([-.\w]+)

chia7712 · 2025-01-24T10:31:12Z

    val (sanitizedUser, sanitizedClientId) = quotaEntity match {
      case UserEntity(user) => (Some(Sanitizer.sanitize(user)), None)
-      case DefaultUserEntity => (Some(ZooKeeperInternals.DEFAULT_STRING), None)
+      case DefaultUserEntity => (Some(ClientQuotaManager.DefaultString), None)


We should use Option[BaseUserEntity] instead of a string. As mentioned previously (#18641 (comment)), there's no need for a specific string to represent the "default" value.

chia7712 · 2025-01-24T10:34:13Z

-    if (brokerId == ZooKeeperInternals.DEFAULT_STRING)
-      brokerConfig.dynamicConfig.updateDefaultConfig(properties)
-    else if (brokerConfig.brokerId == brokerId.trim.toInt) {
+    if (brokerConfig.brokerId == brokerId.trim.toInt) {


We must handle the "default" configs when the broker id is "empty"

chia7712 · 2025-01-30T12:43:15Z

@m1a2st could you please rebase code to include the recent flaky fixes?

chia7712 · 2025-01-31T13:08:45Z

+  ): Option[ClientQuotaEntity.ConfigEntity] = {
+    if (sanitizedClientId.isEmpty)
+      None  
+    else if (sanitizedClientId.get.name() == DefaultString)


in kraft, we don't use <default> so this check is weird. If this is used to fix test, then maybe we should revise the test

chia7712 · 2025-01-31T13:27:22Z

+    else if (sanitizedClientId.get.name() == DefaultString)
+      Some(DefaultClientIdEntity)
+    else {
+      val clientId = sanitizedClientId.map(s => Sanitizer.desanitize(s.name()))


we should handle the ClientIdEntity only.

chia7712

@m1a2st thanks for this patch

chia7712 · 2025-02-03T03:39:09Z

 import scala.jdk.CollectionConverters._

 /**
-  * Dynamic broker configurations are stored in ZooKeeper and may be defined at two levels:


in kraft mode, the two levels are still existent. could you please revise it instead of removing it?

Heya @m1a2st! I have a follow-up ask, while the two levels are still present they will no longer be under the path specified in this file since the path is ZK-specific. As long as we don't use this comment to generate documentation I am okay with this being removed in a subsequent PR. However, if we use this file to generate documentation could you change this as part of this PR?

I think those comments in DynamicBrokerConfig are not used to generate documentation, but it will be better to correct it - as least, the description about zk path must be removed.

Okay, then I will review once they have been changed 😊!

Thanks for @clolov and @chia7712, update this document. If there is any idea, please let me know

m1a2st · 2025-02-03T12:01:40Z

Thanks for @chia7712 review, addressed all comments.

chia7712 · 2025-02-05T13:37:44Z

+      case DefaultUserEntity =>
+        (Some(ClientQuotaManager.DefaultUserEntity), None)
+      case ClientIdEntity(clientId) =>
+        (None, Some(ClientQuotaManager.ClientIdEntity(Sanitizer.sanitize(clientId))))


Do we need to sanitize data in kraft mode? the zk path is gone so we don't need to align the data for zk anymore.

In sanitize method, It deal with * and + symbols. Therefore, I think it is not only handling matters related to Zookeeper.

After under discussion, We should remove this sanitize, it is unused in this path.

chia7712 · 2025-02-06T06:03:50Z

-      val clientIdEntity = sanitizedClientId.map {
-        case ZooKeeperInternals.DEFAULT_STRING => DefaultClientIdEntity
-        case _ => ClientIdEntity(clientId.getOrElse(throw new IllegalStateException("Client-id not provided")))
+      val clientIdEntity = clientEntity match {


we don't need this conversion, right?

chia7712 · 2025-02-06T09:49:15Z

@m1a2st please file a PR for 4.0 as there are some conflicts

Since zk has been removed in 4.0, config handlers no longer need to handle the "<default>" value. This PR streamlines the config update process by eliminating the unnecessary string checks for "<default>" Reviewers: Christo Lolov <lolovc@amazon.com>, Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

chia7712 · 2025-02-06T11:00:12Z

please file a PR for 4.0 as there are some conflicts

I backport some cleanup to eliminate the conflicts

Since zk has been removed in 4.0, config handlers no longer need to handle the "<default>" value. This PR streamlines the config update process by eliminating the unnecessary string checks for "<default>" Reviewers: Christo Lolov <lolovc@amazon.com>, Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

github-actions Bot added triage PRs from the community core Kafka Broker small Small PRs labels Jan 20, 2025

m1a2st changed the title ~~KAFKA 18530 Rename ZooKeeperInternals~~ KAFKA-18530 Rename ZooKeeperInternals Jan 20, 2025

ijuma reviewed Jan 20, 2025

View reviewed changes

github-actions Bot removed triage PRs from the community small Small PRs labels Jan 21, 2025

chia7712 added the ci-approved label Jan 21, 2025

chia7712 reviewed Jan 24, 2025

View reviewed changes

m1a2st force-pushed the KAFKA-18530 branch from 57ab359 to 18d7801 Compare January 25, 2025 11:41

m1a2st added 2 commits January 31, 2025 01:24

remove dynamic Config related

b7bbdff

fix some test, and revert some wrong approach

a022b0c

m1a2st force-pushed the KAFKA-18530 branch from cf4b307 to a022b0c Compare January 30, 2025 18:23

m1a2st changed the title ~~KAFKA-18530 Rename ZooKeeperInternals~~ [WIP]KAFKA-18530 Rename ZooKeeperInternals Jan 30, 2025

m1a2st added 3 commits January 31, 2025 10:29

update the function signature

6cef55f

update the method

72fc80b

add new test for new method

8065ee7

m1a2st changed the title ~~[WIP]KAFKA-18530 Rename ZooKeeperInternals~~ KAFKA-18530 Rename ZooKeeperInternals Jan 31, 2025

m1a2st added 5 commits January 31, 2025 13:54

wip

ff19654

wip

821acd0

fix the error tests

2d16565

update the comment

fe8b6ca

update the comment

b81ae52

chia7712 reviewed Jan 31, 2025

View reviewed changes

m1a2st added 4 commits February 1, 2025 11:54

update the userEntity logic

4ca7f31

update the clientIdEntity logic

a6a52e0

update the clientIdEntity logic

44274d4

make string private

e9b1e5b

fix some detail

1927166

m1a2st changed the title ~~KAFKA-18530 Rename ZooKeeperInternals~~ KAFKA-18530 Remove ZooKeeperInternals Feb 2, 2025

chia7712 reviewed Feb 3, 2025

View reviewed changes

m1a2st added 3 commits February 3, 2025 19:44

addressed by comment

b52aaed

Merge branch 'trunk' into KAFKA-18530

b4b727d

remove unused import

a201ae9

m1a2st added 2 commits February 4, 2025 20:24

Merge branch 'trunk' into KAFKA-18530

dd871e2

update the document

13d9b7f

chia7712 reviewed Feb 5, 2025

View reviewed changes

addressed by comments

ed6353b

chia7712 reviewed Feb 6, 2025

View reviewed changes

update the test

d098405

chia7712 approved these changes Feb 6, 2025

View reviewed changes

chia7712 merged commit a3d9d88 into apache:trunk Feb 6, 2025

Conversation

m1a2st commented Jan 20, 2025

Committer Checklist (excluded from commit message)

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 Jan 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clolov Feb 3, 2025 • 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!

Uh oh!

Uh oh!

m1a2st commented Feb 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st Feb 5, 2025 • 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!

chia7712 commented Feb 6, 2025

Uh oh!

chia7712 commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clolov Feb 3, 2025 •

edited

Loading

m1a2st Feb 5, 2025 •

edited

Loading