dajac commented on code in PR #15211:
URL: https://github.com/apache/kafka/pull/15211#discussion_r1464905968
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -462,7 +463,9 @@ public Group group(String groupId, long committedOffset)
throws GroupIdNotFoundE
public List<ListGroupsResponseData.ListedGroup> listGroups(List<String>
statesFilter, long committedOffset) {
Stream<Group> groupStream = groups.values(committedOffset).stream();
if (!statesFilter.isEmpty()) {
- groupStream = groupStream.filter(group ->
statesFilter.contains(group.stateAsString(committedOffset)));
+ List<String> caseInsensitiveFilterList =
statesFilter.stream().map(String::toLowerCase).collect(Collectors.toList());
Review Comment:
It would be better to use a Set here.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -462,7 +463,9 @@ public Group group(String groupId, long committedOffset)
throws GroupIdNotFoundE
public List<ListGroupsResponseData.ListedGroup> listGroups(List<String>
statesFilter, long committedOffset) {
Stream<Group> groupStream = groups.values(committedOffset).stream();
if (!statesFilter.isEmpty()) {
- groupStream = groupStream.filter(group ->
statesFilter.contains(group.stateAsString(committedOffset)));
+ List<String> caseInsensitiveFilterList =
statesFilter.stream().map(String::toLowerCase).collect(Collectors.toList());
+ groupStream = groupStream.filter(group ->
caseInsensitiveFilterList.contains(group.stateAsString(committedOffset).toLowerCase(
Review Comment:
It is a tad annoying that we have to lower case the state for every group,
every time this method is called. Here I wonder if we could store the lower
cased version of the state in the enum in order to avoid it.
The second think that I wanted to point out is that it may be worth adding a
method like `inState(Set<String> states, long committedOffset)` to the `Group`
interface in order to delegate the implementation of the filter to the group
itself. I think that something like this would be required to implement my
initial suggestion.
What do you think?
--
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
