AndrewJSchofield commented on code in PR #18928:
URL: https://github.com/apache/kafka/pull/18928#discussion_r1959472888
##########
tools/src/main/java/org/apache/kafka/tools/consumer/group/ShareGroupCommand.java:
##########
@@ -208,6 +210,50 @@ public void describeGroups() throws ExecutionException,
InterruptedException {
}
}
+ Map<String, Throwable> deleteShareGroups() {
+ List<String> groupIds = opts.options.has(opts.allGroupsOpt)
+ ? listShareGroups()
+ : opts.options.valuesOf(opts.groupOpt);
+
Review Comment:
Because `deleteShareGroups` actually deletes all types of group, I think it
would be prudent for this method to check that the groups being deleted are
share groups.
In the `--all-groups` case, you're listing the share groups. If you listed
the share groups in the other case too, you'd easily be able to see whether the
groups are share groups and then avoid deleting consumer groups unawares. Do
you think this would be a sensible precaution? It's quite a limited code change.
##########
tools/src/test/java/org/apache/kafka/tools/consumer/group/ShareGroupCommandTest.java:
##########
@@ -588,6 +591,94 @@ public void testGroupStatesFromString() {
assertThrows(IllegalArgumentException.class, () ->
ShareGroupCommand.groupStatesFromString(" , ,"));
}
+ @Test
+ public void testDeleteShareGroupsSuccess() {
+ String firstGroup = "first-group";
+ String secondGroup = "second-group";
+ String bootstrapServer = "localhost:9092";
+
+ String[] cgcArgs = new String[]{"--bootstrap-server", bootstrapServer,
"--delete"};
Review Comment:
I would have expected the command line parsing to fail because `cgcArgs`
doesn't include either `--group` or `--all-groups`. Similar comment for the
other tests.
##########
clients/src/main/java/org/apache/kafka/clients/admin/DeleteShareGroupsResult.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.clients.admin;
+
+import org.apache.kafka.common.KafkaFuture;
+import org.apache.kafka.common.annotation.InterfaceStability;
+
+import java.util.Collection;
+import java.util.Map;
+
+/**
+ * The result of the {@link Admin#deleteShareGroups(Collection <String>,
DeleteShareGroupsOptions)} call.
+ * <p>
+ * The API of this class is evolving, see {@link Admin} for details.
+ */
+@InterfaceStability.Evolving
+public class DeleteShareGroupsResult extends DeleteConsumerGroupsResult {
+ DeleteShareGroupsResult(final Map<String, KafkaFuture<Void>> futures) {
+ super(futures);
+ }
+}
Review Comment:
The implementation for the public methods here could be added.
--
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
