Skip to content

KAFKA-7808: AdminClient#describeTopics should not throw InvalidTopicException if topic name is not found#6124

Merged
cmccabe merged 1 commit intoapache:trunkfrom
dongjinleekr:feature/KAFKA-7808
Jan 11, 2019
Merged

KAFKA-7808: AdminClient#describeTopics should not throw InvalidTopicException if topic name is not found#6124
cmccabe merged 1 commit intoapache:trunkfrom
dongjinleekr:feature/KAFKA-7808

Conversation

@dongjinleekr
Copy link
Copy Markdown
Contributor

Currently, AdminClient#describeTopics is called in WorkerUtils#[getMatchingTopicPartitions,verifyTopics] only. However, WorkerUtils#getMatchingTopicPartitions does not being called by anyone - so it should be removed.

  1. Update KafkaAdminClient#describeTopics to throw UnknownTopicOrPartitionException.
  2. Change the Exception message thrown by MockAdminClient#describeTopics: for consistency with KafkaAdminClient.
  3. Remove unused method: WorkerUtils#getMatchingTopicPartitions.
  4. Add @throws description on WorkerUtils#verifyTopics: when UnknownTopicOrPartitionException is thrown.
  5. Expand javadoc on WorkerUtils#createTopics(Logger, AdminClient, Map, boolean): the only method which calls WorkerUtils#verifyTopics.

Since the Throwable instance thrown by WorkerUtils#createTopics(Logger, AdminClient, Map, boolean) is caught by [ProduceBenchWorker,RoundTripWorker].Prepare, we don't need more cleanup.

Committer Checklist (excluded from commit message)

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

…if topic name is not found

Currently, AdminClient#describeTopics is called in WorkerUtils#[getMatchingTopicPartitions,verifyTopics] only. However, WorkerUtils#getMatchingTopicPartitions does not being called - So it should be removed.

1. Update KafkaAdminClient#describeTopics to throw UnknownTopicOrPartitionException.
2. Change the Exception message thrown by MockAdminClient#describeTopics: for consistency with KafkaAdminClient.
3. Remove unused method: WorkerUtils#getMatchingTopicPartitions.
4. Add @throws description on WorkerUtils#verifyTopics: when UnknownTopicOrPartitionException is thrown.
5. Expand Javadoc on WorkerUtils#createTopics(Logger, AdminClient, Map, boolean): the only method which calles WorkerUtils#verifyTopics.

Since the Throwable instance thrown by WorkerUtils#createTopics(Logger, AdminClient, Map, boolean) is catched by [ProduceBenchWorker,RoundTripWorker].Prepare, we don't need more cleanup.
@dongjinleekr dongjinleekr changed the title AdminClient#describeTopics should not throw InvalidTopicException if topic name is not found KAFKA-7808: AdminClient#describeTopics should not throw InvalidTopicException if topic name is not found Jan 11, 2019
@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Jan 11, 2019

Throwing InvalidTopicException here was a bug; it should have been UnknownTopicOrPartitionException. LGTM

@cmccabe cmccabe merged commit 7df3e8c into apache:trunk Jan 11, 2019
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 12, 2019

We should add a note to the upgrade notes probably.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 12, 2019

Cc @guozhangwang

@guozhangwang
Copy link
Copy Markdown
Contributor

LGTM! Thanks.

@guozhangwang
Copy link
Copy Markdown
Contributor

@ijuma Should this be cherry-picked to older branches?

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 14, 2019

I think it's safest to only do it in trunk since it's a slight change in behaviour even if it's a bug fix. We should have an upgrade note too.

jimjag pushed a commit to jimjag/kafka that referenced this pull request Jan 18, 2019
…ger (apache#6167)

Note we can only remove this handling in 2.2 but not in 2.1 since apache#6124 is only in 2.2.

Reviewers: Bill Bejeck <bill@confluent.io>, Damian Guy <damian@confluent.io>, Matthias J. Sax <matthias@confluent.io>
abbccdda pushed a commit to abbccdda/kafka that referenced this pull request Jan 24, 2019
…ger (apache#6167)

Note we can only remove this handling in 2.2 but not in 2.1 since apache#6124 is only in 2.2.

Reviewers: Bill Bejeck <bill@confluent.io>, Damian Guy <damian@confluent.io>, Matthias J. Sax <matthias@confluent.io>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…if topic name is not found (apache#6124)

* Update KafkaAdminClient#describeTopics to throw UnknownTopicOrPartitionException.

* Remove unused method: WorkerUtils#getMatchingTopicPartitions.

* Add some JavaDoc.

Reviewed-by: Colin P. McCabe <cmccabe@apache.org>, Ryanne Dolan <ryannedolan@gmail.com>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…ger (apache#6167)

Note we can only remove this handling in 2.2 but not in 2.1 since apache#6124 is only in 2.2.

Reviewers: Bill Bejeck <bill@confluent.io>, Damian Guy <damian@confluent.io>, Matthias J. Sax <matthias@confluent.io>
jolshan pushed a commit that referenced this pull request Jan 4, 2024
… ID (#14599)

Throw UnknownTopicIdException instead of InvalidTopicException when no name is found for the topic ID.

Similar to #6124 for describeTopics using a topic name. MockAdminClient already makes use of UnknownTopicIdException for this case.

Reviewers: Justine Olshan <jolshan@confluent.io>, Ashwin Pankaj <apankaj@confluent.io>
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
… ID (apache#14599)

Throw UnknownTopicIdException instead of InvalidTopicException when no name is found for the topic ID.

Similar to apache#6124 for describeTopics using a topic name. MockAdminClient already makes use of UnknownTopicIdException for this case.

Reviewers: Justine Olshan <jolshan@confluent.io>, Ashwin Pankaj <apankaj@confluent.io>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
… ID (apache#14599)

Throw UnknownTopicIdException instead of InvalidTopicException when no name is found for the topic ID.

Similar to apache#6124 for describeTopics using a topic name. MockAdminClient already makes use of UnknownTopicIdException for this case.

Reviewers: Justine Olshan <jolshan@confluent.io>, Ashwin Pankaj <apankaj@confluent.io>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
… ID (apache#14599)

Throw UnknownTopicIdException instead of InvalidTopicException when no name is found for the topic ID.

Similar to apache#6124 for describeTopics using a topic name. MockAdminClient already makes use of UnknownTopicIdException for this case.

Reviewers: Justine Olshan <jolshan@confluent.io>, Ashwin Pankaj <apankaj@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants