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