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