KAFKA-15373: fix exception thrown in Admin#describeTopics for unknown ID#14599
Conversation
|
Hi @jolshan , please take a look at this PR to modify the exception thrown when describing a topic by an unknown topic ID. The CI failures don't appear related to the change. |
e661f60 to
579afd0
Compare
579afd0 to
7ce410c
Compare
|
At first I was confused since the original PR had but on closer inspection (the debugger) I see that the future is actually InvalidTopicException and that this test is wrong. I'm trying to think if there was any rationale for this decision, but after spending 20 minutes or so looking through my notes and old messages I couldn't find one. The only other concern would be compatibility, but given that the unknown topic ID error is being returned by the server, but the Cluster API doesn't know how to handle it I think this change makes sense. @ijuma do you have any concerns from the compatibility angle? Given this is an admin API, I don't think changing this error should break anything in a crazy way. |
|
I synced with @ijuma offline. I think it makes sense to return the UnknownTopicId exception since that is what we do for the deleteTopics api and what the server is using. It is a bit annoying that we can't use the topicError directly and we convert to the cluster object that loses all the detail about the topic IDs and their error responses. But fixing that requires a larger refactor, For now let's just fix |
|
Thanks for the feedback @jolshan . If I'm following your comments on the broken test, the assertion assertThrows(classOf[ExecutionException], () => results.get(nonExistingTopicId).get).getCause.isInstanceOf[UnknownTopicIdException] must be replaced with: assertFutureExceptionTypeEquals(results.get(nonExistingTopicId), classOf[UnknownTopicIdException])Correct? |
|
@MikeEdgar that looks to be right. But please run the test to make sure it works correctly. |
7ce410c to
fd1bb21
Compare
|
@jolshan , I've updated the test and it's passing, but the CI is failing on a seemingly unrelated test. |
|
Sorry for the delay @MikeEdgar. That is a known issue for the test. I will also look at this PR and run the build again. |
|
Hey @MikeEdgar I'm seeing KafkaAdminClientTest.testDescribeTopicByIds failing due to it expecting the old error. Can we update that test? |
Throw UnknownTopicIdException instead of InvalidTopicException Signed-off-by: Michael Edgar <medgar@redhat.com>
Signed-off-by: Michael Edgar <medgar@redhat.com>
Signed-off-by: Michael Edgar <medgar@redhat.com>
|
Thanks @MikeEdgar looks like the test is fixed. I'll run the build again to make sure there aren't any other related tests failing. |
… 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>
… 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>
… 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>
Throw
UnknownTopicIdExceptioninstead ofInvalidTopicExceptionwhen no name is found for the topic ID.Similar to #6124 for
describeTopicsusing a topic name.MockAdminClientalready makes use ofUnknownTopicIdExceptionfor this case.Committer Checklist (excluded from commit message)