KAFKA-16095: Update list group state type filter to include the states for the new consumer group type by DL1231 · Pull Request #15211 · apache/kafka

DL1231 · 2024-01-17T10:56:45Z

While using —list —state the current accepted values correspond to the classic group type states.
This PR include support for the new group type states.

Committer Checklist (excluded from commit message)

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

…s for the new consumer group type

DL1231 · 2024-01-17T10:58:22Z

@rreddy-22 PTAL.

rreddy-22

Thanks for the PR! Left a few comments

DL1231 · 2024-01-22T08:01:08Z

@rreddy-22 Thanks for your comments. I addressed them.

rreddy-22

Thanks for the update! We're currently still straightening out the details on how to implement the case insensitivity for the group type filter. I'll get back to you asap once we figure that out!

dajac

@DL1231 Thanks for the PR. I left a few comments/suggestions.

DL1231 · 2024-01-25T15:34:03Z

Thanks for the suggestion @dajac . I have addressed the comments.

dajac

@DL1231 Thanks for the update! I left a few nits and some suggestions for considerations.

dajac · 2024-01-26T07:45:28Z

    }

-    ClassicGroupState(String name) {
+    ClassicGroupState(String name, String lowerCaseName) {


nit: I wonder if we could remove the lowerCaseName parameter and lower case the name in the constructor. Would it work?

dajac · 2024-01-26T07:46:21Z

        return name;
    }

+    public String toLowerCaseString() {


nit: I wonder if we should also add toLowerCaseString to ConsumerGroup.ConsumerGroupState in order to be consistent.

Actually, it would be great to do the same there. We could also capitatlize the states to be consistent too.

Thanks for the suggestion, I added the toCapitalizedString to ConsumerGroup.ConsumerGroupState.

dajac · 2024-01-26T07:47:09Z


+    @Override
+    public boolean isInStatesCaseInsensitive(List<String> statesFilter, long committedOffset) {
+        Set<String> caseInsensitiveFilterSet = statesFilter.stream().map(String::toLowerCase).map(String::trim).collect(Collectors.toSet());


It is a tad annoying to call toLowerCase here because we have to do it for every group. Would it be better to keep this part on the calling side?

dajac · 2024-01-26T07:48:11Z

+    /**
+     * @return The {{@link GroupType}}'s LowerCase String representation based on the committed offset.
+     */
+    String stateAsLowerCaseString(long committedOffset);


It seems that we only access it from within the class so we could remove it from the interface.

DL1231 · 2024-01-27T04:16:57Z

@dajac

Addressed your review comments. PTAL when you get chance!

dajac · 2024-01-28T07:24:11Z

@DL1231 Thanks. I’ll review it on Monday. I had a quick
look at the tests and there are related failures. Could you please check them?

dajac

@DL1231 Thanks for the update. I left a few minor comments. Please let me know what you think.

dajac · 2024-01-29T08:18:14Z

+
+    @Override
+    public boolean isInStates(Set<String> statesFilter, long committedOffset) {
+        return statesFilter.contains(this.stateAsLowerCaseString()) || statesFilter.contains(this.stateAsString());


I am not sure to understand why do we need to check both versions here? If the statesFilter are lowercased, the first one should be enough, no?

In my opinion, the statefilter cannot be guaranteed to be in lowercase, there might be that the method isinstates is called without doing the lowercase conversion, so needs to check for both versions here.

Yeah, it is not but in this context we know that it is. Could we address this by clarifying the requirement in the javadoc? My point is that if statesFilter is not lowercased, it won't work anyway because you could also received something like sTable.

dajac · 2024-01-29T08:18:35Z

+    public String stateAsLowerCaseString() {
+        return this.state.toLowerCaseString();
+    }


We could probably remove this one and inline it.

dajac · 2024-01-29T08:18:54Z

+        return statesFilter.contains(this.stateAsCapitalizedString(committedOffset)) || statesFilter.contains(
+            this.stateAsString(committedOffset));


Same question about checking the two versions here.

dajac · 2024-01-29T08:19:08Z

+    public String stateAsCapitalizedString(long committedOffset) {
+        return state.get(committedOffset).toCapitalizedString();
+    }


Same suggestion about inlining it.

dajac · 2024-01-29T08:20:45Z

  }

+  def isInStates(states: collection.Set[String]): Boolean = {
+    states.contains(state.toString) || states.contains(state.toLowerCaseString)


Same question.

dajac

LGTM, thanks @DL1231!

…s for the new consumer group type (apache#15211) While using —list —state the current accepted values correspond to the classic group type states. This patch adds the new states introduced by KIP-848. It also make the matching on the server case insensitive. Co-authored-by: d00791190 <dinglan6@huawei.com> Reviewers: Ritika Reddy <rreddy@confluent.io>, David Jacot <djacot@confluent.io>

KAFKA-16095: Update list group state type filter to include the state…

940304c

…s for the new consumer group type

rreddy-22 reviewed Jan 19, 2024

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/ConsumerGroupState.java Outdated

rreddy-22 reviewed Jan 19, 2024

View reviewed changes

Comment thread core/src/test/scala/unit/kafka/admin/ListConsumerGroupTest.scala

jolshan added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Jan 19, 2024

Addressing review comments

6d95654

rreddy-22 reviewed Jan 23, 2024

View reviewed changes

dajac mentioned this pull request Jan 24, 2024

KAFKA-16189; Extend admin to support ConsumerGroupDescribe API #15253

Merged

3 tasks

dajac reviewed Jan 24, 2024

View reviewed changes

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java Outdated

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java Outdated

Addressing review comments

b90303b

dajac reviewed Jan 26, 2024

View reviewed changes

Addressing review comments

3d7b806

d00791190 and others added 2 commits January 29, 2024 09:08

Merge remote-tracking branch 'origin/trunk' into KAFKA-16095

d99ad88

Old group coordinator list groups case insensitive

d7a8f77

dajac reviewed Jan 29, 2024

View reviewed changes

Addressed review comments

d1aa2f4

dajac approved these changes Jan 29, 2024

View reviewed changes

dajac merged commit 82920ff into apache:trunk Jan 29, 2024

nizhikov pushed a commit to nizhikov/kafka that referenced this pull request Jan 29, 2024

KAFKA-14589 Reflection changes from apache#15211

9ae4e2a

nizhikov pushed a commit to nizhikov/kafka that referenced this pull request Jan 29, 2024

KAFKA-14589 Reflection changes from apache#15211

0503189

		return statesFilter.contains(this.stateAsCapitalizedString(committedOffset)) \|\| statesFilter.contains(
		this.stateAsString(committedOffset));

Conversation

DL1231 commented Jan 17, 2024

Committer Checklist (excluded from commit message)

Uh oh!

DL1231 commented Jan 17, 2024

Uh oh!

rreddy-22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DL1231 commented Jan 22, 2024

Uh oh!

rreddy-22 left a comment

Choose a reason for hiding this comment

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DL1231 commented Jan 25, 2024

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DL1231 commented Jan 27, 2024

Uh oh!

dajac commented Jan 28, 2024

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers