[KAFKA-7054] Kafka describe command should throw topic doesn't exist exception. - #5211
[KAFKA-7054] Kafka describe command should throw topic doesn't exist exception.#5211ManoharVanam wants to merge 1 commit into
Conversation
bd9f285 to
70de736
Compare
|
Thanks for the PR. The current implementation does not report anything when a non-existing topic is described. So reporting some message/error would be a good addition IMO. I think however a simple message would probably be better. The consumer group command reports a message like this when the group to describe does not exist: Perhaps something similar for the topic command? |
|
@vahidhashemian actually I checked alter & delete topic commands, In case of non-existing topic both returns below error. So for maintaining consistency, I added the same check for topic describe command. |
|
@ManoharVanam It makes sense. We are not consistent with respect to error handling across the command line tools. Could you please add some unit test for this improvement? Thanks! |
ManoharVanam
changed the title
|
@vahidhashemian Done. |
vahidhashemian
left a comment
vahidhashemian
left a comment
There was a problem hiding this comment.
LGTM, thanks for the PR.
|
retest this please |
|
\cc @hachikuji -- I am not familiar with this part of the code base. |
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Thanks for the patch. I agree we should be consistent. Left a minor suggestion.
|
@hachikuji Done. |
|
@hachikuji Moved common code to utils. |
e62142d to
c3e7d62
Compare
ff9baf7 to
2ef8dda
Compare
|
retest this please |
|
We need to update the --if-exists option description at line no:362 and valid check at line no:397 |
omkreddy
left a comment
omkreddy
left a comment
There was a problem hiding this comment.
@ManoharVanam Thanks for the PR. I just left couple of comments.
There was a problem hiding this comment.
Can we move this line to ensureTopicExists() method here? Same applies to other methods also
There was a problem hiding this comment.
@omkreddy Thanks for reviewing this PR, Currently describe command is not supporting --if-exists option so added the same as per your suggestion, but after adding this option we need to check both topic opt and if-exists option, so due to this change Im not moving this check to ensureTopicExists method.
There was a problem hiding this comment.
Can we also verify the behavior with --if--exists flag. similar to this
There was a problem hiding this comment.
Ok, added scenario with --if--exists flag
2ef8dda to
5d84c93
Compare
|
retest this please |
5d84c93 to
b416922
Compare
omkreddy
left a comment
omkreddy
left a comment
There was a problem hiding this comment.
@ManoharVanam Thanks for the PR. LGTM.
|
retest this please |
1 similar comment
|
retest this please |
…xception **User Interface Improvement :** If topic doesn't exist then Kafka describe command should throw topic doesn't exist exception, like alter and delete commands Author: Manohar Vanam <manohar.crazy09@gmail.com> Reviewers: Vahid Hashemian <vahid.hashemian@gmail.com>, Jason Gustafson <jason@confluent.io>, Manikumar Reddy <manikumar.reddy@gmail.com> Closes apache#5211 from ManoharVanam/KAFKA-7054
5 participants
User Interface Improvement : If topic doesn't exist then Kafka describe command should throw topic doesn't exist exception, like alter and delete commands
Committer Checklist (excluded from commit message)