KAFKA-10764: Add support for returning topic IDs on create, supplying topic IDs for delete#9684
Conversation
| } | ||
|
|
||
| /** | ||
| * Returns a future that provides number of partitions in the topic when the request completes. |
There was a problem hiding this comment.
I noticed all these comments say "If broker version doesn't support replication factor in the response..." should each actually say whatever the method is returning (numPartitions, topicId, etc.)
There was a problem hiding this comment.
In the create result, we wouldn't see UnsupportedVersionException for topicId() just because topic ids are not enabled, would we?
There was a problem hiding this comment.
I think we could either do that or just return Uuid.ZERO_UUID
There was a problem hiding this comment.
Returning Uuid.ZERO_UUID should be fine. The comment just seems odd, I am not sure why we have it that way in the other methods.
There was a problem hiding this comment.
I thought it was odd too.
| { "name": "TopicNames", "type": "[]string", "versions": "0+", "entityType": "topicName", | ||
| { "name": "Topics", "type": "[]DeleteTopicState", "versions": "6+", "about": "The name or topic ID of the topic", | ||
| "fields": [ | ||
| {"name": "Name", "type": "string", "versions": "6+", "nullableVersions": "6+", "about": "The topic name"}, |
There was a problem hiding this comment.
Why is topic name nullable but topicId not? do you mean we will fill all delete requests with a topicId before sending a request?
There was a problem hiding this comment.
We will use the zero UUID to represent null as described in the KIP.
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/DeleteTopicsRequest.java
Outdated
Show resolved
Hide resolved
|
I'm planning on adding one more test, but that test is blocked on #9814 |
core/src/test/scala/unit/kafka/server/DeleteTopicsRequestTest.scala
Outdated
Show resolved
Hide resolved
… version exception
rajinisivaram
left a comment
rajinisivaram
left a comment
There was a problem hiding this comment.
@jolshan Thanks for the PR, looks good. Left some minor comments. I didn't see an authorization test for delete topics where topic id is specified, but we are authorizing topic name. Can we add a test in AuthorizerIntegrationTest?
| } | ||
|
|
||
| /** | ||
| * Returns a future that provides number of partitions in the topic when the request completes. |
There was a problem hiding this comment.
In the create result, we wouldn't see UnsupportedVersionException for topicId() just because topic ids are not enabled, would we?
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/DeleteTopicsRequest.java
Outdated
Show resolved
Hide resolved
clients/src/main/resources/common/message/DeleteTopicsResponse.json
Outdated
Show resolved
Hide resolved
...t/scala/unit/kafka/server/MetadataRequestIBPTest/TopicIdWithOldInterBrokerProtocolTest.scala
Outdated
Show resolved
Hide resolved
…ed authorization logic
core/src/test/scala/unit/kafka/server/TopicIdWithOldInterBrokerProtocolTest.scala
Outdated
Show resolved
Hide resolved
rajinisivaram
left a comment
rajinisivaram
left a comment
There was a problem hiding this comment.
@jolshan Thanks for the PR, LGTM. Left just one minor comment.
|
The failing test FetcherTest.testEarlierOffsetResetArrivesLate is a known flaky test (https://issues.apache.org/jira/browse/KAFKA-12245). Merging to trunk. |
3 participants
Updated CreateTopicResponse, DeleteTopicsRequest/Response and added some new AdminClient methods and classes. Now the newly created topic ID will be returned in CreateTopicsResult and found in TopicAndMetadataConfig, and topics can be deleted by supplying topic IDs through deleteTopicsWithIds which will return DeleteTopicsWithIdsResult.
Committer Checklist (excluded from commit message)