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.

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

KAFKA-18399 Remove ZooKeeper from KafkaApis (1/N): LEADER_AND_ISR, STOP_REPLICA, UPDATE_METADATA #18417

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

m1a2st
Copy link
Contributor

@m1a2st m1a2st commented Jan 7, 2025

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

I will deal with these Api handler in this PR

  • ApiKeys.LEADER_AND_ISR
  • ApiKeys.STOP_REPLICA
  • ApiKeys.UPDATE_METADATA

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 triage PRs from the community core Kafka Broker tests Test fixes (including flaky tests) labels Jan 7, 2025
@m1a2st m1a2st changed the title KAFKA-18399 Remove ZooKeeper from KafkaApis (part 1) KAFKA-18399 Remove ZooKeeper from KafkaApis (1/N) Jan 7, 2025
@github-actions github-actions bot removed the triage PRs from the community label Jan 8, 2025
@m1a2st m1a2st changed the title KAFKA-18399 Remove ZooKeeper from KafkaApis (1/N) KAFKA-18399 Remove ZooKeeper from KafkaApis with ApiKeys LEADER_AND_ISR, STOP_REPLICA, UPDATE_METADATA (1/N) Jan 8, 2025
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.

@m1a2st thanks for this patch!

@@ -197,9 +196,6 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.FETCH => handleFetchRequest(request)
case ApiKeys.LIST_OFFSETS => handleListOffsetRequest(request)
case ApiKeys.METADATA => handleTopicMetadataRequest(request)
case ApiKeys.LEADER_AND_ISR => handleLeaderAndIsrRequest(request)
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 please use KafkaApis.shouldNeverReceive to keep the same error message?

@@ -197,9 +196,6 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.FETCH => handleFetchRequest(request)
case ApiKeys.LIST_OFFSETS => handleListOffsetRequest(request)
case ApiKeys.METADATA => handleTopicMetadataRequest(request)
case ApiKeys.LEADER_AND_ISR => handleLeaderAndIsrRequest(request)
case ApiKeys.STOP_REPLICA => handleStopReplicaRequest(request)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -197,9 +196,6 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.FETCH => handleFetchRequest(request)
case ApiKeys.LIST_OFFSETS => handleListOffsetRequest(request)
case ApiKeys.METADATA => handleTopicMetadataRequest(request)
case ApiKeys.LEADER_AND_ISR => handleLeaderAndIsrRequest(request)
case ApiKeys.STOP_REPLICA => handleStopReplicaRequest(request)
case ApiKeys.UPDATE_METADATA => handleUpdateMetadataRequest(request, requestLocal)
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -3034,99 +3032,7 @@ class KafkaApisTest extends Logging {
val markersResponse = capturedResponse.getValue
assertEquals(2, markersResponse.errorsByProducerId.size())
}

@Test
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 please add unit test to ensure those removed RPC can see correct error?

@m1a2st
Copy link
Contributor Author

m1a2st commented Jan 9, 2025

Thanks for @chia7712 review, In ApiMessageType, ListenerType has already been defined, so the checks at !apiVersionManager.isApiEnabled(request.header.apiKey, request.header.apiVersion) will filter them out. They won’t even have a chance to enter the switch statement.

@chia7712
Copy link
Member

chia7712 commented Jan 9, 2025

Yes, you are right. Removing the handler is fine

@github-actions github-actions bot added the small Small PRs label Jan 9, 2025
@github-actions github-actions bot removed the small Small PRs label Jan 9, 2025
@chia7712 chia7712 changed the title KAFKA-18399 Remove ZooKeeper from KafkaApis with ApiKeys LEADER_AND_ISR, STOP_REPLICA, UPDATE_METADATA (1/N) KAFKA-18399 Remove ZooKeeper from KafkaApis (1/N): LEADER_AND_ISR, STOP_REPLICA, UPDATE_METADATA Jan 9, 2025
@chia7712 chia7712 merged commit 307059c into apache:trunk Jan 9, 2025
15 of 16 checks passed
chia7712 pushed a commit that referenced this pull request Jan 9, 2025
…`STOP_REPLICA`, `UPDATE_METADATA` (#18417)

Delete the handlers for LEADER_AND_ISR, STOP_REPLICA, and UPDATE_METADATA. Also, remove the corresponding unit tests in KafkaApisTest.

Reviewers: Chia-Ping Tsai <[email protected]>
askew pushed a commit to askew/kafka that referenced this pull request Jan 23, 2025
…`STOP_REPLICA`, `UPDATE_METADATA` (apache#18417)

Delete the handlers for LEADER_AND_ISR, STOP_REPLICA, and UPDATE_METADATA. Also, remove the corresponding unit tests in KafkaApisTest.

Reviewers: Chia-Ping Tsai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved core Kafka Broker tests Test fixes (including flaky tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants