KAFKA-18629: ShareGroupDeleteState admin client impl. #18928

smjn · 2025-02-17T11:32:43Z

In this PR, we add various infra classes needed to support the deleteShareGroups functionality via the kafka-share-groups.sh script.

AndrewJSchofield

Thanks for the PR.

I think the testing is a bit light. For example, --delete --all-topics where there is 0, 1 or multiple groups. That kind of thing.

AndrewJSchofield · 2025-02-18T10:31:08Z

tools/src/main/java/org/apache/kafka/tools/consumer/group/ShareGroupCommand.java

+            List<String> groupIds = opts.options.has(opts.allGroupsOpt)
+                ? listShareGroups()
+                : opts.options.valuesOf(opts.groupOpt);
+


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.

AndrewJSchofield · 2025-02-18T10:35:29Z

tools/src/test/java/org/apache/kafka/tools/consumer/group/ShareGroupCommandTest.java

+        String secondGroup = "second-group";
+        String bootstrapServer = "localhost:9092";
+
+        String[] cgcArgs = new String[]{"--bootstrap-server", bootstrapServer, "--delete"};


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.

AndrewJSchofield · 2025-02-18T10:41:37Z

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

+    DeleteShareGroupsResult(final Map<String, KafkaFuture<Void>> futures) {
+        super(futures);
+    }
+}


The implementation for the public methods here could be added.

As of now, it is exactly the same as the parent class.

Yes, OK. I suggest making a common superclass DeleteGroupsResult and having both DeleteConsumerGroupsResult and DeleteShareGroupsResult inheriting from that.

smjn · 2025-02-18T13:11:37Z

Thanks for the PR.

I think the testing is a bit light. For example, --delete --all-topics where there is 0, 1 or multiple groups. That kind of thing.

--all-topics is not related to delete group call. Is it?

AndrewJSchofield · 2025-02-21T12:33:28Z

Thanks for the PR.
I think the testing is a bit light. For example, --delete --all-topics where there is 0, 1 or multiple groups. That kind of thing.

--all-topics is not related to delete group call. Is it?

Sorry! I meant all-groups.

AndrewJSchofield

Thanks for the changes. A few more comments.

AndrewJSchofield · 2025-02-21T12:34:31Z

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

@@ -953,6 +953,14 @@ default ListConsumerGroupOffsetsResult listConsumerGroupOffsets(Map<String, List
     */
    DeleteConsumerGroupsResult deleteConsumerGroups(Collection<String> groupIds, DeleteConsumerGroupsOptions options);

+    /**


Personally, I'd prefer to see these two methods declared with the other share group methods, not mixed in with the consumer group methods.

AndrewJSchofield · 2025-02-21T12:36:56Z

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

+    DeleteShareGroupsResult(final Map<String, KafkaFuture<Void>> futures) {
+        super(futures);
+    }
+}


Yes, OK. I suggest making a common superclass DeleteGroupsResult and having both DeleteConsumerGroupsResult and DeleteShareGroupsResult inheriting from that.

AndrewJSchofield · 2025-02-21T12:38:34Z

clients/src/main/java/org/apache/kafka/clients/admin/internals/DeleteShareGroupsHandler.java

+
+import org.apache.kafka.common.utils.LogContext;
+
+public class DeleteShareGroupsHandler extends DeleteConsumerGroupsHandler {


Again, in a similar vein, maybe having a common DeleteGroupsHandler and having 2 subclasses would be sensible here. Having the share group handler inherit from the consumer group one is strange. I'm pretty sure that there will be an Admin.deleteGroups method before long too.

AndrewJSchofield · 2025-02-21T12:43:53Z

tools/src/main/java/org/apache/kafka/tools/consumer/group/ShareGroupCommand.java

+                : opts.options.valuesOf(opts.groupOpt);
+
+            if (groupIds.isEmpty()) {
+                throw new IllegalArgumentException("--groups or --all-groups argument is mandatory");


I would have said that the checking that there is either --groups or --all-groups should be done in the command line validation, not here.

As it stands, if the user specified --all-groups and there are no share groups, the error message is --groups or --all-groups is mandatory. Really, I would have said that this should be treated as successful deletion of no groups. That's what kafka-consumer-groups.sh does. It says "Deletion of requested share groups ('') was successful." Slight inelegant, but effective.

@AndrewJSchofield
It is already happening correctly. The util method getShareGroupService was not performing the validation check and hence I missed it and added it at the wrong place.
I will rectify.

AndrewJSchofield · 2025-02-21T12:46:02Z

tools/src/main/java/org/apache/kafka/tools/consumer/group/ShareGroupCommand.java

+                System.out.println("Deletion of requested share groups (" + "'" + success.keySet().stream().map(Object::toString).collect(Collectors.joining(", ")) + "'" + ") was successful.");
+            else {
+                printError("Deletion of some share groups failed:", Optional.empty());
+                failed.forEach((group, error) -> System.out.println("* Share Group '" + group + "' could not be deleted due to: " + error));


nit: Share group not Share Group please.

smjn · 2025-02-21T15:33:23Z

@AndrewJSchofield Thanks for the review
have incorporated comments.

AndrewJSchofield

Looking good now, with just one more area for improvement.

The output of the kafka-share-groups.sh --delete command in error cases is a bit untidy. Some examples:

bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group SG1 when the group is not empty, the exception stack trace is printed. The equivalent for bin/kafka-consumer-groups.sh is much neater.

bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group CG1 where CG1 is a consumer group prints an error message, and then prints the command usage message which is massive. Ideally, it would just say something like:

* Group 'CG1' could not be deleted due to: java.lang.IllegalStateException: Group CG1 is not a share group.

or something like that.

smjn · 2025-02-22T07:56:53Z

Looking good now, with just one more area for improvement.

The output of the kafka-share-groups.sh --delete command in error cases is a bit untidy. Some examples:

bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group SG1 when the group is not empty, the exception stack trace is printed. The equivalent for bin/kafka-consumer-groups.sh is much neater.

bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group CG1 where CG1 is a consumer group prints an error message, and then prints the command usage message which is massive. Ideally, it would just say something like:
* Group 'CG1' could not be deleted due to: java.lang.IllegalStateException: Group CG1 is not a share group.
or something like that.

@AndrewJSchofield Thanks for the review, incorporated comments.
Sample outputs

$ bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group gs1                      [13:23:20]

Error: Deletion of some share groups failed:
* Share group 'gs1' could not be deleted due to: java.lang.IllegalArgumentException: Group 'gs1' is not a share group.

$ bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group gs1 --group gs2          [13:23:23]

Error: Deletion of some share groups failed:
* Share group 'gs1' could not be deleted due to: java.lang.IllegalArgumentException: Group 'gs1' is not a share group.
* Share group 'gs2' could not be deleted due to: java.lang.IllegalArgumentException: Group 'gs2' is not a share group.

$ bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group gs1 --group gs2                     [13:23:19]

Error: Deletion of some share groups failed:
* Share group 'gs2' could not be deleted due to: org.apache.kafka.common.errors.GroupNotEmptyException: Share group 'gs2' is not EMPTY.

These share groups were deleted successfully: 'gs1'

$ bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --group gs1 --group gs2                     [13:25:38]

Deletion of requested share groups ('gs1', 'gs2') was successful.

$ bin/kafka-share-groups.sh --bootstrap-server localhost:9092 --delete --all-groups                                [13:25:40]

Deletion of requested share groups ('gs1', 'gs2') was successful.

AndrewJSchofield

Thanks for the updates. Looks good to me.

KAFKA-18629: ShareGroupDeleteState admin client impl.

56e2aa8

github-actions bot added triage PRs from the community streams clients labels Feb 17, 2025

AndrewJSchofield added the KIP-932 Queues for Kafka label Feb 17, 2025

additional infra classes.

0a6eb65

smjn marked this pull request as ready for review February 17, 2025 15:45

github-actions bot added the tools label Feb 17, 2025

smjn added 2 commits February 17, 2025 22:22

additional tests

bb0893c

Merge remote-tracking branch 'apache-kafka/trunk' into KAFKA-18629-4

24207cc

AndrewJSchofield added ci-approved and removed triage PRs from the community labels Feb 17, 2025

AndrewJSchofield self-requested a review February 17, 2025 17:00

AndrewJSchofield requested changes Feb 18, 2025

View reviewed changes

additional tests

4fd9afd

smjn requested a review from AndrewJSchofield February 18, 2025 13:32

smjn added 2 commits February 18, 2025 21:54

Do not delete share groups if they are non-empty.

ba96a6f

fixed messages

3d2c4b7

AndrewJSchofield requested changes Feb 21, 2025

View reviewed changes

incorporated comments

2da770d

conflict fix

81056ec

smjn requested a review from AndrewJSchofield February 21, 2025 15:36

smjn added 2 commits February 21, 2025 21:22

fix generic array creation

0afbffa

fixed javadoc

19d0439

AndrewJSchofield requested changes Feb 21, 2025

View reviewed changes

inc comments

a54a23b

smjn requested a review from AndrewJSchofield February 22, 2025 07:57

AndrewJSchofield approved these changes Feb 22, 2025

View reviewed changes

AndrewJSchofield merged commit 3fc103b into apache:trunk Feb 22, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-18629: ShareGroupDeleteState admin client impl. #18928

KAFKA-18629: ShareGroupDeleteState admin client impl. #18928

smjn commented Feb 17, 2025 •

edited

Loading

AndrewJSchofield left a comment

AndrewJSchofield Feb 18, 2025

AndrewJSchofield Feb 18, 2025

AndrewJSchofield Feb 18, 2025

smjn Feb 18, 2025

AndrewJSchofield Feb 21, 2025

smjn commented Feb 18, 2025

AndrewJSchofield commented Feb 21, 2025

AndrewJSchofield left a comment

AndrewJSchofield Feb 21, 2025

AndrewJSchofield Feb 21, 2025

AndrewJSchofield Feb 21, 2025

AndrewJSchofield Feb 21, 2025

smjn Feb 21, 2025

AndrewJSchofield Feb 21, 2025

smjn commented Feb 21, 2025

AndrewJSchofield left a comment

smjn commented Feb 22, 2025

AndrewJSchofield left a comment


		import org.apache.kafka.common.utils.LogContext;

		public class DeleteShareGroupsHandler extends DeleteConsumerGroupsHandler {

KAFKA-18629: ShareGroupDeleteState admin client impl. #18928

KAFKA-18629: ShareGroupDeleteState admin client impl. #18928

Conversation

smjn commented Feb 17, 2025 • edited Loading

AndrewJSchofield 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

smjn commented Feb 18, 2025

AndrewJSchofield commented Feb 21, 2025

AndrewJSchofield 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

smjn commented Feb 21, 2025

AndrewJSchofield left a comment

Choose a reason for hiding this comment

smjn commented Feb 22, 2025

AndrewJSchofield left a comment

Choose a reason for hiding this comment

smjn commented Feb 17, 2025 •

edited

Loading