jolshan commented on a change in pull request #10892: URL: https://github.com/apache/kafka/pull/10892#discussion_r659995458
########## File path: clients/src/main/java/org/apache/kafka/common/TopicCollection.java ########## @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.common; + +import java.util.Collection; + +/** + * A class used to represent a collection of topics. This collection may define topics by topic name + * attribute or topic ID attribute. Subclassing this class beyond the classes provided here is not supported. + */ +public abstract class TopicCollection { + private final TopicAttribute attribute; + + /** + * An enum used to describe how topics in the collection are identified + */ + public enum TopicAttribute { Review comment: I kept this TopicAttribute field for now, though we could case on the class (TopicIdCollection/TopicNameCollection) One reason I didn't remove it is that the result class(es) still needs it. (Since we are building off the existing class). If there is a way to remove from the result class, or a better place to put the attribute, let me know. ########## File path: clients/src/main/java/org/apache/kafka/clients/admin/DeleteTopicsResult.java ########## @@ -30,24 +31,62 @@ */ @InterfaceStability.Evolving public class DeleteTopicsResult { - final Map<String, KafkaFuture<Void>> futures; + private Map<String, KafkaFuture<Void>> nameFutures; + private Map<Uuid, KafkaFuture<Void>> topicIdFutures; - protected DeleteTopicsResult(Map<String, KafkaFuture<Void>> futures) { - this.futures = futures; + protected DeleteTopicsResult() {} Review comment: So the idea is that we have two parameters and set one as null in the static methods? ########## File path: clients/src/main/java/org/apache/kafka/clients/admin/DeleteTopicsResult.java ########## @@ -30,24 +31,62 @@ */ @InterfaceStability.Evolving public class DeleteTopicsResult { - final Map<String, KafkaFuture<Void>> futures; + private Map<String, KafkaFuture<Void>> nameFutures; + private Map<Uuid, KafkaFuture<Void>> topicIdFutures; - protected DeleteTopicsResult(Map<String, KafkaFuture<Void>> futures) { - this.futures = futures; + protected DeleteTopicsResult() {} + + private void setNameFutures(Map<String, KafkaFuture<Void>> nameFutures) { + this.nameFutures = nameFutures; + } + + private void setTopicIdFutures(Map<Uuid, KafkaFuture<Void>> topicIdFutures) { + this.topicIdFutures = topicIdFutures; + } + + protected static DeleteTopicsResult ofTopicNames(Map<String, KafkaFuture<Void>> nameFutures) { + DeleteTopicsResult result = new DeleteTopicsResult(); + result.setNameFutures(nameFutures); + return result; + } + + protected static DeleteTopicsResult ofTopicIds(Map<Uuid, KafkaFuture<Void>> topicIdFutures) { + DeleteTopicsResult result = new DeleteTopicsResult(); + result.setTopicIdFutures(topicIdFutures); + return result; + } + + /** + * Return a map from topic names to futures which can be used to check the status of + * individual deletions if the deleteTopics request used topic names. Otherwise return null. Review comment: I was following the convention from this class before, but I can update. ########## File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java ########## @@ -1688,15 +1691,27 @@ void handleFailure(Throwable throwable) { } @Override - public DeleteTopicsResult deleteTopics(final Collection<String> topicNames, + public DeleteTopicsResult deleteTopics(final TopicCollection topics, final DeleteTopicsOptions options) { + DeleteTopicsResult result; + if (topics instanceof TopicIdCollection) + result = DeleteTopicsResult.ofTopicIds(new HashMap<>(handleDeleteTopicsUsingIds(((TopicIdCollection) topics).topicIds(), options))); + else if (topics instanceof TopicNameCollection) + result = DeleteTopicsResult.ofTopicNames(new HashMap<>(handleDeleteTopicsUsingNames(((TopicNameCollection) topics).topicNames(), options))); + else + throw new UnsupportedOperationException("The TopicCollection provided did not match any supported classes for deleteTopics."); Review comment: I was trying to remember the name of that error. I agree. ########## File path: clients/src/main/java/org/apache/kafka/common/TopicCollection.java ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.common; + +import java.util.Collection; + +/** + * 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 { Review comment: Static factories in addition to a public constructor? Or make the constructor private? ########## File path: clients/src/main/java/org/apache/kafka/common/TopicCollection.java ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.common; + +import java.util.Collection; + +/** + * 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 { Review comment: I think it's simpler with just the factories, so I like this idea. ########## File path: clients/src/main/java/org/apache/kafka/clients/admin/DeleteTopicsResult.java ########## @@ -30,24 +31,62 @@ */ @InterfaceStability.Evolving public class DeleteTopicsResult { - final Map<String, KafkaFuture<Void>> futures; + private Map<String, KafkaFuture<Void>> nameFutures; + private Map<Uuid, KafkaFuture<Void>> topicIdFutures; - protected DeleteTopicsResult(Map<String, KafkaFuture<Void>> futures) { - this.futures = futures; + protected DeleteTopicsResult() {} Review comment: Ah I remember the issue with that being private. There is a test that extends this class. > @hachikuji I originally wanted the constructor for DeleteTopicsResult to be private, but InternalTopicManagerTest required creating a subclass. https://github.com/apache/kafka/pull/10892#issuecomment-862876282 The issue with just using the static method is that it is protected in the class. So the options are to either make the class protected (as is now), make the static method public, or change the test. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
