KAFKA-12329; kafka-reassign-partitions command should give a better error message when a topic does not exist by dajac · Pull Request #10141 · apache/kafka

dajac · 2021-02-17T09:48:24Z

kafka-reassign-partitions command gives a generic error message when one tries to reassign a topic which does not exist:

$ ./bin/kafka-reassign-partitions.sh --bootstrap-server localhost:9092 --execute --reassignment-json-file reassignment.json
Error: org.apache.kafka.common.errors.UnknownTopicOrPartitionException: This server does not host this topic-partition.

When the reassignment contains multiple topics, it is hard to find out the correct one. This PR improves this to give the name of the topic in the error:

$ ./bin/kafka-reassign-partitions.sh --bootstrap-server localhost:9092 --execute --reassignment-json-file reassignment.json
Error: org.apache.kafka.common.errors.UnknownTopicOrPartitionException: Topic test-test not found.

Committer Checklist (excluded from commit message)

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

chia7712

@dajac Nice improvement. Some minor comments are left. Please take a look.

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java#L1893 also check the existence of topic. Will it become a redundant check after we merge this PR?
Should we apply this readable error message to other APIs? deleteTopics, for example.

chia7712 · 2021-02-17T12:27:27Z

How about using topicError.exception("Topic " + topicName + " not found.")

dajac · 2021-03-01T09:04:06Z

@dajac Nice improvement. Some minor comments are left. Please take a look.

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java#L1893 also check the existence of topic. Will it become a redundant check after we merge this PR?

Should we apply this readable error message to other APIs? deleteTopics, for example.

@chia7712 Thanks for your comment. To answer your questions:

No, it is not. That one throws an exception if the topic was not returned at all by the broker.
That's a good question... I do agree that it is a bit weird to do this only in one case. Thinking about this, I think that it might be better to keep the error generic in the admin client and to handle it on the calling side instead. I will try this so we can compare.

…rror message when a topic does not exist, refactored

dajac · 2021-03-01T09:55:25Z

@chia7712 I just pushed an update. Please take a look and let me know what you think about it.

chia7712

overall +1

one small suggestion is left.

chia7712 · 2021-03-01T10:55:12Z

+          topicName -> topicDescriptionFuture.get
+        }
+        catch {
+          case t: ExecutionException if t.getCause != null =>


How about using classOf[UnknownTopicOrPartitionException].isInstance(t.getCause)? That includes null check.

dajac · 2021-03-01T11:18:59Z

@chia7712 Thanks. I just pushed a commit to address your comment. Could you take another look?

chia7712

+1 to latest commit :)

dajac · 2021-03-01T13:57:30Z

Failed test is not related. Merging to trunk.

ijuma · 2021-03-02T20:52:46Z

+        }
+        catch {
+          case t: ExecutionException =>
+            if (classOf[UnknownTopicOrPartitionException].isInstance(t.getCause)) {


Hmm. why not write this as t.getCause.isInstanceOf[UnknownTopicOrPartitionException]?

Also, you can have the if in the case t line and then a second case for the rethrow case.

I initially used t.getCause != null && t.getCause.isInstanceOf[UnknownTopicOrPartitionException]. @chia7712 suggested to use classOf[UnknownTopicOrPartitionException].isInstance(t.getCause) to avoid having to do the null check as isInstance does it. That seemed reasonable to me so I went with it. Is there a reason not to use it?

I do agree with your second comment.

Hmm, I think this is an example of code that is less readable. If cause may be nullable, it's better to write code that makes that clear rather than a non obvious alternative that could have been used for many other reasons (using Option to handle nullables is fine as a counter example since it's common usage to do that to handle nulls).

What do you think?

Oh, one more thing, did you check that the null check is actually needed? I think isInstanceOf is defined for null too.

I think isInstanceOf is defined for null too.

you are right. Scala does define it. https://scala-lang.org/files/archive/spec/2.13/spec.pdf

‘’’

isInstanceOf[T] always returns false.
‘’’

Good to know that :)

+1 to use ‘t.getCause.isInstanceOf[UnknownTopicOrPartitionException]‘ as it can deal with null check.

Sorry for my imprecise comment :(

@ijuma Yeah, I do agree with you. For the null check, I was not aware that isInstanceOf handles it. That's good to know, thanks.

I will open a small PR to fix this.

* apache-github/trunk: (37 commits) KAFKA-10357: Extract setup of changelog from Streams partition assignor (apache#10163) KAFKA-10251: increase timeout for consuming records (apache#10228) KAFKA-12394; Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission (apache#10223) MINOR: Disable transactional/idempotent system tests for Raft quorums (apache#10224) KAFKA-10766: Unit test cases for RocksDBRangeIterator (apache#9717) KAFKA-12289: Adding test cases for prefix scan in InMemoryKeyValueStore (apache#10052) KAFKA-12268: Implement task idling semantics via currentLag API (apache#10137) MINOR: Time and log producer state recovery phases (apache#10241) MINOR: correct the error message of validating uint32 (apache#10193) MINOR: Format the revoking active log output in `StreamsPartitionAssignor` (apache#10242) KAFKA-12323 Follow-up: Refactor the unit test a bit (apache#10205) MINOR: Remove stack trace of the lock exception in a debug log4j (apache#10231) MINOR: Word count should account for extra whitespaces between words (apache#10229) MINOR; Small refactor in `GroupMetadata` (apache#10236) KAFKA-10340: Proactively close producer when cancelling source tasks (apache#10016) KAFKA-12329; kafka-reassign-partitions command should give a better error message when a topic does not exist (apache#10141) KAFKA-12254: Ensure MM2 creates topics with source topic configs (apache#10217) MINOR: fix kafka-metadata-shell.sh (apache#10226) KAFKA-12374: Add missing config sasl.mechanism.controller.protocol (apache#10199) KAFKA-10101: Fix edge cases in Log.recoverLog and LogManager.loadLogs (apache#8812) ...

chia7712 reviewed Feb 17, 2021

View reviewed changes

KAFKA-12329; kafka-reassign-partitions command should give a better e…

e6a031a

…rror message when a topic does not exist, refactored

dajac force-pushed the minor-reassignment branch from 65c5f75 to e6a031a Compare March 1, 2021 09:54

dajac requested a review from chia7712 March 1, 2021 09:54

remove unused import

72defea

chia7712 approved these changes Mar 1, 2021

View reviewed changes

address comment

e71bcc1

chia7712 approved these changes Mar 1, 2021

View reviewed changes

dajac merged commit d78a923 into apache:trunk Mar 1, 2021

dajac deleted the minor-reassignment branch March 1, 2021 13:59

ijuma reviewed Mar 2, 2021

View reviewed changes

chia7712 mentioned this pull request Mar 14, 2021

MINOR: remove redundant null check when testing specified type #10314

Merged

3 tasks

Conversation

dajac commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac commented Mar 1, 2021

Uh oh!

dajac commented Mar 1, 2021

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac commented Mar 1, 2021

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

dajac commented Mar 1, 2021

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!

ijuma Mar 8, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dajac commented Feb 17, 2021 •

edited

Loading

ijuma Mar 8, 2021 •

edited

Loading