KAFKA-10774; Support Describe topic using topic IDs by dengziming · Pull Request #9769 · apache/kafka

dengziming · 2020-12-19T10:42:50Z

More detailed description of your change

Add topicNames in MetadataCache and alter KafkaApis.handleTopicMetadataRequest according to the KIP
Add method describeTopics(TopicCollection) in KafkaAdminClient, similar to describeTopics
Change DescribeTopicsResult to support TopicId

Summary of testing strategy (including rationale)
Test locally, here is some result:
New server + new Client

kafka-topics.sh --describe --zookeeper localhost:2181 --topic-id a-Paxi-LSka9sIVy9UFp_Q
Exception in thread "main" java.lang.IllegalArgumentException: --topic-id can used only with --bootstrap-server

kafka-topics.sh --describe --bootstrap-server localhost:9092 --topic-id a-Paxi-LSka9sIVy9UFp_Q
Topic: old-version-topic	TopicId: a-Paxi-LSka9sIVy9UFp_Q	PartitionCount: 2	ReplicationFactor: 1	Configs: segment.bytes=1073741824
	Topic: old-version-topic	Partition: 0	Leader: 0	Replicas: 0	Isr: 0
	Topic: old-version-topic	Partition: 1	Leader: 0	Replicas: 0	Isr: 0

Old Server + new Client

kafka-topics.sh --describe --bootstrap-server localhost:9092 --topic-id a-Paxi-LSka9sIVy9UFp_Q
Error while executing topic command : TopicId a-Paxi-LSka9sIVy9UFp_Q not found.

New server + old client

kafka-topics.sh --describe --bootstrap-server localhost:9092 --topic-id a-Paxi-LSka9sIVy9UFp_Q
Exception in thread "main" joptsimple.UnrecognizedOptionException: topic-id is not a recognized option

Committer Checklist (excluded from commit message)

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

clients/src/main/java/org/apache/kafka/common/protocol/Errors.java

clients/src/main/java/org/apache/kafka/clients/admin/DescribeTopicsResult.java

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

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

rajinisivaram

@dengziming Thanks for the PR, left some comments. The main thing to watch out for is compatibility of the public API.

clients/src/main/java/org/apache/kafka/clients/admin/DescribeTopicsResult.java

clients/src/main/java/org/apache/kafka/clients/admin/TopicDescription.java

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java

clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java

core/src/main/scala/kafka/admin/TopicCommand.scala

rajinisivaram · 2021-01-20T22:34:28Z

@dengziming #9814 has been merged, so this needs rebasing and the check for IBP. Thanks.

jolshan · 2021-01-20T23:11:35Z

@dengziming I realized that for this, it really depends on the IBP of the controller. (That is, we need UpdateMetadata to send topic IDs to all the brokers). So maybe instead of checking IBP it would make sense to check if the MetadataCache does not have any topic IDs. What do you think?

dengziming · 2021-01-21T03:54:59Z

#9769 (comment)

@rajinisivaram @jolshan I also realized this, we add topicId in MetadataCache from IBP=2.8.1, so instead of using config.usesTopicId it would make sense to check config.interBrokerProtocolVersion >= KAFKA_2_8_IV1

dengziming · 2021-01-21T06:02:37Z

@rajinisivaram I rebased the code and added a test in PlaintextAdminIntegrationTest, PTAL.

rajinisivaram

@dengziming Thanks for the updates. Left some comments/questions. I ran one of the tests and there were NullPointerExceptions, so those needs fixing as well.

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

clients/src/main/java/org/apache/kafka/clients/admin/TopicDescription.java

clients/src/main/java/org/apache/kafka/common/errors/UnknownTopicIdException.java

core/src/main/scala/kafka/admin/TopicCommand.scala

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

dengziming · 2021-01-22T05:25:40Z

Hello, @rajinisivaram , I resolved the comments/questions and fixed the NullPointerExceptions, call for a third review.

Add topicId toTopicListing and add TopicCommand.getTopicIds
move the code shared between describeTopics and describeTopicsWithIds to a common private method getTopicDescriptionFromCluster.

rajinisivaram

@dengziming Thanks for the updates and tests, looks good. Left just a couple of minor comments. @jolshan Can you also review to make sure I haven't missed anything? Thanks.

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala

rajinisivaram

@dengziming Thanks for the updates, LGTM. Will merge after @jolshan has a chance to review.

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

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala

jolshan · 2021-01-25T17:16:25Z

Thanks for the updates and all the hard work! I added a few comments. Mostly just questions for clarification.

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

satishd

thanks @dengziming for the PR. Oveall LGTM, left minor comments.

clients/src/main/java/org/apache/kafka/clients/admin/TopicListing.java

core/src/main/scala/kafka/admin/TopicCommand.scala

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

satishd

thanks @dengziming for addressing the review comments, LGTM.

dengziming · 2021-07-28T01:57:13Z

@rajinisivaram Are you interested in taking this across finish line? I think this has something that is very close to a first version, we can try to land that and then improvise since this has been worked for a long time, thank you!

jolshan · 2021-07-28T02:43:59Z

@dengziming -- @rajinisivaram can help in about 2 weeks. I will try to help review so it will be ready when she gets to it.

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

clients/src/main/java/org/apache/kafka/clients/admin/DescribeTopicsResult.java

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala

rajinisivaram

@dengziming Thanks for the updates, looks good. Left some minor comments, I think it is ready to go after that.

clients/src/main/java/org/apache/kafka/clients/admin/DescribeTopicsResult.java

clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java

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

core/src/main/scala/kafka/admin/TopicCommand.scala

KAFKA-10774; add topicId in TopicCommand Gate topic IDs behind IBP 2.8.1 add ITCase of metadata request edge case between different ipb Add Unauthorized unit test use matchSameElements replace EasyMock.eq since the order of the request is unknown resolve conflicts check KAFKA-12394 check KAFKA-12701

rajinisivaram

@dengziming Thanks for the updates, LGTM. Sorry about the delay in getting this in and thanks for your patience! Merging to trunk.

Reviewers: Justine Olshan <jolshan@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, Satish Duggana <satishd@apache.org>, Rajini Sivaram <rajinisivaram@googlemail.com>

… the deprecated `all` API ### What changes were proposed in this pull request? This PR aims to use `DescribeTopicsResult.allTopicNames` API instead of the deprecated `all` API. ### Why are the changes needed? `all` API was deprecated at Apache Kafka 3.1.0 and removed at 4.0.0. - apache/kafka#9769 - apache/kafka#18255 > The <code>all()</code> method was removed from the <code>org.apache.kafka.clients.admin.DescribeTopicsResult</code>. Please use <code>allTopicNames()</code> instead. ### Does this PR introduce _any_ user-facing change? No behavior change. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #52597 from dongjoon-hyun/DescribeTopicsResult. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

… the deprecated `all` API ### What changes were proposed in this pull request? This PR aims to use `DescribeTopicsResult.allTopicNames` API instead of the deprecated `all` API. ### Why are the changes needed? `all` API was deprecated at Apache Kafka 3.1.0 and removed at 4.0.0. - apache/kafka#9769 - apache/kafka#18255 > The <code>all()</code> method was removed from the <code>org.apache.kafka.clients.admin.DescribeTopicsResult</code>. Please use <code>allTopicNames()</code> instead. ### Does this PR introduce _any_ user-facing change? No behavior change. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#52597 from dongjoon-hyun/DescribeTopicsResult. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

jolshan reviewed Dec 19, 2020

View reviewed changes

clients/src/main/java/org/apache/kafka/common/protocol/Errors.java Outdated Show resolved Hide resolved

jolshan reviewed Dec 21, 2020

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/admin/DescribeTopicsResult.java Outdated Show resolved Hide resolved

jolshan reviewed Dec 21, 2020

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java Outdated Show resolved Hide resolved

dengziming force-pushed the KAFKA-10774 branch from 6018b4b to 73a8ea1 Compare January 2, 2021 04:09

jolshan reviewed Jan 4, 2021

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved

rajinisivaram reviewed Jan 12, 2021

View reviewed changes

dengziming force-pushed the KAFKA-10774 branch 2 times, most recently from 0e25260 to 5df1f32 Compare January 13, 2021 14:42

dengziming requested a review from rajinisivaram January 14, 2021 23:22

dengziming force-pushed the KAFKA-10774 branch from 311f488 to 4e20a8d Compare January 21, 2021 03:58

rajinisivaram reviewed Jan 21, 2021

View reviewed changes

dengziming requested a review from rajinisivaram January 23, 2021 15:59

rajinisivaram reviewed Jan 25, 2021

View reviewed changes

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala Outdated Show resolved Hide resolved

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala Outdated Show resolved Hide resolved

dengziming requested a review from rajinisivaram January 25, 2021 13:09

rajinisivaram approved these changes Jan 25, 2021

View reviewed changes

jolshan reviewed Jan 25, 2021

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved

jolshan reviewed Jan 25, 2021

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved

jolshan reviewed Jan 25, 2021

View reviewed changes

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala Outdated Show resolved Hide resolved

jolshan reviewed Jan 25, 2021

View reviewed changes

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala Outdated Show resolved Hide resolved

jolshan reviewed Jan 25, 2021

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved

jolshan reviewed Jan 26, 2021

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved

satishd reviewed Jan 26, 2021

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/admin/TopicListing.java Outdated Show resolved Hide resolved

core/src/main/scala/kafka/admin/TopicCommand.scala Outdated Show resolved Hide resolved

jolshan reviewed Jan 27, 2021

View reviewed changes

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved

satishd approved these changes Jan 28, 2021

View reviewed changes

dengziming force-pushed the KAFKA-10774 branch 2 times, most recently from 8da4a39 to 83c3f0d Compare July 24, 2021 12:49

jolshan reviewed Jul 28, 2021

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java Outdated Show resolved Hide resolved

jolshan reviewed Jul 28, 2021

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java Outdated Show resolved Hide resolved

jolshan reviewed Jul 28, 2021

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/admin/DescribeTopicsResult.java Outdated Show resolved Hide resolved

jolshan reviewed Jul 28, 2021

View reviewed changes

core/src/test/scala/integration/kafka/server/MetadataRequestBetweenDifferentIbpTest.scala Outdated Show resolved Hide resolved

dengziming force-pushed the KAFKA-10774 branch 2 times, most recently from cedaec4 to 140ade8 Compare July 31, 2021 08:41

rajinisivaram reviewed Aug 27, 2021

View reviewed changes

dengziming added 9 commits August 27, 2021 22:17

check KAFKA-12976

605259e

check KAFKA-13011: using TopicCollection

e3ae1b2

resolve comments and rebase trunk

97b45f6

rebase trunk and resolve conflicts

0965346

more feedback from comments

ced95b0

remove usage of deprecated methods

acc8ba9

more feedbacks from comments

9a7a395

more feedbacks from rajinisivaram

d0bc3cd

dengziming force-pushed the KAFKA-10774 branch from 140ade8 to d0bc3cd Compare August 27, 2021 15:31

fix error version

a443b4f

rajinisivaram approved these changes Aug 28, 2021

View reviewed changes

rajinisivaram merged commit 1d22b0d into apache:trunk Aug 28, 2021

lct45 mentioned this pull request Aug 30, 2021

chore: resolve ambiguous method reference confluentinc/ksql#8078

Merged

dengziming deleted the KAFKA-10774 branch October 8, 2022 11:52

dongjoon-hyun mentioned this pull request Oct 13, 2025

[SPARK-53892][SS] Use DescribeTopicsResult.allTopicNames instead of the deprecated all API apache/spark#52597

Closed

Conversation

dengziming commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajinisivaram commented Jan 20, 2021

Uh oh!

jolshan commented Jan 20, 2021

Uh oh!

dengziming commented Jan 21, 2021

Uh oh!

dengziming commented Jan 21, 2021

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dengziming commented Jan 22, 2021

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jolshan commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

satishd left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

satishd left a comment

Choose a reason for hiding this comment

Uh oh!

dengziming commented Jul 28, 2021

Uh oh!

jolshan commented Jul 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

dengziming commented Dec 19, 2020 •

edited

Loading

jolshan commented Jan 25, 2021 •

edited

Loading

satishd left a comment •

edited

Loading