Thanks, Andrew. LGTM. On Mon, Oct 28, 2024 at 3:58 PM Andrew Schofield < andrew_schofield_j...@outlook.com> wrote:
> Hi David, > I've updated the KIP accordingly. GROUP_ID_NOT_FOUND and an > error message is used to disambiguate the error cases when the group > ID cannot be found. > > Thanks for your time reviewing this KIP. > > Andrew > > ________________________________________ > From: David Jacot <dja...@confluent.io.INVALID> > Sent: 28 October 2024 13:37 > To: dev@kafka.apache.org <dev@kafka.apache.org> > Subject: Re: [DISCUSS] KIP-1043: Administration of groups > > Hi Andrew, > > Thanks for your response. I think that your last proposal makes sense to > me. It seems that we have no choice but to also change the DescribeGroups > API. However, I still have a preference for the GROUP_ID_NOT_FOUND > approach. It is simpler in my opinion and consistent across all APIs. From > an UX perspective, I don't really see the value of > the INCONSISTENT_GROUP_TYPE error too. In the end, if someone looks up a > share group called X but X is not a share group, the share group X does not > exist / is not found. The new error would be useful if users would want to > handle it to request the correct type but I don't believe that it is a > relevant use case. > > DJ12: Perfect, thanks. > > Best, > David > > On Tue, Oct 22, 2024 at 3:46 PM Andrew Schofield < > andrew_schofield_j...@outlook.com> wrote: > > > Hi David, > > Thanks for your response. > > > > I really don't like an API response whose error message is > > a significant part of the interface. If we have code that checks > > for specific error messages and takes different actions, we've > > got it wrong. If we are just displaying the error text or adding > > it to an exception, that's fine. > > > > I think the tricky part here is that a "consumer group" could > > actually be a modern consumer group or a classic consumer > > group. > > > > How about this? > > > > The DescribeGroups API is used to describe classic groups only. > > A new version of the response (v6) adds an error message. It > > would be very weird to populate the error message for an > > error code of NONE, so v6 makes two changes: > > a) If the group ID exists but it's not a classic group, the error code > > INCONSISTENT_GROUP_TYPE is returned along with an error message > > such as "Group %s is not a classic group". > > b) If the group ID does not exist, the error code > > GROUP_ID_NOT_FOUND is returned along with an error message > > such as "Group %s not found". Formerly, this used to return > > error code NONE and bogus dead group. > > > > Admin#describeConsumerGroups first uses the > > ConsumerGroupDescribe API: > > a) If the group ID is a modern consumer group, the error code > > NONE is returned along with the group description. > > b) If the group ID exists but it's not a modern consumer group, > > the error code INCONSISTENT_GROUP_TYPE is returned > > along with an error message such as "Group %s is not a consumer > > group". > > c) If the group ID does not exist, the error code GROUP_ID_NOT_FOUND > > is returned along with an error message such as > > "Group %s not found". > > > > In case (b), the admin client then uses the DescribeGroups API > > to see whether the inconsistent group is a classic group or not. > > The bogus dead group is no longer used in the admin client interface. > > If the group doesn't exist, it's an exception. > > > > Admin#describeShareGroups uses the ShareGroupDescribe API: > > a) If the group ID is a share group, the error code NONE is returned > > along with the group description. > > b) If the group ID exists but it's not a share group, the error code > > INCONSISTENT_GROUP_TYPE is returned along with an error > > message such as "Group %s is not a share group". > > c) If the group ID does not exist, the error code GROUP_ID_NOT_FOUND > > is returned along with an error message such as "Group %s not found". > > > > Admin#describeClassicGroups uses the DescribeGroups v6 API: > > a) If the group ID is a classic group, the error code NONE is returned > > along with the group description. > > b) If the group ID exists but it's not a classic group, the error code > > INCONSISTENT_GROUP_TYPE is returned along with an error message > > such as "Group %s is not a classic group". > > c) If the group ID does not exist, the error code GROUP_ID_NOT_FOUND > > is returned along with an error message such as "Group %s not found". > > There is no bogus dead group. > > > > > > Having run through these scenarios, they would work almost the same > > if GROUP_ID_NOT_FOUND was used in the place of INCONSISTENT_GROUP_TYPE. > > Personally, I prefer the separate error code and consequently exception > > because you don't have to look inside the error message to figure out > > what went wrong. But actually, making it all use GROUP_ID_NOT_FOUND would > > give the same experience to the operator using the command line tools. > > > > If you're still convinced that GROUP_ID_NOT_FOUND is preferable, I'll > > change the KIP to remove the new error code. > > > > > > DL12: I will deprecate Admin#listConsumerGroups in this KIP, > > and remove Admin#listShareGroups from KIP-932, along with > > related tidying up that results from this. > > > > Thanks, > > Andrew > > > > ________________________________________ > > From: David Jacot <dja...@confluent.io.INVALID> > > Sent: 22 October 2024 11:08 > > To: dev@kafka.apache.org <dev@kafka.apache.org> > > Subject: Re: [DISCUSS] KIP-1043: Administration of groups > > > > Hi Andrew, > > > > Please excuse me for my delayed response. Thanks for the update. > Overall, I > > am fine with the KIP, except for the INCONSISTENT_GROUP_TYPE error. > > > > We have three methods to describe groups: > > 1) Admin#describeClassicGroup (added by the KIP) > > 2) Admin#describeConsumerGroup > > 3) Admin#describeShareGroup > > > > With your proposal: > > 1) returns a group in a dead state if the group does not exist and if the > > group has the wrong type. This is the current behavior. > > 2.1) if the group is a classic group, ConsumerGroupDescribe returns > > INCONSISTENT_GROUP_TYPE, so we call DescribeGroup which returns the group > > or an empty group if it does exist. > > 2.2) if the group is a share group, ConsumerGroupDescribe returns > > INCONSISTENT_GROUP_TYPE, so we call DescribeGroup which returns an empty > > group in the dead state. We must call DescribeGroup because we don't know > > the correct type so we must check. > > 3) returns INCONSISTENT_GROUP_TYPE if the group has the wrong type. > > > > Hence new INCONSISTENT_GROUP_TYPE error is propagated to the user only > with > > Admin#describeShareGroup. Admin#describeConsumerGroup never returns it > > based on my current understanding. It does not seem that useful in the > > current state and more importantly it is inconsistent. > > > > At minimum, we should ensure that the new error code is used everywhere, > > even by the DescribeGroups API, in order to be consistent. Note that it > is > > still a bit weird to return INCONSISTENT_GROUP_TYPE because it would be > the > > only error returned. I am not really happy with how we designed this API > in > > the first place. Alternatively, we could also extend the DescribeGroup > API > > to return GROUP_ID_NOT_FOUND with an error message. It would achieve the > > same end result while keeping an existing error code used by all the > other > > APIs for this purpose. Personally, I prefer the latter. > > > > Regarding DJ12, I think that we should deprecate them in this KIP because > > they are deprecated because of it. It does not make sense to do another > KIP > > just to deprecate them in my opinion. > > > > Best, > > David > > > > On Wed, Oct 2, 2024 at 6:21 PM Andrew Schofield < > > andrew_schofield_j...@outlook.com> wrote: > > > > > Hi David, > > > Thanks for your replies. > > > > > > DJ2: OK. > > > > > > DJ3: I've added AdminClient.describeClassicGroups to the KIP. I added > > > ClassicGroupDescription and ClassicGroupState too. I did consider using > > > GroupState directly, but all of the other XXXGroupDescription have > > > their own specific XXXGroupState so I went for consistency. > > > > > > DJ6: I'll try again to justify the new INCONSISTENT_GROUP_TYPE error > > code. > > > Consider the RPCs and errors codes when trying to describe a consumer > > > group and actually the group ID is taken by a share group. > > > > > > ConsumerGroupDescribe returns error code GROUP_ID_NOT_FOUND (69). > > > This means the group ID is not a modern consumer group, but we don't > know > > > whether it's a classic consumer group. > > > > > > DescribeGroups never uses GROUP_ID_NOT_FOUND. Instead, it just fakes up > > > a (classic) consumer group with a state of Dead and returns OK. Because > > > this > > > is an ancient RPC response, it does not currently include an > > ErrorMessage. > > > > > > Admin.describeConsumerGroups uses these two RPCs and it cannot > currently > > > work out when a group ID exists and has the wrong group type. > > > > > > I think the cleanest way to handle this is to have XXXGroupDescribe > > return > > > a new error code, INCONSISTENT_GROUP_TYPE. I personally think that > > > just adding it to the XXXGroupDescribe responses is sufficient. It's > > never > > > in any doubt what the situation is with any other RPCs. > > > > > > I'm happy to consider alternatives if you have any in mind, but I've > > > wrestled with this for a while and I think this suggestion is sound. > > > > > > DJ7: I've removed --describe from kafka-topics.sh and made --list > output > > a > > > table. > > > > > > DJ10: Although it slightly offends me, I have added > isSimpleConsumerGroup > > > to GroupListing. This method is now present in GroupListing, > > > ConsumerGroupDescription and ClassicGroupDescription. I think > > > that will do the job. A simple consumer group is GroupType.CLASSIC > > > with an empty protocol, and you can use Admin.describeConsumerGroups > > > or Admin.describeClassicGroups to describe it. > > > > > > DJ12: I will propose a new KIP that deprecates > > > Admin.listXXXGroups and related classes, and also deprecates > > > kafka-xxx-groups.sh --list. > > > > > > Thanks, > > > Andrew > > > > > > ________________________________________ > > > From: David Jacot <dja...@confluent.io.INVALID> > > > Sent: 02 October 2024 08:54 > > > To: dev@kafka.apache.org <dev@kafka.apache.org> > > > Subject: Re: [DISCUSS] KIP-1043: Administration of groups > > > > > > Hi Andrew, > > > > > > Thanks for your replies. > > > > > > DJ1: Thanks for clarifying. Somehow, I was sure that the tool would at > > > minimum list those groups. > > > > > > DJ2: Oh, I see. Actually, removing it would break the admin client as > > > listConsumerGroups rely on it to filter groups. I think that we should > > just > > > keep it as it is. > > > > > > DJ3: AdminClient.describeClassicGroups sounds good to me. > > > > > > DJ4: Ack. > > > > > > DJ5: Ack. > > > > > > DJ6: I am not sure that I understand the issue that you're trying to > > solve > > > with the new error code. Could you please elaborate? Is the goal to > avoid > > > the extra lookup if the group really does not exist? I agree that > adding > > > the new code everywhere is a lot of work and this is why I did not > > > introduce it in the first place :). Personally, I don't like the > > > inconsistency between the APIs. It will be confusing. Hence I would do > it > > > everywhere or nowhere but this is just my opinion. > > > > > > DJ7: In my opinion, we could just keep --list and always return a > table. > > > For context, we did not change it to a table in kafka-consumer-groups > > when > > > we added the new filter for backward compatibility reasons. In this > case, > > > it is a new tool so we could do it. > > > > > > DJ8: OK. > > > > > > DJ10: That's a good question. We have not really thought through how we > > > will handle those in the future. I don't like the Simple group type > > because > > > we don't have it on the server. The reason why I wanted to add > > > isSimpleConsumerGroup is because it would allow us to deprecate > > > listConsumerGroups. In the end, isSimpleConsumerGroup would only be > true > > > for classic groups without a protocol type. > > > > > > DJ11: OK. > > > > > > DJ12: I also prefer option (a). I think that we should avoid having a > > list > > > method per type that we add. > > > > > > Best, > > > David > > > > > > On Mon, Sep 23, 2024 at 6:39 PM Andrew Schofield < > > > andrew_schofi...@live.com> > > > wrote: > > > > > > > Hi David, > > > > Thanks for your comments. > > > > > > > > DJ1: If I start the Confluent schema registry, it creates a classic > > group > > > > called "sr" using > > > > the protocol "sr" which is not included in the list from > > > > kafka-consumer-groups.sh. > > > > If I describe it using kafka-consumer-groups.sh, it says "Error: > > Consumer > > > > group 'sr' does not exist". > > > > > > > > If I start a distributed Kafka Connect worker, it creates a classic > > group > > > > called "connect-cluster" > > > > using the protocol "connect". Neither is this group included in the > > list > > > > from > > > > kafka-consumer-groups.sh. If I describe it using > > > kafka-consumer-groups.sh, > > > > it spews out > > > > an exception which contains "GroupId connect-cluster is not a > consumer > > > > group(connect)". > > > > Quite why this appears to take a different error path is a mystery. > > > > > > > > So, I think that your understanding is not quite correct. It's worse. > > > This > > > > KIP is all about > > > > tidying this up and not making it worse as we add share groups and > > > streams > > > > groups. > > > > > > > > DJ2: I tend to agree about setting the protocol only for classic > > groups, > > > > but the ListGroups RPC > > > > already exists (this KIP does not change it) and we already do set > the > > > > protocol type. For example, > > > > a modern consumer group uses "consumer" and a share group uses > > "share". I > > > > could omit the > > > > protocol type in the output from kafka-groups.sh without changing the > > > RPC. > > > > To remove it from > > > > the RPC would likely necessitate a version bump on ListGroups. Which > do > > > > you prefer here? > > > > Do you agree about my assertion about the version bump? It's probably > > > > removing a piece of > > > > information which everyone ignores. > > > > > > > > DJ3: Yes, I think that AdminClient.describeClassicGroups would be > > > > appropriate, but > > > > AdminClient.describeGroups would need to be able to return > information > > > > about all different > > > > kinds of groups, and I suggest that's not necessary. If you agree, I > > can > > > > add it. > > > > > > > > DJ4: See DJ12 option (a) below. Essentially, if we take this option, > > > maybe > > > > we don't introduce > > > > AbstractListGroupsResult at all because there would be no subclasses > in > > > > the future, and its > > > > privacy becomes moot. > > > > > > > > DJ5: We could just have a single constructor. I have changed the KIP. > > > > > > > > DJ6: I think we need to be careful here not to bloat the amount of > work > > > > unnecessarily. > > > > There's quite a cost to adding a new RPC version, conditionally > > returning > > > > a new error code, > > > > and adding tests to ensure that the old and new behaviour is correct. > > If > > > > we do this for about > > > > 10 RPCs, that's quite a lot of work with little benefit. Would anyone > > > > actually use LeaveGroup > > > > against a modern consumer group? With the KIP as written, the new > error > > > > code is used for > > > > RPCs which describe groups only. > > > > > > > > I could understand adding it to Consumer|ShareGroupHeartbeat too, > but I > > > > don't think it's > > > > necessary. This is because these RPC responses are modern and have > > > > ErrorMessage which > > > > can easily be used to populate the exception reason in the > > application. I > > > > think that > > > > GroupIdNotFoundException if a consumer application attempts to join a > > > > group of the wrong > > > > kind is OK if the exception also contains the string "Group XYZ is > not > > a > > > > consumer group". > > > > An InconsistentGroupTypeException is probably a little better, but > only > > > > marginally I think. > > > > > > > > ConsumerGroupDescribe is different. Because of the co-existence of > > > classic > > > > and modern > > > > consumer groups, and the way that the admin client drops through when > > it > > > > receives a > > > > GROUP_ID_NOT_FOUND error code from ConsumerGroupDescribe and then > tries > > > > DescribeGroups to check for a classic group instead, I think we need > a > > > new > > > > error code > > > > on the ConsumerGroupDescribe. > > > > > > > > Ultimately, if you're convinced that the new error code should be > > present > > > > for all RPCs served > > > > by the GC which are dependent on group type, I can change the KIP. I > > was > > > > careful to write > > > > the KIP in this way, but it's ultimately just my opinion. > > > > > > > > DJ7: In general, the tools use --list to return just a bare list of > > > > resource names as strings and > > > > then --describe to return a table of the resources. So, I've gone for > > > > consistency with > > > > kafka-topics.sh, kafka-consumer-groups.sh and others. I do note that > > > > specifically > > > > "kafka-consumer-groups.sh --list --state" returns a table. I think > that > > > > one slipped through the > > > > cracks. > > > > > > > > DJ8: Hmm. I think I see what you mean by deprecating > > > > kafka-consumer-groups.sh --list. > > > > Essentially, it would encourage users to use kafka-groups.sh to list > > > > groups, and then only > > > > use kafka-consumer-groups.sh for operating on consumer groups. I > wonder > > > if > > > > that would > > > > make a sensible and trivial follow-on KIP because we would make it > > simple > > > > to gauge > > > > opinions on deprecating an option which people are used to. > > > > > > > > DJ9: Yes, you are correct. I have updated the KIP accordingly. > > > > > > > > DJ10: Do you have an opinion about the future of simple consumer > > groups? > > > I > > > > imagine that > > > > classic consumer groups will disappear in AK 5.0, but it will still > be > > > > possible to have a > > > > consumer which just commits offsets and does not engage in the > consumer > > > > group protocol. > > > > I'm not convinced that isSimpleConsumerGroup should be in > > GroupListing. > > > > Maybe a group > > > > type of Simple would be better. What do you think? > > > > > > > > DJ11: It probably is slightly more convenient but I am a big fan of > > > > consistency. > > > > AdminClient.listConsumerGroups uses ListConsumerGroupsOptions for > > > > filtering the types > > > > and states of groups returned. AdminClient.listTopics uses > > > > ListTopicsOptions for the same > > > > purpose. This is why I prefer AdminClient.listGroups to use > > > > ListGroupsOptions for filtering. > > > > > > > > DJ12: I think there are two options here: > > > > > > > > Option (a): AdminClient.listGroups becomes the one true method for > > > listing > > > > groups. It returns > > > > instances of GroupListing, and GroupListing is sufficiently general > to > > > > handle all types of groups. > > > > We introduce a new enum GroupState which is essentially the union of > > > > ConsumerGroupState > > > > and ShareGroupState. We deprecate AdminClient.listConsumerGroups and > > > > AdminClient.listShareGroups, and deprecate ConsumerGroupListing and > > > > ShareGroupListing. > > > > There would be no need to worry about the code duplication in > > > > ListConsumerGroupsResult, > > > > ListShareGroupsResult and ListGroupsResult because only the last of > > these > > > > would not be > > > > deprecated. As a result, introducing AbstractListGroupsResult now > would > > > > not be worth the effort. > > > > We cannot deprecate ConsumerGroupState and ShareGroupState because > they > > > > are used by > > > > AdminClient.describeConsumerGroups/describeShareGroups. We remove the > > > > deprecated > > > > interfaces in AK 5.0. > > > > > > > > Option (b): We keep AdminClient.listConsumerGroup/listShareGroups, in > > > > addition to > > > > AdminClient.listGroups. We do not deprecate ConsumerGroupListing and > > > > ShareGroupListing. > > > > > > > > Personally, I prefer option (a) but I wasn't convinced it would be a > > > > viable option. I wonder what you think. > > > > > > > > > > > > Thanks, > > > > Andrew > > > > > > > > > > > > ________________________________________ > > > > From: David Jacot <dja...@confluent.io.INVALID> > > > > Sent: 23 September 2024 13:57 > > > > To: dev@kafka.apache.org <dev@kafka.apache.org> > > > > Subject: Re: [DISCUSS] KIP-1043: Administration of groups > > > > > > > > Hi Andrew, > > > > > > > > Thanks for the KIP. I have some comments/questions: > > > > > > > > DJ1: In the motivation, you wrote the following: "You can’t see this > > > group > > > > in the list of consumer groups with the kafka-consumer-groups.sh > tool, > > > but > > > > if you try to describe a consumer group called "connect-cluster" or > > even > > > > use this group ID with a consumer, you get an error.". My > understanding > > > is > > > > that you can list those groups but you cannot describe them. Is my > > > > understanding correct? > > > > > > > > DJ2: Regarding the information returned by the ListGroups RPC, I > wonder > > > if > > > > we should set the protocol only for classic groups. The protocol is > > > > actually a reference to the protocol type used in the classic > > protocol. I > > > > suppose that it is also fine to return the group type for the > > non-classic > > > > groups. > > > > > > > > DJ3: I wonder if we should also introduce Admin#describeGroups or > > > > Admin#describeClassicGroup. At the moment, it is impossible to > describe > > > > classic groups which do not use the consumer protocol. What do you > > think? > > > > > > > > DJ4: Would it be possible to keep AbstractListGroupsResult private > > > somehow? > > > > I don't like to publish it as part of our public APIs because we have > > to > > > > maintain it over time. > > > > > > > > DJ5: Could we limit the number of constructors in GroupListing? I > > suppose > > > > that having only one should be fine. > > > > > > > > DJ6: I think that we should extend all the APIs to > > > > use INCONSISTENT_GROUP_TYPE if we introduce a new error code. For > > > instance, > > > > it would make sense for the DescribeGroup to follow the same pattern. > > > > Moreover, if we have it, it would also make sense for > > > > ConsumerGroupHearbeat, ShareGroupHeartbeat and > > > > JoinGroup/SyncGroup/Heartbeat to also use it in order to have a > > > consistent > > > > error across the board. What do you think? > > > > > > > > DJ7: I would remove the "--describe" from the new tool. We could just > > > make > > > > "--list" return the same information by default. "--describe" is used > > by > > > > other command line tools to describe resources so it may be confusing > > for > > > > users. > > > > > > > > DJ8: Should we deprecate "--list" in the kafka-consumer-group command > > > line > > > > tool? > > > > > > > > DJ9: ListOffsets RPC is not related to groups. I suppose that you > means > > > > OffsetFetch/OffsetCommit. > > > > > > > > DJ10: Should we add isSimpleConsumerGroup to GroupListing? It is > also a > > > > sort of group type. > > > > > > > > DJ11: What do you think about having listGroups(GroupType), > > > > listGroups(Set<GroupType>) and listGroups() instead of passing types > > via > > > > the options? It may be more convenient for users. > > > > > > > > DJ12: I wonder if we should deprecate ConsumerGroupListing > > > > and ShareGroupListing and their associate methods. If we have the > > above, > > > we > > > > don't need them anymore. What's your take on this? > > > > > > > > Best, > > > > David > > > > > > > > On Fri, Sep 13, 2024 at 10:53 AM Andrew Schofield < > > > > andrew_schofield_j...@outlook.com> wrote: > > > > > > > > > Hi Lucas, > > > > > Thanks for the review. I've updated the KIP with the suggestions > > > > > and will open up a vote soon. > > > > > > > > > > Thanks, > > > > > Andrew > > > > > > > > > > ________________________________________ > > > > > From: Lucas Brutschy <lbruts...@confluent.io.INVALID> > > > > > Sent: 11 September 2024 15:17 > > > > > To: dev@kafka.apache.org <dev@kafka.apache.org> > > > > > Subject: Re: [DISCUSS] KIP-1043: Administration of groups > > > > > > > > > > Hi Andrew, > > > > > > > > > > thanks for the KIP! > > > > > > > > > > It is looking good from my side! I like the simplification, and > that > > > > > we added the new error but only of the Describe RPCs. It's a good > > > > > pragmatic improvement of the current state of things. > > > > > > > > > > I only have very minor comments: > > > > > - nit: In `GroupListing`, you seem to import `ShareGroupState` and > > > > > it's not clear why. > > > > > - The documentation for `--consumer` in the table is not enough. > We > > > > > should make sure that the comment below the table is also included > in > > > > > the command-line help of the CLI tool -- I was confused by this at > > > > > first. Possibly just explain it in terms of the equivalent sequence > > of > > > > > commands. > > > > > > > > > > From my point of view, this is ready for a vote. > > > > > > > > > > Cheers, > > > > > Lucas > > > > > > > > > > > > > > > > > > > > On Tue, Sep 3, 2024 at 2:56 PM Andrew Schofield > > > > > <andrew_schofield_j...@outlook.com> wrote: > > > > > > > > > > > > Hi, > > > > > > I’ve spent some time working with clusters containing groups of > > > > multiple > > > > > > types, fixing problems and improving error handling. > > > > > > > > > > > > I’ve simplified the KIP so that it just adds kafka-groups.sh and > > > > improves > > > > > > the error handling for describing groups of the wrong type. With > > the > > > > > other > > > > > > improvements I’ve already made, it seems to me that this is > > > sufficient > > > > to > > > > > > make working with groups of multiple types work nicely. > > > > > > > > > > > > I’d like to ask for another round of reviews before hopefully > > opening > > > > up > > > > > > a vote soon. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1043%3A+Administration+of+groups > > > > > > > > > > > > Thanks, > > > > > > Andrew > > > > > > > > > > > > ________________________________________ > > > > > > From: Andrew Schofield <andrew_schofield_j...@live.com> > > > > > > Sent: 02 August 2024 15:00 > > > > > > To: dev@kafka.apache.org <dev@kafka.apache.org> > > > > > > Subject: Re: [DISCUSS] KIP-1043: Administration of groups > > > > > > > > > > > > Hi Lianet, > > > > > > Thanks for your comment. > > > > > > > > > > > > I’ve been digging more into the situation with describing groups > > in a > > > > > > broker with groups of multiple types. It’s a bit fiddly because > of > > > the > > > > > > introduction of the modern consumer groups by KIP-848 and the > > > > > > need for the admin client to cope with both kinds of consumer > > groups > > > > > > and older brokers. > > > > > > > > > > > > If you use `kafka-consumer-groups.sh --describe --group MYSHARE` > > > > > > the output is: > > > > > > > > > > > > Error: Consumer group ‘MYSHARE’ does not exist. > > > > > > > > > > > > How does it get there? AdminClient.describeConsumerGroups > > > > > > is complicated. > > > > > > > > > > > > First, it uses the ConsumerGroupDescribe RPC which responds > > > > > > with GROUP_ID_NOT_FOUND (69) and an empty error message. > > > > > > The broker *could* fill in the error message to help with this > > > > situation > > > > > > but I don’t like that as a solution. Seems quite brittle. > > > > > > > > > > > > Then, it uses the DescribeGroups RPC in case it’s a classic > > consumer > > > > > > group. This responds with error code NONE (0) and makes the group > > > > > > look like a Dead consumer group. There is no error message field > > > > > > in that RPC at all, so we don’t have the option of using an error > > > > > > message to disambiguate. > > > > > > > > > > > > So, `kafka-consumer-groups.sh` thinks that it’s dealing with a > dead > > > > > > consumer group and its output makes sense. > > > > > > > > > > > > My preferred course of action here is as you suggest to introduce > > > > > > the new error code, INVALID_GROUP_TYPE. If you use any of the > > > following > > > > > > RPCs with the wrong type of group, you get this response: > > > > > > > > > > > > * ConsumerGroupDescribe > > > > > > * ShareGroupDescribe > > > > > > * ConsumerGroupHeartbeat > > > > > > * ShareGroupHeartbeat > > > > > > > > > > > > The remaining RPCs for consumer groups, such as ListOffsets and > > > > > > TxnOffsetCommit continue to use `GROUP_ID_NOT_FOUND`. > > > > > > > > > > > > Does that make sense? Any further comments? > > > > > > > > > > > > Thanks, > > > > > > Andrew > > > > > > > > > > > > > On 23 Jul 2024, at 17:26, Lianet M. <liane...@gmail.com> > wrote: > > > > > > > > > > > > > > Hello Andrew, > > > > > > > > > > > > > > Bringing here the point I surfaced on the KIP-1071 thread: > > > > > > > > > > > > > > I wonder if at this point, where we're getting several new > group > > > > types > > > > > > >> added, each with RPCs that are supposed to include groupId of > a > > > > > certain > > > > > > >> type, we should be more explicit about this situation. Maybe a > > > kind > > > > of > > > > > > >> INVALID_GROUP_TYPE (group exists but not with a valid type for > > > this > > > > > RPC) vs > > > > > > >> a GROUP_ID_NOT_FOUND (group does not exist). Those errors > would > > > be > > > > > > >> consistently used across consumer, share, and streams RPCs > > > whenever > > > > > the > > > > > > >> group id is not of the expected type. > > > > > > > > > > > > > > > > > > > > > I noticed it on KIP-1071 but totally agree with you that it > would > > > > make > > > > > more > > > > > > > sense to consider it here. > > > > > > > > > > > > > > LM9. Regarding the point of introducing a new > INVALID_GROUP_TYPE > > vs > > > > > reusing > > > > > > > the existing INCONSISTENT_PROTOCOL_TYPE. My concern with > reusing > > > > > > > INCONSISTENT_GROUP_PROTOCOL for errors with the group ID is > that > > it > > > > > mixes > > > > > > > the concepts of group type and protocol. Even though they are > > > closely > > > > > > > related, we have 2 separate concepts (internally and presented > in > > > > > output > > > > > > > for commands), and the relationship is not 1-1 in all cases. > > Also, > > > > the > > > > > > > INCONSISTENT_GROUP_PROTOCOL is already used not only for > protocol > > > but > > > > > also > > > > > > > when validating the list of assignors provided by a consumer > in a > > > > > > > JoinGroupRequest. Seems a bit confusing to me already, so maybe > > > > better > > > > > not > > > > > > > to add more to it? Just first thoughts. What do you think? > > > > > > > > > > > > > > Thanks, > > > > > > > Lianet > > > > > > > > > > > > > > On Fri, Jul 19, 2024 at 5:00 AM Andrew Schofield < > > > > > andrew_schofi...@live.com> > > > > > > > wrote: > > > > > > > > > > > > > >> Hi Apoorv, > > > > > > >> Thanks for your comments. > > > > > > >> > > > > > > >> AM1: I chose to leave the majority of the administration for > the > > > > > different > > > > > > >> types of groups in their own tools. The differences between > the > > > > group > > > > > > >> types are significant and I think that one uber tool that > > subsumes > > > > > > >> kafka-consumer-groups.sh, kafka-share-groups.sh and > > > > > > >> kafka-streams-application-reset.sh would be too overwhelming > and > > > > > > >> difficult to use. For example, the output from describing a > > > consumer > > > > > group > > > > > > >> is not the same as the output from describing a share group. > > > > > > >> > > > > > > >> AM2: I think you’re highlighting some of the effects of the > > > > evolution > > > > > > >> of groups. The classic consumer group protocol defined the > idea > > > > > > >> of protocol as a way of distinguishing between the various > ways > > > > people > > > > > > >> had extended the base protocol - “consumer", “connect", and > “sr" > > > are > > > > > the > > > > > > >> main ones I’ve seen, and the special “” for groups that are > not > > > > using > > > > > > >> member assignment. > > > > > > >> > > > > > > >> For the modern group protocol, each of the proposed > > > implementations > > > > > > >> brings its own use of the protocol string - “consumer”, > “share” > > > and > > > > > > >> “streams”. > > > > > > >> > > > > > > >> Now, prior to AK 4.0, in order to make the console consumer > use > > > the > > > > > > >> new group protocol, you set `--consumer-property > > > > > group.protocol=consumer`. > > > > > > >> This tells a factory method in the consumer to use the > > > > > AsyncKafkaConsumer > > > > > > >> (group type is Consumer, protocol is “consumer") as opposed to > > the > > > > > > >> LegacyKafkaConsumer (group type is Classic, protocol is > > > “consumer”). > > > > > > >> In AK 4.0, the default group protocol will change and setting > > the > > > > > property > > > > > > >> will not be necessary. The name of the configuration > > > > “group.protocol” > > > > > > >> is slightly misleading. In practice, this is most likely to be > > > used > > > > > pre-AK > > > > > > >> 4.0 > > > > > > >> by people wanting to try out the new consumer. > > > > > > >> > > > > > > >> AM3: When you try to create a share group and that group ID is > > > > already > > > > > > >> in use by another type of group, the error message is “Group > CG1 > > > is > > > > > not > > > > > > >> a share group”. It exists already, with the wrong type. > > > > > > >> > > > > > > >> AM4: This KIP changes the error behaviour for > > > > > `kafka-consumer-groups.sh` > > > > > > >> and `kafka-share-groups.sh` such that any operation on a group > > > that > > > > > finds > > > > > > >> the > > > > > > >> group type is incorrect reports “Error: Group XXX is not a > > > consumer > > > > > group” > > > > > > >> or > > > > > > >> equivalent for the other group types. This change makes things > > > much > > > > > easier > > > > > > >> to > > > > > > >> understand than they are today. > > > > > > >> > > > > > > >> AM5: That section is just clarifying what the behaviour is. I > > > don’t > > > > > think > > > > > > >> it had > > > > > > >> been written down before. > > > > > > >> > > > > > > >> Thanks, > > > > > > >> Andrew > > > > > > >> > > > > > > >>> On 18 Jul 2024, at 16:43, Apoorv Mittal < > > > apoorvmitta...@gmail.com> > > > > > > >> wrote: > > > > > > >>> > > > > > > >>> Hi Andrew, > > > > > > >>> Thanks for the KIP. The group administration is getting > > difficult > > > > > with > > > > > > >> new > > > > > > >>> types of groups being added and certainly the proposal looks > > > great. > > > > > > >>> I have some questions: > > > > > > >>> > > > > > > >>> AM1: The current proposal defines the behaviour for listing > and > > > > > > >> describing > > > > > > >>> groups, simplifying create for `kafka-share-groups.sh`. Do we > > > want > > > > to > > > > > > >> leave > > > > > > >>> the other group administration like delete to respective > groups > > > or > > > > > shall > > > > > > >>> have common behaviour defined i.e. leave to respective > > > > > > >>> kafka-consumer-groups.sh or kafka-share-groups.sh? > > > > > > >>> > > > > > > >>> AM2: Reading the notes on KIP-848, > > > > > > >>> > > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/The+Next+Generation+of+the+Consumer+Rebalance+Protocol+%28KIP-848%29+-+Early+Access+Release+Notes > > > > > > >> , > > > > > > >>> which requires `--consumer-property group.protocol=consumer` > to > > > > > enable > > > > > > >>> modern consumer group. But the listing for `classic` "type" > > also > > > > > defines > > > > > > >>> "protocol" as `consumer` in some scenarios. Is it intended or > > > > > `classic` > > > > > > >>> type should different protocol? > > > > > > >>> > > > > > > >>> AM3: The KIP adds behaviour for `kafka-share-groups.sh` > which > > > > > defines > > > > > > >> the > > > > > > >>> `--create` option. Though it simplifies group creation, what > > > should > > > > > be > > > > > > >> the > > > > > > >>> error behaviour when the group with the same name exists but > > not > > > of > > > > > > >> "share" > > > > > > >>> group type? > > > > > > >>> > > > > > > >>> AM4: The GroupMetadataManager.java stores all groups in the > > same > > > > data > > > > > > >>> structure which means the name has to be unique across > > different > > > > > group > > > > > > >>> types. Do you think we should also change the error message > for > > > > > existing > > > > > > >>> kafka-consumer-groups.sh and kafka-share-groups.sh to > recommend > > > > > using new > > > > > > >>> kafka-groups.sh for extensive groups list? As currently the > > > > > individual > > > > > > >>> scripts will result in "Group already exists" while creating > > new > > > > > groups > > > > > > >> but > > > > > > >>> listing with respective utility will not yield the group. > > > > > > >>> > > > > > > >>> AM5: The KIP defines compatibility considerations for the > > > > ListGroups > > > > > RPC. > > > > > > >>> But it's unclear to me why it's needed as the client and > server > > > > > > >> supporting > > > > > > >>> `kafka-groups.sh` will be post ListGroups v5 API anyways > hence > > > > > > >> TypesFilter > > > > > > >>> will be supported in both client and server. Am I missing > > > something > > > > > here? > > > > > > >>> > > > > > > >>> Regards, > > > > > > >>> Apoorv Mittal > > > > > > >>> +44 7721681581 > > > > > > >>> > > > > > > >>> > > > > > > >>> On Wed, Jul 17, 2024 at 6:26 PM Andrew Schofield < > > > > > > >> andrew_schofi...@live.com> > > > > > > >>> wrote: > > > > > > >>> > > > > > > >>>> Hi Lianet, > > > > > > >>>> Thanks for your comments. > > > > > > >>>> > > > > > > >>>> LM5. Unfortunately, the protocol type has to be a string > > rather > > > > than > > > > > > >>>> an enumeration. This is because when people have created > their > > > own > > > > > > >>>> extensions of the classic consumer group protocol, they have > > > > chosen > > > > > > >>>> their own protocol strings. For example, the Confluent > schema > > > > > registry > > > > > > >>>> uses “sr” and there are other examples in the wild. > > > > > > >>>> > > > > > > >>>> LM6.1. It’s because of the difference between a > parameterised > > > > > > >>>> type and a raw type. > > > > > > >>>> > > > > > > >>>> If I use: > > > > > > >>>> public class ListGroupsResult<T extends GroupListing> > > > > > > >>>> public class ListConsumerGroupsResult<ConsumerGroupListing> > > > > > > >>>> > > > > > > >>>> then ListGroupsResult (no type variable) is a raw type which > > > does > > > > > > >>>> not provide a type for the type variable. This causes > compiler > > > > > warnings > > > > > > >>>> when the type is used, unless it’s used as > > > > > > >> ListGroupsResult<GroupListing>. > > > > > > >>>> > > > > > > >>>> However, this works better. > > > > > > >>>> public class AbstractListGroupsResult<T extends > GroupListing> > > > > > > >>>> public class ListGroupsResult extends > > > > > > >>>> AbstractListGroupsResult<GroupListing> > > > > > > >>>> public class ListConsumerGroupsResult extends > > > > > > >>>> AbstractListGroupsResult<ConsumerGroupListing> > > > > > > >>>> > > > > > > >>>> I’ll change the KIP to use this. > > > > > > >>>> > > > > > > >>>> LM6.2. I was just pointing out a difference and you’re happy > > > > > > >>>> with it. That’s good. > > > > > > >>>> > > > > > > >>>> LM7. If you have a cluster with a mixture of classic and > > modern > > > > > > >>>> consumer groups, to list them all you could use this: > > > > > > >>>> > > > > > > >>>> bin/kafka-groups.sh --protocol consumer > > > > > > >>>> > > > > > > >>>> When there are no classic consumer groups, you could do: > > > > > > >>>> > > > > > > >>>> bin/kafka-groups.sh --group-type consumer > > > > > > >>>> > > > > > > >>>> However, this only gives a complete list if you don’t have > any > > > > > classic > > > > > > >>>> consumer groups. > > > > > > >>>> > > > > > > >>>> As a result, I suggested --consumer so you don’t need to > know > > > > > > >>>> or care about the existence of classic and modern consumer > > > groups. > > > > > > >>>> I think it helps, but you aren’t convinced I think, which > > tells > > > me > > > > > > >>>> more thinking needed here. > > > > > > >>>> > > > > > > >>>> Maybe adding --share would help, so only power users would > > > > > > >>>> use --group-type or --protocol to deal with the more > > complicated > > > > > > >>>> cases. > > > > > > >>>> > > > > > > >>>> LM8. It’s just not clear. I was trying to make the output > the > > > same > > > > > > >>>> whether the group was created, or whether it already > existed. > > In > > > > > > >>>> either case, there’s a share group in existence. The > > > > > > >>>> AlterShareGroupOffsets RPC doesn’t distinguish between the > > > > > > >>>> two cases in its response. > > > > > > >>>> > > > > > > >>>> Thanks, > > > > > > >>>> Andrew > > > > > > >>>> > > > > > > >>>>> On 16 Jul 2024, at 21:24, Lianet M. <liane...@gmail.com> > > > wrote: > > > > > > >>>>> > > > > > > >>>>> Hello Andrew, thanks for considering the feedback. Some > > > > follow-ups > > > > > and > > > > > > >>>>> other comments: > > > > > > >>>>> > > > > > > >>>>> LM4. Good point about the older RPC versions and therefore > > the > > > > > > >>>>> Optional<GroupType>, agreed. > > > > > > >>>>> > > > > > > >>>>> LM5. In GroupListing, should we use the existing > > > > > > >>>>> org.apache.kafka.clients.ProtocolType to represent the > > protocol > > > > > > >> (instead > > > > > > >>>> of > > > > > > >>>>> String). I don’t quite like the fact that the enum is > inside > > > the > > > > > > >>>>> GroupRebalanceConfig though, feels like it should be a > first > > > > level > > > > > > >>>> citizen. > > > > > > >>>>> > > > > > > >>>>> > > > > > > >>>>> LM6. Regarding the changes around ListGroupResults with > > > generics. > > > > > > >>>>> - LM6.1. What’s the need for keeping both, a base > > > > > > >>>>> AbstractListGroupsResult<T extends GroupListing> and the > > > > > > >> ListGroupsResult > > > > > > >>>>> extends AbstractListGroupsResult<GroupListing>? Would it > work > > > if > > > > > > >> instead > > > > > > >>>> we > > > > > > >>>>> simply have a single ListGroupsResult<T extends > GroupListing> > > > > from > > > > > > >> which > > > > > > >>>>> specific groups would inherit? I'm thinking of this: > > > > > > >>>>> > > > > > > >>>>> public class *ListGroupsResult<T extends GroupListing>* -> > > this > > > > > would > > > > > > >>>>> probably end up containing the implementation that > currently > > > > > exists in > > > > > > >>>>> ListConsumerGroupsResult for #all, #errors and #valid, that > > all > > > > > group > > > > > > >>>> types > > > > > > >>>>> would be able to reuse if we use a generic T extends > > > GroupListing > > > > > > >>>>> > > > > > > >>>>> > > > > > > >>>>> public class *ListConsumerGroupsResult extends > > > > > > >>>>> ListGroupsResult<ConsumerGroupListing>* -> slim impl, > agreed > > > > > > >>>>> > > > > > > >>>>> - LM6.2. Related to the concern of the javadoc for > > > > > > >>>>> ListConsumerGroupsResult. This class will inherit 3 funcs > > (all, > > > > > valid, > > > > > > >>>>> error), that have a common behaviour (and javadoc) > regardless > > > of > > > > > the > > > > > > >>>>> generic type, so I expect it won’t be confusing in > practice? > > We > > > > > will > > > > > > >> end > > > > > > >>>> up > > > > > > >>>>> with the java doc for, let’s say, > > ListConsumerGroupsResult#all > > > > > showing > > > > > > >>>> the > > > > > > >>>>> parent javadoc that aligns perfectly with what the #all > does. > > > If > > > > > ever > > > > > > >> we > > > > > > >>>>> need a different behaviour/javadoc for any of the functions > > in > > > > the > > > > > > >> child > > > > > > >>>>> classes, we would have the alternative of overriding the > func > > > and > > > > > > >>>> javadoc. > > > > > > >>>>> Makes sense? Not sure if I’m missing other readability > issues > > > > with > > > > > the > > > > > > >>>>> javadocs you’re seeing. > > > > > > >>>>> > > > > > > >>>>> LM7. Looks better to me now with the added filter on the > > > > > kafka-group.sh > > > > > > >>>> for > > > > > > >>>>> the protocol. But then, the new –consumer filter achieves > the > > > > same > > > > > as > > > > > > >>>>> –protocol CONSUMER right? If so, I wonder if it would just > be > > > > > simpler > > > > > > >> to > > > > > > >>>>> support the --protocol as a way to achieve this? (sharing > > your > > > > > struggle > > > > > > >>>> on > > > > > > >>>>> how to get this right, but feels easier to discover and > > reason > > > > > about > > > > > > >> the > > > > > > >>>>> more we have filters based on the output, and not made up > of > > > > > > >>>>> combinations....let's keep iterating and we'll get there > :) ) > > > > > > >>>>> > > > > > > >>>>> LM8. Is the output wrong (or just not clear) in this > example? > > > (It > > > > > > >> seemed > > > > > > >>>> to > > > > > > >>>>> me this was referring to the successful case where we > create > > a > > > > new > > > > > > >> share > > > > > > >>>>> group, so I was expecting a "successfully created" kind of > > > > output) > > > > > > >>>>> > > > > > > >>>>> $ bin/kafka-share-groups.sh --bootstrap-server > > > > > localhost:9092 > > > > > > >>>>> --create --group NewShareGroup > > > > > > >>>>> Share group 'NewShareGroup' exists. > > > > > > >>>>> > > > > > > >>>>> Thanks! > > > > > > >>>>> > > > > > > >>>>> Lianet > > > > > > >>>>> > > > > > > >>>>> On Mon, Jul 15, 2024 at 6:00 AM Andrew Schofield < > > > > > > >>>> andrew_schofi...@live.com> > > > > > > >>>>> wrote: > > > > > > >>>>> > > > > > > >>>>>> Hi Lianet, > > > > > > >>>>>> Thanks for your comments. > > > > > > >>>>>> > > > > > > >>>>>> LM1. Admin.listGroups() in principle needs to be able to > > > return > > > > > > >>>>>> results from any version of the ListGroups RPC. The older > > > > > versions do > > > > > > >>>>>> not contain the group type, so I think it’s reasonable to > > have > > > > > > >>>>>> Optional<GroupType>. I think there’s a difference between > > > > > > >>>>>> Optional.empty (I don’t know the group type) and > > > > > > >>>>>> GroupType.UNKNOWN (I know and do not understand the group > > > type). > > > > > > >>>>>> As a result, I’ve changed the KIP to use > > Optional<GroupType>. > > > > > > >>>>>> > > > > > > >>>>>> I think that changing ConsumerGroupListing to extend > > > > > > >>>>>> GroupListing, and to do the same for ShareGroupListing > makes > > > > > sense. > > > > > > >>>>>> This does require that the overridden methods such as > type() > > > > have > > > > > > >>>>>> signatures that match today’s definition of > > > ConsumerGroupListing > > > > > but > > > > > > >>>>>> that’s fine with the change I made to use > > Optional<GroupType> > > > > > above. > > > > > > >>>>>> > > > > > > >>>>>> LM2. I think it’s possible to do something with generics > > along > > > > the > > > > > > >>>>>> lines you described. > > > > > > >>>>>> > > > > > > >>>>>> * public abstract class AbstractListGroupsResult<T extends > > > > > > >> GroupListing> > > > > > > >>>>>> * public class ListGroupsResult extends > > > > > > >>>>>> AbstractListGroupsResult<GroupListing> > > > > > > >>>>>> * public class ListConsumerGroupsResult extends > > > > > > >>>>>> AbstractListGroupsResult<ConsumerGroupListing> > > > > > > >>>>>> > > > > > > >>>>>> This does make the javadoc for ListConsumerGroupsResult > less > > > > > > >>>>>> readable because its methods are now all inherited. The > > > classes > > > > > > >>>>>> such as ListConsumerGroupsResult of course still have to > > exist > > > > > > >>>>>> but the implementation is very slim. > > > > > > >>>>>> > > > > > > >>>>>> What do you think of this? I haven’t yet updated the KIP > in > > > this > > > > > > >>>>>> case. > > > > > > >>>>>> > > > > > > >>>>>> LM3. I have been kicking around the syntax for > > kafka-group.sh > > > > > > >>>>>> for a while now and I too am not happy with the filters > > yet. I > > > > > > >>>> absolutely > > > > > > >>>>>> want to be able to display all consumer groups with a > simple > > > > > option, > > > > > > >>>>>> but history means that not a single filter under the > covers. > > > > > > >>>>>> > > > > > > >>>>>> I suggest the following: > > > > > > >>>>>> --group-type which supports all group types > > > > > > >>>>>> --protocol which supports any string for protocol (there’s > > no > > > > > > >>>> enumeration) > > > > > > >>>>>> --consumer which matches all classic and modern consumer > > > groups > > > > > > >>>>>> (and is thus a confection made by filtering on both group > > type > > > > and > > > > > > >>>>>> protocol). > > > > > > >>>>>> > > > > > > >>>>>> I’ve changed the KIP accordingly. Let me know what you > > think. > > > > > > >>>>>> > > > > > > >>>>>> Thanks, > > > > > > >>>>>> Andrew > > > > > > >>>>>> > > > > > > >>>>>> > > > > > > >>>>>>> On 12 Jul 2024, at 21:48, Lianet M. <liane...@gmail.com> > > > > wrote: > > > > > > >>>>>>> > > > > > > >>>>>>> Hey Andrew, thanks for the KIP, we definitely need > > visibility > > > > > from a > > > > > > >>>>>> higher > > > > > > >>>>>>> level now that groups are growing. > > > > > > >>>>>>> > > > > > > >>>>>>> LM1. Should we have the existing > > > > > > >>>>>>> org.apache.kafka.clients.admin.ConsumerGroupListing > extend > > > the > > > > > > >>>>>> GroupListing > > > > > > >>>>>>> you’re proposing? ConsumerGroupListing already exists > with > > a > > > > very > > > > > > >>>> similar > > > > > > >>>>>>> shape, and this would allow to set a common ground for > the > > > > > existing > > > > > > >>>> group > > > > > > >>>>>>> types, and the ones that are coming up (share groups and > KS > > > > > groups). > > > > > > >>>> Side > > > > > > >>>>>>> note, the existing ConsumerGroupListing has the type as > > > > > Optional, but > > > > > > >>>>>> given > > > > > > >>>>>>> that the GroupType enum has an UNKNOWN type, I don’t > quite > > > get > > > > > the > > > > > > >> need > > > > > > >>>>>> for > > > > > > >>>>>>> Optional and seems ok to me as you’re proposing it. > > > > > > >>>>>>> > > > > > > >>>>>>> LM2: if the point above makes sense, it would allow us to > > > > > consider > > > > > > >>>>>> changing > > > > > > >>>>>>> the new ListGroupResult you’re proposing to make it > generic > > > and > > > > > > >>>>>> potentially > > > > > > >>>>>>> reused by all group types: > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>> public class ListGroupsResult { > > > > > > >>>>>>> > > > > > > >>>>>>> public KafkaFuture<Collection<? extends GroupListing>> > > all() > > > > > > >>>>>>> > > > > > > >>>>>>> public KafkaFuture<Collection<? extends GroupListing>> > > > valid() > > > > > { } > > > > > > >>>>>>> > > > > > > >>>>>>> public KafkaFuture<Collection<Throwable>> errors() { } > > > > > > >>>>>>> > > > > > > >>>>>>> } > > > > > > >>>>>>> > > > > > > >>>>>>> Makes sense? With this, maybe we won’t need specific > result > > > > > classes > > > > > > >> for > > > > > > >>>>>>> each group (like the existing ListConsumerGroupsResult), > > > given > > > > > that > > > > > > >> in > > > > > > >>>>>> the > > > > > > >>>>>>> end it’s just a wrapper around the GroupListing (which is > > > what > > > > > each > > > > > > >>>> group > > > > > > >>>>>>> type would redefine). > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>> LM3. I get how you're playing with the filters for group > > > types > > > > > and > > > > > > >>>>>>> protocol, but then I find it confusing how we end up with > > > > filters > > > > > > >> that > > > > > > >>>> do > > > > > > >>>>>>> not match the output ( --group-type that matches the > > protocol > > > > > from > > > > > > >> the > > > > > > >>>>>>> output and not the type for "consumer" example). What > > about > > > > > having > > > > > > >> the > > > > > > >>>>>>> –group-type filter on the actual GroupType field of the > RPC > > > > > response > > > > > > >>>>>> (shown > > > > > > >>>>>>> in the cmd line output as TYPE); and add a –protocol-type > > > that > > > > > would > > > > > > >>>>>> filter > > > > > > >>>>>>> on the ProtocolType field of RPC response (shown in the > > cmd > > > > line > > > > > > >>>> output > > > > > > >>>>>> as > > > > > > >>>>>>> PROTOCOL). We would have the filters aligned with the > > output > > > > for > > > > > all > > > > > > >>>>>> cases, > > > > > > >>>>>>> which seems more consistent. > > > > > > >>>>>>> > > > > > > >>>>>>> Thanks! > > > > > > >>>>>>> > > > > > > >>>>>>> Lianet > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>> On Thu, Jun 6, 2024 at 8:16 AM Andrew Schofield < > > > > > > >>>>>> andrew_schofi...@live.com> > > > > > > >>>>>>> wrote: > > > > > > >>>>>>> > > > > > > >>>>>>>> Hi Kirk, > > > > > > >>>>>>>> Thanks for your comments. > > > > > > >>>>>>>> > > > > > > >>>>>>>> 1. I’m a big fan of consistency in these things and the > > > method > > > > > > >>>>>> signatures > > > > > > >>>>>>>> match > > > > > > >>>>>>>> ListConsumerGroupsResult and ListShareGroupsResult. > > > > > > >>>>>>>> > > > > > > >>>>>>>> 2. Yes, client-side filtering. > > > > > > >>>>>>>> > > > > > > >>>>>>>> 3. I didn’t offer “classic” as an option for > --group-type. > > > > I’ve > > > > > > >> kicked > > > > > > >>>>>> the > > > > > > >>>>>>>> options > > > > > > >>>>>>>> around in my mind for a while and I decided that using > > > > > --group-type > > > > > > >>>> as a > > > > > > >>>>>>>> way of > > > > > > >>>>>>>> filtering types in a way that a normal user would > > understand > > > > > them > > > > > > >> was > > > > > > >>>> a > > > > > > >>>>>>>> good > > > > > > >>>>>>>> place to start. For example, I didn’t have `--protocol > > > > > consumer` for > > > > > > >>>>>>>> consumer groups > > > > > > >>>>>>>> and `--group-type share` for share groups, even though > > > that’s > > > > > > >>>>>> technically > > > > > > >>>>>>>> more > > > > > > >>>>>>>> correct. > > > > > > >>>>>>>> > > > > > > >>>>>>>> Since KIP-848, the set of consumer groups is actually > > formed > > > > > from > > > > > > >>>> those > > > > > > >>>>>>>> which > > > > > > >>>>>>>> use the classic protocol and those which use the modern > > > > > protocol. > > > > > > >> This > > > > > > >>>>>> tool > > > > > > >>>>>>>> gives you both together when you use `--group-type > > > consumer`, > > > > > which > > > > > > >> is > > > > > > >>>>>>>> exactly > > > > > > >>>>>>>> what kafka-consumer-groups.sh does. > > > > > > >>>>>>>> > > > > > > >>>>>>>> Do you think - -group-type classic is helpful? It would > > > give a > > > > > list > > > > > > >> of > > > > > > >>>>>> all > > > > > > >>>>>>>> groups using > > > > > > >>>>>>>> any variant of the classic group protocol. I can easily > > add > > > > it. > > > > > > >>>>>>>> > > > > > > >>>>>>>> 4, 5. Yes, maybe the wording of the message could > improve. > > > > These > > > > > > >>>> things > > > > > > >>>>>>>> are always > > > > > > >>>>>>>> tricky. I went with “Group CG1 is not a share group.” > > > because > > > > it > > > > > > >>>> doesn’t > > > > > > >>>>>>>> require the tool > > > > > > >>>>>>>> to interpret the group type in order to generate the > > > message. > > > > > > >>>>>>>> > > > > > > >>>>>>>> Imagine this scenario. You are using > kafka-share-groups.sh > > > > > > >> --describe > > > > > > >>>>>> and > > > > > > >>>>>>>> you’ve > > > > > > >>>>>>>> used the group ID of a consumer group. Here are some > > > options: > > > > > > >>>>>>>> > > > > > > >>>>>>>> a) “Group CG1 is not a share group.” > > > > > > >>>>>>>> b) “Incorrect group type (Consumer). Group CG1 is not a > > > share > > > > > > >> group.” > > > > > > >>>>>>>> c) “Group CG1 has the wrong type for this operation. It > is > > > > not a > > > > > > >> share > > > > > > >>>>>>>> group." > > > > > > >>>>>>>> > > > > > > >>>>>>>> I don’t think “There is already a (consumer) group named > > > > ‘CG1’” > > > > > is > > > > > > >>>> quite > > > > > > >>>>>>>> right. > > > > > > >>>>>>>> > > > > > > >>>>>>>> Any preference? > > > > > > >>>>>>>> > > > > > > >>>>>>>> 6. Yes, it is a change in behaviour which is why I > mention > > > it > > > > > in the > > > > > > >>>>>> KIP. > > > > > > >>>>>>>> Personally, I think that’s OK because the existing > message > > > is > > > > > > >>>> misleading > > > > > > >>>>>>>> and could definitely cause frustration. Let’s see what > > other > > > > > > >> reviewers > > > > > > >>>>>>>> think. > > > > > > >>>>>>>> > > > > > > >>>>>>>> Thanks, > > > > > > >>>>>>>> Andrew > > > > > > >>>>>>>> > > > > > > >>>>>>>>> On 6 Jun 2024, at 00:44, Kirk True <k...@kirktrue.pro> > > > > wrote: > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> Hi Andrew, > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> Thanks for the KIP! I don’t have much experience as a > > Kafka > > > > > > >> operator, > > > > > > >>>>>>>> but this seems like a very sane proposal. > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> Questions & comments: > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> 1. Do you think the ListGroupsResult.all() method is a > > bit > > > > of a > > > > > > >>>>>>>> potential ‘foot gun’? I can imagine cases where > developers > > > > > reach for > > > > > > >>>>>> that > > > > > > >>>>>>>> without understanding its potential of throwing errors. > It > > > > could > > > > > > >> lead > > > > > > >>>> to > > > > > > >>>>>>>> cases where all() works in development but not in > > > production. > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> 2. Based on the LIST_GROUPS RPC, it appears that > > filtering > > > is > > > > > all > > > > > > >>>>>>>> performed client side, correct? (I know that’s not > > specific > > > to > > > > > this > > > > > > >>>> KIP, > > > > > > >>>>>>>> but just want to make sure I understand.) > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> 3. For kafka-groups.sh --list, is ‘classic’ valid for > > > > > --group-type? > > > > > > >>>> If > > > > > > >>>>>>>> so, should we allow users of kafka-groups.sh --list to > > > provide > > > > > > >>>> multiple > > > > > > >>>>>>>> --group-type arguments? > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> 4. In the last kafka-share-groups.sh --create example > > > > > > >>>>>> (“ConsumerGroup”), > > > > > > >>>>>>>> the error simply states that “Group 'ConsumerGroup’ is > > not a > > > > > share > > > > > > >>>>>> group.” > > > > > > >>>>>>>> I’m assuming that’s the case where the user gets a > failure > > > > when > > > > > > >>>> there’s > > > > > > >>>>>>>> already a group named “ConsumerGroup”, right? If so, the > > > error > > > > > > >> should > > > > > > >>>> be > > > > > > >>>>>>>> something like “There is already a (consumer) group > named > > > > > > >>>>>> ’ConsumerGroup’”. > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> 5. In the last kafka-share-groups.sh --describe > example, > > > how > > > > > hard > > > > > > >> is > > > > > > >>>> it > > > > > > >>>>>>>> to add the type of group that CG1 is, just for a bit of > > > > clarity > > > > > for > > > > > > >>>> the > > > > > > >>>>>>>> user? > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> 6. In the kafka-consumer-groups.sh section, it states > "if > > > > that > > > > > > >> group > > > > > > >>>>>>>> exists but is not a consumer group, the command fails > > with a > > > > > message > > > > > > >>>>>>>> indicating that the group type is incorrect, rather than > > the > > > > > > >> existing > > > > > > >>>>>>>> message that the group does not exist.” That sounds > like a > > > > > change > > > > > > >> that > > > > > > >>>>>>>> could trip up some brittle scripts somewhere, but I > don’t > > > know > > > > > if > > > > > > >>>>>> that’s a > > > > > > >>>>>>>> serious problem. > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> Thanks! > > > > > > >>>>>>>>> > > > > > > >>>>>>>>>> On Jun 4, 2024, at 10:08 AM, Andrew Schofield < > > > > > > >>>>>>>> andrew_schofi...@live.com> wrote: > > > > > > >>>>>>>>>> > > > > > > >>>>>>>>>> Hi, > > > > > > >>>>>>>>>> I would like to start a discussion thread on KIP-1043: > > > > > > >>>> Administration > > > > > > >>>>>>>> of groups. This KIP enhances the command-line tools to > > make > > > it > > > > > > >> easier > > > > > > >>>> to > > > > > > >>>>>>>> administer groups on clusters with a variety of types of > > > > groups. > > > > > > >>>>>>>>>> > > > > > > >>>>>>>>>> > > > > > > >>>>>>>> > > > > > > >>>>>> > > > > > > >>>> > > > > > > >> > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1043%3A+Administration+of+groups > > > > > > >>>>>>>>>> > > > > > > >>>>>>>>>> Thanks. > > > > > > >>>>>>>>>> Andrew > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > >
