Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-18431: Remove KafkaController #18573

Merged
merged 2 commits into from
Jan 17, 2025
Merged

KAFKA-18431: Remove KafkaController #18573

merged 2 commits into from
Jan 17, 2025

Conversation

FrankYang0529
Copy link
Member

Remove KafkaController and related unused references:

  • ControllerChannelContext
  • ControllerChannelManager
  • ControllerEventManager
  • ControllerState
  • PartitionStateMachine
  • ReplicaStateMachine
  • TopicDeletionManager
  • ZkBrokerEpochManager

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added the core Kafka Broker label Jan 16, 2025
ijuma
ijuma reviewed Jan 16, 2025
View reviewed changes
core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala
assertNotEquals(broker1.hashCode, broker3.hashCode)
assertNotEquals(broker1.hashCode, broker4.hashCode)

assertEquals(Some(1), Map(broker1 -> 1).get(broker1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this test is unrelated to the zk stuff, but not too useful if we convert BrookerEndPoint to a record. I submitted #18577 for the latter.

ijuma
ijuma approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean-up! LGTM.

@ijuma
Copy link
Member

ijuma commented Jan 16, 2025

There were a few test failures, re-running the tests to see if these are flakes. If you get a chance, try to run the failures locally to see if they are related.

@ijuma ijuma mentioned this pull request Jan 16, 2025
Merged
3 tasks
@FrankYang0529
Copy link
Member Author

FrankYang0529 commented Jan 16, 2025 •
edited
Loading

If you get a chance, try to run the failures locally to see if they are related.

Yes, they're related, I get error with following cases:

  • DeleteSegmentsByRetentionSizeTest#executeTieredStorageTest
  • DeleteSegmentsByRetentionTimeTest#executeTieredStorageTest
  • RollAndOffloadActiveSegmentTest#executeTieredStorageTest
  • DisableRemoteLogOnTopicTest#executeTieredStorageTest
  • EnableRemoteLogOnTopicTest#executeTieredStorageTest
  • RemoteTopicCrudTest#testClusterWideDisablementOfTieredStorageWithEnabledTieredTopic
  • DynamicConfigChangeTest#testConfigChange
  • DynamicConfigChangeTest#testDynamicTopicConfigChange

@FrankYang0529
Copy link
Member Author

I will do some debug. Thanks for the review.

ijuma
ijuma requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@ijuma ijuma left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating review to indicate we need to fix the test failures.

@FrankYang0529
Copy link
Member Author

I found root cause. I will push the comment again. Do we want to keep BrokerEndpointTest? If yes, I will remove zk related cases only.

@ijuma
Copy link
Member

ijuma commented Jan 16, 2025

Do we want to keep BrokerEndpointTest?

You can remove it. With the conversion to record via the other PR, the remaining test doesn't add value.

@ijuma
Copy link
Member

ijuma commented Jan 16, 2025 •
edited
Loading

Btw, when you fix a PR, it's much easier for the reviewer, if you add additional commits versus rewriting the original commit (which has 7k lines of changes and some small new change somewhere in this case). In any case, can you describe what the fix was in this case?

@FrankYang0529
Copy link
Member Author

FrankYang0529 commented Jan 16, 2025 •
edited
Loading

can you describe what the fix was in this case?

Sorry for that. I deleted code in TopicConfigHandler#updateLogConfig. We should not remove this. We can just fix it by removing last argument in LogManager#updateTopicConfig.

Before (remove all following code):

    // kafkaController is only defined in Zookeeper's mode
    logManager.updateTopicConfig(topic, topicConfig, kafkaConfig.remoteLogManagerConfig.isRemoteStorageSystemEnabled(),
      wasRemoteLogEnabled, kafkaController.isDefined)

After (only remove last argument):

    logManager.updateTopicConfig(topic, topicConfig, kafkaConfig.remoteLogManagerConfig.isRemoteStorageSystemEnabled(),
      wasRemoteLogEnabled)

@ijuma
Copy link
Member

ijuma commented Jan 16, 2025

Found 5 test failures:
FAILED ❌ SaslPlainPlaintextConsumerTest > testCoordinatorFailover(String, String).quorum=kraft.groupProtocol=consumer
FAILED ❌ TransactionsBounceTest > testWithGroupMetadata(String, String).quorum=kraft.groupProtocol=classic
FAILED ❌ ClientIdQuotaTest > testThrottledProducerConsumer(String, String).quorum=kraft.groupProtocol=classic
FAILED ❌ ClientIdQuotaTest > testThrottledProducerConsumer(String, String).quorum=kraft.groupProtocol=consumer
FAILED ❌ ClientIdQuotaTest > testQuotaOverrideDelete(String, String).quorum=kraft.groupProtocol=consumer
Found 4 flaky test failures:
FLAKY ⚠️ EosIntegrationTest > shouldCheckpointRestoredOffsetsWhenClosingCleanDuringRestoringStateUpdaterDisabled()
FLAKY ⚠️ AbstractCoordinatorTest > testWakeupAfterSyncGroupReceivedExternalCompletion()
FLAKY ⚠️ AbstractCoordinatorTest > testWakeupAfterSyncGroupSentExternalCompletion()
FLAKY ⚠️ KafkaAdminClientTest > testAdminClientApisAuthenticationFailure()

@FrankYang0529
Copy link
Member Author

@ijuma, Those cases can't be reproduced on my laptop. I merge trunk to trigger CI again. Thanks.

@ijuma
Copy link
Member

ijuma commented Jan 17, 2025

Looks like the tests passed.

@ijuma ijuma merged commit 0e502e0 into apache:trunk Jan 17, 2025
9 checks passed
@FrankYang0529 FrankYang0529 deleted the KAFKA-18431 branch January 17, 2025 02:56
@FrankYang0529
Copy link
Member Author

Hi @ijuma, thanks for the review. Will you backport this to 4.0? I try to cherry-pick locally and there is no conflict. Thanks.

@ijuma
Copy link
Member

ijuma commented Jan 17, 2025

Yes, I will. I do the backports in batches and update the JIRA once they're done.

TaiJuWu pushed a commit to TaiJuWu/kafka that referenced this pull request Jan 17, 2025
@FrankYang0529 @TaiJuWu
KAFKA-18431: Remove KafkaController (apache#18573)
dff7c72 
Remove KafkaController and related unused references:

* ControllerChannelContext
* ControllerChannelManager
* ControllerEventManager
* ControllerState
* PartitionStateMachine
* ReplicaStateMachine
* TopicDeletionManager
* ZkBrokerEpochManager

Reviewers: Ismael Juma <[email protected]>
ijuma pushed a commit that referenced this pull request Jan 17, 2025
@FrankYang0529 @ijuma
KAFKA-18431: Remove KafkaController (#18573)
f93588b 
Remove KafkaController and related unused references:

* ControllerChannelContext
* ControllerChannelManager
* ControllerEventManager
* ControllerState
* PartitionStateMachine
* ReplicaStateMachine
* TopicDeletionManager
* ZkBrokerEpochManager

Reviewers: Ismael Juma <[email protected]>
ijuma added a commit that referenced this pull request Jan 17, 2025
@ijuma
Convert BrokerEndPoint to record (#18577)
b90ef86 
Tests for this class is being removed via #18573 - making it
a record avoids the need for the tests being removed.

Reviewers: Andrew Schofield <[email protected]>
@frankvicky frankvicky mentioned this pull request Jan 19, 2025
Merged
3 tasks
@chia7712 chia7712 mentioned this pull request Jan 19, 2025
Merged
3 tasks
chia7712
chia7712 reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FrankYang0529 I apologize for the delayed response. Could you please assist me in enhancing the content of zk2kraft.html?

core/src/main/scala/kafka/controller/ControllerChannelManager.scala
import scala.jdk.CollectionConverters._

object ControllerChannelManager {
private val QueueSizeMetricName = "QueueSize"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update zk2kraft.html to say all metrics under kafka.controller:type=ControllerChannelManager are removed?

core/src/main/scala/kafka/controller/ControllerEventManager.scala
eventQueueTimeTimeoutMs: Long = 300000) {
import ControllerEventManager._

private val metricsGroup = new KafkaMetricsGroup(this.getClass)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto. all metrics under kafka.controller:type=ControllerEventManager are removed

core/src/main/scala/kafka/controller/KafkaController.scala
threadNamePrefix: Option[String] = None)
extends ControllerEventProcessor with Logging {

private val metricsGroup = new KafkaMetricsGroup(this.getClass)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto. all metrics under kafka.controller:type=KafkaController are removed

core/src/main/scala/kafka/controller/KafkaController.scala
}

private[controller] class ControllerStats {
private val metricsGroup = new KafkaMetricsGroup(this.getClass)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto. kafka.controller:type=ControllerStats

@FrankYang0529 FrankYang0529 mentioned this pull request Jan 21, 2025
Open
3 tasks
@FrankYang0529
Copy link
Member Author

@chia7712 Thanks for the reminder. Create a PR for it.

#18654

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chia7712 chia7712 chia7712 left review comments

@ijuma ijuma ijuma requested changes

Assignees
No one assigned
Labels
core Kafka Broker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FrankYang0529 @ijuma @chia7712