KAFKA-17616: Remove KafkaServer by mimaison · Pull Request #18384 · apache/kafka

mimaison · 2025-01-03T18:17:02Z

Committer Checklist (excluded from commit message)

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

ijuma

Thanks for the PR, left a few comments.

ijuma · 2025-01-04T00:31:17Z

-        Time.SYSTEM,
-        threadNamePrefix = None
-      )
+      throw new RuntimeException("ZooKeeper is not supported")


Can we not simply remove this class? It's not a public class and I don't think anything references it besides the relevant shell script.

If the KafkaServer is still existent, we can't remove many unused classes/configs from code base. For example, zk configs and ZkMetadataCache. Do we have any use cases for KafkaServer in 4.0?

I actually meant something else - I was asking whether we need to keep kafka.Kafka. The answer is yes since it is still used for KRaft. But looking at this in more detail, this exception message doesn't make sense in a world where ZK is not supported since it will throw this when process roles is empty.

We should simply remove this conditional code and leave it to KafkaConfig to throw the appropriate exception if processRoles is empty.

Sorry for misunderstanding your comment

We should simply remove this conditional code and leave it to KafkaConfig to throw the appropriate exception if processRoles is empty.

make sense

ijuma · 2025-01-04T00:32:21Z

      server match {
-        case kafkaServer: KafkaServer => kafkaServer.kafkaController.updateBrokerInfo(kafkaServer.createBrokerInfo)
-        case _ => throw new RuntimeException("Unable to handle non-kafkaServer")
+        case _ => throw new RuntimeException("Unable to handle reconfigure")


If we always throw this exception, we probably don't need to do verifyListenerRegistrationAlterationSupported before that and we may be able to delete that method if it's not used anywhere else.

Also, we probably should include more context: it looks like we don't allow dynamic reconfiguration of listener registrations.

@cmccabe @jsancio Is this documented as part of the zk to kraft migration?

We should remove SocketServerConfigs.ADVERTISED_LISTENERS_CONFIG from ReconfigurableConfigs as well.

kafka/core/src/main/scala/kafka/server/DynamicBrokerConfig.scala

Line 912 in a628d9b

SocketServerConfigs.ADVERTISED_LISTENERS_CONFIG,

That can disallow users to configure advertised listeners dynamically.

I open a Jira to check all dynamic config which can't dynamic configure in Kraft, and only find ADVERTISED_LISTENERS_CONFIG can't, If we addressed it on this PR, I will close Jira and #18390

There's a bunch of other ZooKeeper related logic in this class so I was planning to address them together in a follow up issue. Deleting KafkaServer enables us to start removing other classes that depend on this one.

There's a bunch of other ZooKeeper related logic in this class so I was planning to address them together in a follow up issue. Deleting KafkaServer enables us to start removing other classes that depend on this one.

I'm ok to merge this and then address comments in the follow-up, because there are many cleanup blocked by KafkaServer

I'm ok with that, but let's file a JIRA so we don't lose track.

I care particularly about the error messages and such things since they are not easy to find afterwards.

I opened https://issues.apache.org/jira/browse/KAFKA-18405 with details about this issue.

ijuma · 2025-01-04T00:33:30Z

   */
  val STARTED_MESSAGE = "Kafka Server started"
+
+  val MIN_INCREMENTAL_FETCH_SESSION_EVICTION_MS: Long = 120000


It's a bit odd that we're not following the Scala convention here. Since we're rewriting this stuff in Java anyway, maybe it's ok.

chia7712

+1 due to #18384 (comment)

It would be nice to get feedback from @ijuma before merging it

mimaison · 2025-01-06T10:29:35Z

Thanks for the reviews. I pushed another commit removing a bunch of methods/classes that were only used by KafkaServer.

chia7712

LGTM, we are lucky that all CI are green :)

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

mimaison · 2025-01-06T13:54:44Z

Applied to 4.0 too: db0fc78

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

ijuma · 2025-01-06T18:30:52Z


  /** ********* Controlled shutdown configuration ***********/
-  val controlledShutdownMaxRetries = getInt(ServerConfigs.CONTROLLED_SHUTDOWN_MAX_RETRIES_CONFIG)
-  val controlledShutdownRetryBackoffMs = getLong(ServerConfigs.CONTROLLED_SHUTDOWN_RETRY_BACKOFF_MS_CONFIG)


Hmm, is it expected that this is not respected anymore? cc @mumrah @jsancio @cmccabe ?

Or is the KRaft code using ServerConfigs.CONTROLLED_SHUTDOWN_MAX_RETRIES_CONFIG?

it seems kraft broker relies on the event queue sending BrokerHeartbeatRequest with wantShutDown=true to complete controlled shutdown. The event queue already have retry mechanism and backoff so maybe we don't need to have extra retries and backoff configs.

I have opened https://issues.apache.org/jira/browse/KAFKA-18417 to remove both configs

We should also add a note to the zk to kraft migration guide regarding this. Said the same in the JIRA ticket.

We should also add a note to the zk to kraft migration guide regarding this. Said the same in the JIRA ticket.

Yes, I have added the note to KAFKA-18364

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

KAFKA-17616: Remove KafkaServer

ae3f65e

github-actions Bot added the core Kafka Broker label Jan 3, 2025

ijuma reviewed Jan 4, 2025

View reviewed changes

chia7712 approved these changes Jan 5, 2025

View reviewed changes

Cleanup unused methods/classes

6572d5d

m1a2st approved these changes Jan 6, 2025

View reviewed changes

chia7712 approved these changes Jan 6, 2025

View reviewed changes

mimaison merged commit 64bbdb1 into apache:trunk Jan 6, 2025

mimaison deleted the kafka-17616 branch January 6, 2025 13:36

mimaison added a commit that referenced this pull request Jan 6, 2025

KAFKA-17616: Remove KafkaServer (#18384)

db0fc78

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

tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025

KAFKA-17616: Remove KafkaServer (apache#18384)

9f54cd0

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

ijuma reviewed Jan 6, 2025

View reviewed changes

Yunyung mentioned this pull request Jan 8, 2025

KAFKA-18417: Remove controlled.shutdown.max.retries and controlled.shutdown.retry.backoff.ms #18431

Merged

3 tasks

This was referenced Jan 13, 2025

KAFKA-18405: Remove ZooKeeper logic from DynamicBrokerConfig #18508

Merged

KAFKA-18591: Remove unused QuotaConfgHandler #18617

Merged

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

KAFKA-17616: Remove KafkaServer (apache#18384)

c92e327

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

Conversation

mimaison commented Jan 3, 2025

Committer Checklist (excluded from commit message)

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!

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!

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

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!

mimaison commented Jan 6, 2025

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jan 6, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

m1a2st Jan 5, 2025 •

edited

Loading