KAFKA-13011: Update deleteTopics Admin API #10892

jolshan · 2021-06-17T02:43:40Z

Added two new apis to support deleteTopics using topic IDs or names. Added a new class TopicCollection to keep a collection of topics defined either by names or IDs. Modified DeleteTopicsResult to support both names and IDs and deprecated the old methods. Eventually we will want to deprecate the old deleteTopics apis as well.

Tested using the existing deleteTopics tests. Also created unit tests for new/updated classes.

Committer Checklist (excluded from commit message)

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

jolshan · 2021-06-17T02:53:16Z

@hachikuji I originally wanted the constructor for DeleteTopicsResult to be private, but InternalTopicManagerTest required creating a subclass.

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

clients/src/main/java/org/apache/kafka/clients/admin/TopicCollection.java

hachikuji · 2021-06-22T17:26:45Z

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

     */
-    DeleteTopicsWithIdsResult deleteTopicsWithIds(Collection<Uuid> topics, DeleteTopicsOptions options);
+    DeleteTopicsResult deleteTopics(TopicCollection topics, DeleteTopicsOptions options);


Since TopicCollection can handle both "by name" and "by ID" deletion, is the intent to eventually deprecate the old deletion api or will we keep it around for convenience?

Eventually we want to deprecate the old api yes

clients/src/main/java/org/apache/kafka/common/TopicCollection.java

…ting to get the type that doesn't match the request

clients/src/main/java/org/apache/kafka/clients/admin/DeleteTopicsResult.java

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

clients/src/main/java/org/apache/kafka/common/TopicCollection.java

hachikuji · 2021-06-28T22:43:01Z

clients/src/main/java/org/apache/kafka/common/TopicCollection.java

+ * A class used to represent a collection of topics. This collection may define topics by topic name
+ * or topic ID. Subclassing this class beyond the classes provided here is not supported.
+ */
+public abstract class TopicCollection {


Would it be helpful to have a couple static factories? For example:

public static TopicIdCollection ofTopicIds(Collection<Uuid> uuids); public static TopicNameCollection ofTopicNames(Collection<String> names);

Static factories in addition to a public constructor? Or make the constructor private?

No strong opinion, but I'd probably vote to keep the constructors private. Might be worth getting a second opinion. @ijuma ?

I think it's simpler with just the factories, so I like this idea.

clients/src/main/java/org/apache/kafka/common/TopicCollection.java

hachikuji · 2021-06-30T01:02:05Z

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

                                           final DeleteTopicsOptions options) {
+        DeleteTopicsResult result;
+        if (topics instanceof TopicIdCollection)
+            result = DeleteTopicsResult.ofTopicIds(new HashMap<>(handleDeleteTopicsUsingIds(((TopicIdCollection) topics).topicIds(), options)));


Why do we copy the result of handleDeleteTopicsUsingIds? Seems like that method is already returning a fresh map.

This was an artifact of the old version of the code. We would create a copy of the map

return new DeleteTopicsResult(new HashMap<>(topicFutures));

I can remove though.

Hmmm. When I try to remove I get
'ofTopicIds(java.util.Map<org.apache.kafka.common.Uuid,org.apache.kafka.common.KafkaFuture<java.lang.Void>>)' in 'org.apache.kafka.clients.admin.DeleteTopicsResult' cannot be applied to '(java.util.Map<org.apache.kafka.common.Uuid,org.apache.kafka.common.internals.KafkaFutureImpl<java.lang.Void>>)'

Many other admin client calls also follow this pattern with using KafkaFutureImpl and copying the result to get KafkaFutures

I can move this cast to the handle... call and return a KafkaFuture if it makes things cleaner.

clients/src/test/java/org/apache/kafka/clients/admin/DeleteTopicsResultTest.java

clients/src/test/java/org/apache/kafka/clients/admin/TopicCollectionTest.java

hachikuji · 2021-06-30T01:10:34Z

clients/src/main/java/org/apache/kafka/clients/admin/DeleteTopicsResult.java

    }

+    protected static DeleteTopicsResult ofTopicIds(Map<Uuid, KafkaFuture<Void>> topicIdFutures) {


Do these methods need to be protected or could they be package access?

Forgive my ignorance but is that not the same thing? Is package private just that we can't use in subclasses outside the package? If that's the case then sure.

Yes, that's right. The less exposed it is, the better.

clients/src/main/java/org/apache/kafka/clients/admin/DeleteTopicsResult.java

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

clients/src/main/java/org/apache/kafka/common/TopicCollection.java

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

...ams/src/test/java/org/apache/kafka/streams/processor/internals/InternalTopicManagerTest.java

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

hachikuji · 2021-06-30T23:15:55Z

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

        final Map<Uuid, KafkaFutureImpl<Void>> topicFutures = new HashMap<>(topicIds.size());
        final List<Uuid> validTopicIds = new ArrayList<>(topicIds.size());
        for (Uuid topicId : topicIds) {
            if (topicId.equals(Uuid.ZERO_UUID)) {
                KafkaFutureImpl<Void> future = new KafkaFutureImpl<>();
                future.completeExceptionally(new UnknownTopicIdException("The given topic ID '" +
-                        topicId + "' cannot be represented in a request."));
+                    topicId + "' cannot be represented in a request."));


This message is a little strange. We can certainly represent the topic id, but it is invalid. I wonder if it would make sense to raise IllegalArgumentException directly instead of through the result since this is likely a logical error of some kind.

Ah yeah. The name equivalent is InvalidTopicException so maybe we could just use that (or IllegalArugmentException is probably better) so the cases are similar? I think that we should keep it as through the result so it is consistent with the name equivalent, but that's just me.

Hmm, I like InvalidTopicException over IllegalArgumentException if we are raising it through the future. Typically IllegalArgumentException is raised directly.

Ok. We can change to that.

hachikuji

Thanks for the patch! LGTM

ijuma · 2021-07-01T22:10:09Z

clients/src/main/java/org/apache/kafka/clients/admin/Admin.java

     * with default options. See the overload for more details.
     * <p>
-     * This operation is supported by brokers with version 2.8.0 or higher.
+     * When using topic IDs, this operation is supported by brokers with version 3.0.0 or higher.


This does require a specific inter-broker protocol version?

Ah, it will need the controller to use topic IDs (IBP 2.8) to get a successful deletion. The 3.0.0 part was for the code to handle the request itself.

Ah actually, now that I think of it, the broker code was added in 2.8, so maybe this should just say 2.8

OK, I suggest submitting a follow-up PR with the improvements. If a particular IBP is required, we should state that too.

Sure. I'm thinking the change is to make the comment clearer about the version and IBP.

https://github.com/apache/kafka/pull/10966/files

This patch adds two new apis to support topic deletion using topic IDs or names. It uses a new class `TopicCollection` to keep a collection of topics defined either by names or IDs. Finally, it modifies `DeleteTopicsResult` to support both names and IDs and deprecates the old methods which have become ambiguous. Eventually we will want to deprecate the old `deleteTopics` apis as well, but this patch does not do so. Reviewers: Jason Gustafson <jason@confluent.io>

jolshan marked this pull request as draft June 17, 2021 02:46

New Admin API for deleteTopics

3c414f2

jolshan force-pushed the deleteApi branch from 0af76a6 to 3c414f2 Compare June 17, 2021 02:50

hachikuji reviewed Jun 22, 2021

View reviewed changes

Change TopicCollection to have two sublasses for names and IDs

d33b542

jolshan commented Jun 28, 2021

View reviewed changes

clients/src/main/java/org/apache/kafka/common/TopicCollection.java Outdated Show resolved Hide resolved

Removed attribute, will return null for DeleteTopicResult when attemp…

638e5ce

…ting to get the type that doesn't match the request

hachikuji reviewed Jun 28, 2021

View reviewed changes

Static factories + delete constructor changes

3789871

jolshan marked this pull request as ready for review June 29, 2021 17:32

jolshan mentioned this pull request Jun 29, 2021

KAFKA-10774; Support Describe topic using topic IDs #9769

Merged

3 tasks

jolshan changed the title ~~[WIP] New Admin API for deleteTopics~~ KAFKA-13011: Update deleteTopics Admin API Jun 29, 2021

Fix issues with unmodifiable maps

37a442a

hachikuji reviewed Jun 30, 2021

View reviewed changes

More fixes, cleaning code, clarifying comments

d94dd6b

hachikuji reviewed Jun 30, 2021

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Show resolved Hide resolved

hachikuji reviewed Jun 30, 2021

View reviewed changes

Spacing and comment fixes

0ea59b3

hachikuji reviewed Jun 30, 2021

View reviewed changes

jolshan added 2 commits June 30, 2021 16:31

More comments

39f0cba

Changed error message

efd90cc

jolshan force-pushed the deleteApi branch from cc1333b to efd90cc Compare June 30, 2021 23:54

hachikuji approved these changes Jul 1, 2021

View reviewed changes

hachikuji merged commit cee2e97 into apache:trunk Jul 1, 2021

ijuma reviewed Jul 1, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-13011: Update deleteTopics Admin API #10892

KAFKA-13011: Update deleteTopics Admin API #10892

jolshan commented Jun 17, 2021 •

edited

jolshan commented Jun 17, 2021

hachikuji Jun 22, 2021

jolshan Jun 22, 2021

hachikuji Jun 28, 2021

jolshan Jun 28, 2021

hachikuji Jun 28, 2021

jolshan Jun 29, 2021

hachikuji Jun 30, 2021

jolshan Jun 30, 2021

jolshan Jun 30, 2021

jolshan Jun 30, 2021 •

edited

jolshan Jun 30, 2021

hachikuji Jun 30, 2021

jolshan Jun 30, 2021

hachikuji Jun 30, 2021

hachikuji Jun 30, 2021

jolshan Jun 30, 2021

hachikuji Jun 30, 2021

jolshan Jun 30, 2021

hachikuji left a comment

ijuma Jul 1, 2021

jolshan Jul 1, 2021

jolshan Jul 2, 2021

ijuma Jul 2, 2021

jolshan Jul 2, 2021

jolshan Jul 3, 2021

		}

		protected static DeleteTopicsResult ofTopicIds(Map<Uuid, KafkaFuture<Void>> topicIdFutures) {

KAFKA-13011: Update deleteTopics Admin API #10892

KAFKA-13011: Update deleteTopics Admin API #10892

Conversation

jolshan commented Jun 17, 2021 • edited

Committer Checklist (excluded from commit message)

jolshan commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolshan Jun 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hachikuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolshan commented Jun 17, 2021 •

edited

jolshan Jun 30, 2021 •

edited