-
Notifications
You must be signed in to change notification settings - Fork 14.9k
KAFKA-17897: Deprecate Admin.listConsumerGroups #19477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
chia7712
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndrewJSchofield thanks for this patch. a couple of comments are left. PTAL
clients/src/main/java/org/apache/kafka/clients/admin/Admin.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/Admin.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Returns the list of protocol types that are requested or empty if no protocol types have been specified. | ||
| */ | ||
| public Set<String> protocolTypes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there will be a follow-up to use this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add a few more tests. And update the KIP of course.
clients/src/main/java/org/apache/kafka/clients/admin/ListGroupsOptions.java
Show resolved
Hide resolved
| * <p> | ||
| * This is a convenience method for {@link #listConsumerGroups(ListConsumerGroupsOptions)} with default options. | ||
| * See the overload for more details. | ||
| * @deprecated Since 4.1. Use {@link Admin#listGroups(ListGroupsOptions)} instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use {@link Admin#listGroups()} instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. If you want Admin.listGroups() to return just consumer groups, you need to apply a filter to group type and protocol type.
| * Only consumer groups will be returned by listGroups(). | ||
| * This operation sets filters on group type and protocol type which select consumer groups. | ||
| */ | ||
| public ListGroupsOptions forConsumerGroups() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new API is not in KIP-1043, therefore its code style is probably open for discussion 😃
Given that this API appears to be a helper function for creating a specific query, perhaps we could refactor it as a static method.
public static ListGroupsOptions forConsumerGroups() {
return new ListGroupsOptions()
.withTypes(Set.of(GroupType.CLASSIC, GroupType.CONSUMER))
.withProtocolTypes(Set.of("", ConsumerProtocol.PROTOCOL_TYPE));
}and then the code new ListGroupsOptions().forConsumerGroups() can be replaced by ListGroupsOptions.forConsumerGroups()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. I was trying to see what the best way to express this would be and I like the static method suggestion.
FrankYang0529
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ConfigCommand still uses listConsumerGroups. Do we want to update it to listGroups?
kafka/core/src/main/scala/kafka/admin/ConfigCommand.scala
Lines 352 to 353 in 67fa365
| case GroupType => | |
| adminClient.listConsumerGroups().all.get.asScala.map(_.groupId).toSeq |
|
@chia7712 I actually discovered a problem with the code running against older brokers. |
The final part of KIP-1043 is to deprecate Admin.listConsumerGroups() in favour of Admin.listGroups() which works for all group types. Reviewers: PoAn Yang <[email protected]>, Chia-Ping Tsai <[email protected]>
The final part of KIP-1043 is to deprecate Admin.listConsumerGroups() in
favour of Admin.listGroups() which works for all group types.
Reviewers: PoAn Yang [email protected], Chia-Ping Tsai
[email protected]