KAFKA-15373: fix exception thrown in Admin#describeTopics for unknown ID

[MikeEdgar]

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.

Committer Checklist (excluded from commit message)

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

[MikeEdgar]

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.

[jolshan]

At first I was confused since the original PR had

kafka/core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala

Line 200 in 2a5fbf2

assertThrows(classOf[ExecutionException], () => results.get(nonExistingTopicId).get).getCause.isInstanceOf[UnknownTopicIdException]

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.

[jolshan]

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

kafka/core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala

Line 200 in 2a5fbf2

assertThrows(classOf[ExecutionException], () => results.get(nonExistingTopicId).get).getCause.isInstanceOf[UnknownTopicIdException]

[MikeEdgar]

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?

[jolshan]

@MikeEdgar that looks to be right. But please run the test to make sure it works correctly.

[MikeEdgar]

@jolshan , I've updated the test and it's passing, but the CI is failing on a seemingly unrelated test.

Gradle Test Run :storage:test > Gradle Test Executor 76 > TransactionsWithTieredStoreTest > testCommitTransactionTimeout(String) > testCommitTransactionTimeout(String).quorum=kraft FAILED

    org.apache.kafka.common.errors.TimeoutException: Timeout expired after 3000ms while awaiting InitProducerId

[jolshan]

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.

[jolshan]

Hey @MikeEdgar I'm seeing KafkaAdminClientTest.testDescribeTopicByIds failing due to it expecting the old error. Can we update that test?

[jolshan]

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.

[jolshan]

Thanks for the fix!