KAFKA-18533: Remove KafkaConfig zookeeper related logic by m1a2st · Pull Request #18547 · apache/kafka

m1a2st · 2025-01-15T12:08:22Z

as title

Committer Checklist (excluded from commit message)

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

m1a2st · 2025-01-15T12:12:20Z

requiresZookeeper should wait for #18483 and #18508

# Conflicts: # core/src/main/scala/kafka/server/KafkaConfig.scala

# Conflicts: # core/src/main/scala/kafka/server/DynamicBrokerConfig.scala # core/src/main/scala/kafka/server/KafkaConfig.scala # core/src/main/scala/kafka/server/MetadataSupport.scala # core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala

# Conflicts: # core/src/main/scala/kafka/controller/KafkaController.scala

ijuma · 2025-01-18T13:48:43Z

This PR seems to be adding an empty Kafka controller file.

ijuma · 2025-01-22T02:44:07Z

-  val zkConnectionTimeoutMs: Int =
-    Option(getInt(ZkConfigs.ZK_CONNECTION_TIMEOUT_MS_CONFIG)).map(_.toInt).getOrElse(getInt(ZkConfigs.ZK_SESSION_TIMEOUT_MS_CONFIG))
-  val zkEnableSecureAcls: Boolean = getBoolean(ZkConfigs.ZK_ENABLE_SECURE_ACLS_CONFIG)
-  val zkMaxInFlightRequests: Int = getInt(ZkConfigs.ZK_MAX_IN_FLIGHT_REQUESTS_CONFIG)


Can we also remove them from ZkConfigs?

ijuma

Thanks for the PR, left a few comments.

ijuma · 2025-01-22T14:46:04Z

-      contextLabels.put(BrokerIdLabel, config.brokerId.toString)
-    }
-
+    contextLabels.put(BrokerIdLabel, config.brokerId.toString)


Hmm, this seems like a bug - we may want to add a unit test if nothing failed. We should be setting NodeIdLabel, not BrokerIdLabel. Right?

In ServerTest.scala will test this bug, I think we won't need to add a new test for it.

ijuma · 2025-01-22T14:46:17Z

    val listenerNames = listeners.map(_.listenerName).toSet
    if (processRoles.isEmpty || processRoles.contains(ProcessRole.BrokerRole)) {
-      // validations for all broker setups (i.e. ZooKeeper and KRaft broker-only and KRaft co-located)
+      // validations for all broker setups (i.e. KRaft broker-only and KRaft co-located)


Shall we remove KRaft?

ijuma · 2025-01-22T14:47:41Z

    ))
    alterConfigs.put(topicResource2, util.Arrays.asList(new AlterConfigOp(new ConfigEntry(TopicConfig.COMPRESSION_TYPE_CONFIG, "snappy"), OpType.SET)))
-    alterConfigs.put(brokerResource, util.Arrays.asList(new AlterConfigOp(new ConfigEntry(ZkConfigs.ZK_CONNECT_CONFIG, "localhost:2181"), OpType.SET)))
+    alterConfigs.put(brokerResource, util.Arrays.asList(new AlterConfigOp(new ConfigEntry(KRaftConfigs.NODE_ID_CONFIG, "123"), OpType.SET)))


Hmm, it would be weird to update the node id, can we pick a config that makes more sense to update? Same for other cases similar to this one.

chia7712

@m1a2st thanks for this cleanup. please take a look at one small comment

chia7712 · 2025-01-23T16:07:03Z

    }

    public static final ConfigDef CONFIG_DEF =  new ConfigDef()
-            .define(ZK_CONNECT_CONFIG, STRING, null, HIGH, ZK_CONNECT_DOC)


Could you please add those configs to zk2kraft.html?

I think these config already on zk2kraft.html

chia7712 · 2025-01-23T16:11:21Z

 import static org.apache.kafka.common.config.ConfigDef.Type.PASSWORD;
 import static org.apache.kafka.common.config.ConfigDef.Type.STRING;

 public final class ZkConfigs {


open https://issues.apache.org/jira/browse/KAFKA-18631 to remove this config file

chia7712

LGTM and one small comment remains

chia7712 · 2025-01-24T11:30:04Z

-    if (config.usesSelfManagedQuorum) {
-      contextLabels.put(NodeIdLabel, config.nodeId.toString)
-    } else {
-      contextLabels.put(BrokerIdLabel, config.brokerId.toString)


Please remove BrokerIdLabel and MockMetricsReporter.BROKERID

addressed it :)

chia7712 · 2025-01-25T20:29:43Z

this PR is related to zk configs cleanup, so I'm going to backport to 4.0

Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

chia7712 · 2025-02-06T05:53:15Z

+    props.setProperty(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG, "CONTROLLER")
+    props.setProperty(KRaftConfigs.NODE_ID_CONFIG, "1")
+    props.setProperty(SocketServerConfigs.LISTENERS_CONFIG, "PLAINTEXT://localhost:0,CONTROLLER://localhost:5000")
+    props.setProperty(QuorumConfig.QUORUM_VOTERS_CONFIG, "2@localhost:5000")


this config is invalid. It uses combined mode so the id should be aligned with node.id. 2@localhost:5000 -> 1@localhost:5000 - otherwise, the test cases using this config will always get invalid KafkaConfig

@mingyen066 Could you please file a minor to fix it? @m1a2st is busy today

yeah, I've create a minor fix PR
#18815

Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

fix the error test

1908217

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

m1a2st added 7 commits January 15, 2025 20:13

remove the zookeeper comment

dbc87c4

Merge branch 'trunk' into KAFKA-18533

1ee34b4

# Conflicts: # core/src/main/scala/kafka/server/KafkaConfig.scala

resolve conflict

7bc0660

update some missing

c9b8a34

do more clean up

57c6334

Merge branch 'trunk' into KAFKA-18533

5ac12a3

# Conflicts: # core/src/main/scala/kafka/server/DynamicBrokerConfig.scala # core/src/main/scala/kafka/server/KafkaConfig.scala # core/src/main/scala/kafka/server/MetadataSupport.scala # core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala

resolve the conflict

5b687b3

frankvicky mentioned this pull request Jan 17, 2025

Remove unreferenced zk related properties #18584

Closed

3 tasks

Merge branch 'trunk' into KAFKA-18533

c81d9bb

# Conflicts: # core/src/main/scala/kafka/controller/KafkaController.scala

chia7712 added the ci-approved label Jan 18, 2025

addressed by comments

719905a

ijuma reviewed Jan 22, 2025

View reviewed changes

github-actions Bot removed the triage PRs from the community label Jan 22, 2025

m1a2st added 3 commits January 22, 2025 14:00

Merge branch 'trunk' into KAFKA-18533

90e50e7

addressed by comment

9644491

remove unused import

b6992fc

ijuma reviewed Jan 22, 2025

View reviewed changes

m1a2st added 3 commits January 23, 2025 12:22

fix the test

909aabc

addressed by comments in PlaintextAdminIntegrationTest

221fde7

update the comments

fe5f209

chia7712 reviewed Jan 23, 2025

View reviewed changes

chia7712 reviewed Jan 24, 2025

View reviewed changes

m1a2st added 2 commits January 24, 2025 19:32

Merge branch 'trunk' into KAFKA-18533

f8a8192

addressed by comments

46a9460

github-actions Bot removed the small Small PRs label Jan 24, 2025

chia7712 approved these changes Jan 25, 2025

View reviewed changes

chia7712 merged commit c40e7a1 into apache:trunk Jan 25, 2025

mingyen066 mentioned this pull request Jan 25, 2025

KAFKA-18631: Remove ZkConfigs #18693

Merged

3 tasks

chia7712 pushed a commit that referenced this pull request Jan 25, 2025

KAFKA-18533 Remove KafkaConfig zookeeper related logic (#18547)

3ee4508

Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

pranavt84 pushed a commit to pranavt84/kafka that referenced this pull request Jan 27, 2025

KAFKA-18533 Remove KafkaConfig zookeeper related logic (apache#18547)

55fc39c

Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

airlock-confluentinc Bot pushed a commit to confluentinc/kafka that referenced this pull request Jan 27, 2025

KAFKA-18533 Remove KafkaConfig zookeeper related logic (apache#18547)

b11be53

Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

chia7712 reviewed Feb 6, 2025

View reviewed changes

pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025

KAFKA-18533 Remove KafkaConfig zookeeper related logic (apache#18547)

07cacb9

Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025

KAFKA-18533 Remove KafkaConfig zookeeper related logic (apache#18547)

6bfec88

Reviewers: Ismael Juma <ismael@juma.me.uk>, Chia-Ping Tsai <chia7712@gmail.com>

Conversation

m1a2st commented Jan 15, 2025

Committer Checklist (excluded from commit message)

Uh oh!

m1a2st commented Jan 15, 2025

Uh oh!

ijuma commented Jan 18, 2025

Uh oh!

ijuma Jan 22, 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!

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

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!

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

chia7712 commented Jan 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ijuma Jan 22, 2025 •

edited

Loading