Hi Andrew, Took another full pass here after the latest changes (thanks for addressing all the feedback!).
Only minor comment left is to add to the reject alternatives the INCONSISTENT_GROUP_PROTOCOL error that we discussed and discarded (see LM9). Also agree with the 2 minor points made by Lucas. With that, I have no other comments and agree we could proceed to voting. Thanks! Lianet On Wed, Sep 11, 2024 at 10:17 AM Lucas Brutschy <lbruts...@confluent.io.invalid> wrote: > 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 > > >> > > >> > > >> >
