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