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
