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
> > > > > >>
> > > > > >>
> > > > > >>
> > > >
> > >
> >
>

Reply via email to