You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Colin McCabe <cm...@apache.org> on 2020/02/06 21:43:53 UTC
Re: [DISCUSS] KIP-518: Allow listing consumer groups per state
Hi Mickael,
Can you please specify what the result is when a newer client tries to use this on an older broker? Does that result in an UnsupportedVersionException?
I would prefer an Optional in the Java API so that “show all groups” can be EMPTY.
best,
Colin
On Mon, Jan 27, 2020, at 07:53, Mickael Maison wrote:
> Hi David,
>
> Did that answer your questions? or do you have any further feedback?
>
> Thanks
>
> On Thu, Jan 16, 2020 at 4:11 PM Mickael Maison <mi...@gmail.com> wrote:
> >
> > Hi David,
> >
> > Thanks for taking a look.
> > 1) Yes, updated
> >
> > 2) I had not considered that but indeed this would be useful if the
> > request contained multiple states and would avoid doing another call.
> > The ListGroups response already includes the group ProtocolType, so I
> > guess we could add the State as well. The response will still be
> > significantly smaller than DescribeGroups. With this change, one thing
> > to note is that having Describe on the Cluster resource will allow
> > retrieving the state of all groups. Currently retrieving the state of
> > a group requires Describe on the Group.
> >
> > 3) Yes if ListGroups response includes the state, it makes sense to
> > expose it via the command line tool and the AdminClient. With
> > ConsumerGroupCommand, to avoid compatibility issues we can only print
> > states when the --states flag is specified.
> >
> > I've updated the KIP accordingly.
> >
> > On Mon, Jan 13, 2020 at 12:20 PM David Jacot <dj...@confluent.io> wrote:
> > >
> > > Hi Michael,
> > >
> > > Please, excuse me for my late feedback. I've got a few questions/comments
> > > while reviewing the KIP.
> > >
> > > 1. I would suggest to clearly state in the documentation of the state field
> > > that omitting it or providing an empty list means "all".
> > >
> > > 2. Have you considered including the state in the response? The API allows
> > > to search for multiple states so it could be
> > > convenient to have the state in the response to let the user differentiate
> > > the groups.
> > >
> > > 3. If 2. makes sense, I would suggest to also include it in the information
> > > printed out by the ConsumerGroupCommand tool. Putting
> > > myself in the shoes of an operator, I would like to see the state of each
> > > group if I select specific states. Perhaps we could
> > > use a table instead of the simple list used today. What do you think?
> > >
> > > Thanks for the KIP!
> > >
> > > Best,
> > > David
> > >
> > > On Mon, Nov 11, 2019 at 12:40 PM Mickael Maison <mi...@gmail.com>
> > > wrote:
> > >
> > > > Hi all,
> > > >
> > > > If there's no more feedback, I'll open a vote in the next few days.
> > > >
> > > > Thanks
> > > >
> > > >
> > > > On Fri, Nov 1, 2019 at 4:27 PM Mickael Maison <mi...@gmail.com>
> > > > wrote:
> > > > >
> > > > > Hi Tom,
> > > > >
> > > > > Thanks for taking a look at the KIP!
> > > > > You are right, even if we serialize the field as a String, we should
> > > > > use ConsumerGroupState in the API.
> > > > > As suggested, I've also updated the API so a list of states is specified.
> > > > >
> > > > > Regards,
> > > > >
> > > > >
> > > > > On Tue, Oct 22, 2019 at 10:03 AM Tom Bentley <tb...@redhat.com>
> > > > wrote:
> > > > > >
> > > > > > Hi Mickael,
> > > > > >
> > > > > > Thanks for the KIP.
> > > > > >
> > > > > > The use of String to represent the desired state in the API seems less
> > > > > > typesafe than would be ideal. Is there a reason not to use the existing
> > > > > > ConsumerGroupState enum (even if the state is serialized as a String)?
> > > > > >
> > > > > > While you say that the list-of-names result from listConsumerGroups is
> > > > a
> > > > > > reason not to support supplying a set of desired states I don't find
> > > > that
> > > > > > argument entirely convincing. Sure, if the results are going to be
> > > > shown to
> > > > > > a user then it would be ambiguous and multiple queries would be
> > > > needed. But
> > > > > > it seems quite possible that the returned list of groups will
> > > > immediately
> > > > > > be used in a describeConsumerGroups query (for example, so show a user
> > > > > > additional information about the groups of interest, for example). In
> > > > that
> > > > > > case the grouping by state could be done on the descriptions, and some
> > > > RPCs
> > > > > > could be avoided. It would also avoid the race inherent in making
> > > > multiple
> > > > > > listConsumerGroups requests. So supporting a set of states isn't
> > > > entirely
> > > > > > worthless and it wouldn't really add very much complexity.
> > > > > >
> > > > > > Kind regards,
> > > > > >
> > > > > > Tom
> > > > > >
> > > > > > On Mon, Oct 21, 2019 at 5:54 PM Mickael Maison <
> > > > mickael.maison@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Bump
> > > > > > > Now that the rush for 2.4.0 is ending I hope to get some feedback
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > On Mon, Sep 9, 2019 at 5:44 PM Mickael Maison <
> > > > mickael.maison@gmail.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > I have created a KIP to allow listing groups per state:
> > > > > > > >
> > > > > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-518%3A+Allow+listing+consumer+groups+per+state
> > > > > > > >
> > > > > > > > Have a look and let me know what you think.
> > > > > > > > Thank you
> > > > > > >
> > > >
>
Re: [DISCUSS] KIP-518: Allow listing consumer groups per state
Posted by David Jacot <dj...@confluent.io>.
Hi Mickael,
My apologies for the delay. Overall, the KIP looks good to me.
One small suggestion is to update the docstring of the States field in the
request to
clearly state that all groups are returned if omitted or empty. People rely
on the JSON
description so it would be good to provide the information there.
Thanks,
David
On Thu, Feb 20, 2020 at 6:05 PM Mickael Maison <mi...@gmail.com>
wrote:
> Thanks Colin,
>
> > I guess I don't feel that strongly about it. However, if we are going
> to rely on inAnyState() / inStates(...) then let's make sure that the
> latter throws an exception when getting an empty list as an argument (since
> it will do something other than the obvious behavior of listing nothing).
>
> Yes I've made that explicit in the KIP
>
> On Wed, Feb 19, 2020 at 7:21 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > On Thu, Feb 13, 2020, at 08:24, Mickael Maison wrote:
> > > Hi Colin,
> > >
> > > > Can you please specify what the result is when a newer client tries
> to use
> > > > this on an older broker? Does that result in an
> > > > UnsupportedVersionException?
> > > Yes, in case this feature is not supported by the broker,
> > > UnsupportedVersionException should be raised. Your question made me
> > > look at it again and to properly handle all cases, I decided to bump
> > > the ListGroups API version. That way, we can't end up in cases where
> > > brokers know about the protocol version but not about the new optional
> > > field. I've updated the KIP accordingly
> >
> > Yes, a protocol version bump seems fine here.
> >
> > >
> > > > I would prefer an Optional in the Java API so that “show all groups”
> can
> > > > be EMPTY.
> > > I think it's nicer to provide a separate method in
> > > ListConsumerGroupsOptions to request all states without listing them
> > > rather than relying on empty Optional. I added a new method to the
> > > proposal: "inAnyState()".
> >
> > I guess I don't feel that strongly about it. However, if we are going
> to rely on inAnyState() / inStates(...) then let's make sure that the
> latter throws an exception when getting an empty list as an argument (since
> it will do something other than the obvious behavior of listing nothing).
> >
> > >
> > > Hi David,
> > >
> > > > 1. We already have the "--state" option in the command line tool
> which can
> > > > be used
> > > > with "--describe" and we will have "--states" which can be used with
> > > > "--list". I feel this
> > > > is going to be confusing for users. I wonder if we could combine
> both cases
> > > > to reduce
> > > > the confusion but I am not sure it would be better. What do you
> think?
> > > Yes absolutely, it makes sense to reuse this existing flag. Good catch!
> > >
> > > > 2. Regarding the output of the command line when "--states" is used,
> I
> > > > wonder if it
> > > > wouldn't be better to use a proper table with a header. We could use
> only
> > > > when
> > > > filters such as "--states" are used.
> > > Yes that's a good idea.
> > >
> > > I've updated the KIP to take all these suggestions into account.
> > > Thanks again for the feedback
> >
> > Thanks, Mickael. It looks good.
> >
> > best,
> > Colin
> >
> >
> > >
> > > On Mon, Feb 10, 2020 at 1:13 PM David Jacot <dj...@confluent.io>
> wrote:
> > > >
> > > > Hi Michael,
> > > >
> > > > Thank you for the updated KIP. I have few comments/questions:
> > > >
> > > > 1. We already have the "--state" option in the command line tool
> which can
> > > > be used
> > > > with "--describe" and we will have "--states" which can be used with
> > > > "--list". I feel this
> > > > is going to be confusing for users. I wonder if we could combine
> both cases
> > > > to reduce
> > > > the confusion but I am not sure it would be better. What do you
> think?
> > > >
> > > > 2. Regarding the output of the command line when "--states" is used,
> I
> > > > wonder if it
> > > > wouldn't be better to use a proper table with a header. We could use
> only
> > > > when
> > > > filters such as "--states" are used.
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Thu, Feb 6, 2020 at 10:44 PM Colin McCabe <cm...@apache.org>
> wrote:
> > > >
> > > > > Hi Mickael,
> > > > >
> > > > > Can you please specify what the result is when a newer client
> tries to use
> > > > > this on an older broker? Does that result in an
> > > > > UnsupportedVersionException?
> > > > >
> > > > > I would prefer an Optional in the Java API so that “show all
> groups” can
> > > > > be EMPTY.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Mon, Jan 27, 2020, at 07:53, Mickael Maison wrote:
> > > > > > Hi David,
> > > > > >
> > > > > > Did that answer your questions? or do you have any further
> feedback?
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > On Thu, Jan 16, 2020 at 4:11 PM Mickael Maison <
> mickael.maison@gmail.com>
> > > > > wrote:
> > > > > > >
> > > > > > > Hi David,
> > > > > > >
> > > > > > > Thanks for taking a look.
> > > > > > > 1) Yes, updated
> > > > > > >
> > > > > > > 2) I had not considered that but indeed this would be useful
> if the
> > > > > > > request contained multiple states and would avoid doing
> another call.
> > > > > > > The ListGroups response already includes the group
> ProtocolType, so I
> > > > > > > guess we could add the State as well. The response will still
> be
> > > > > > > significantly smaller than DescribeGroups. With this change,
> one thing
> > > > > > > to note is that having Describe on the Cluster resource will
> allow
> > > > > > > retrieving the state of all groups. Currently retrieving the
> state of
> > > > > > > a group requires Describe on the Group.
> > > > > > >
> > > > > > > 3) Yes if ListGroups response includes the state, it makes
> sense to
> > > > > > > expose it via the command line tool and the AdminClient. With
> > > > > > > ConsumerGroupCommand, to avoid compatibility issues we can
> only print
> > > > > > > states when the --states flag is specified.
> > > > > > >
> > > > > > > I've updated the KIP accordingly.
> > > > > > >
> > > > > > > On Mon, Jan 13, 2020 at 12:20 PM David Jacot <
> djacot@confluent.io>
> > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Michael,
> > > > > > > >
> > > > > > > > Please, excuse me for my late feedback. I've got a few
> > > > > questions/comments
> > > > > > > > while reviewing the KIP.
> > > > > > > >
> > > > > > > > 1. I would suggest to clearly state in the documentation of
> the
> > > > > state field
> > > > > > > > that omitting it or providing an empty list means "all".
> > > > > > > >
> > > > > > > > 2. Have you considered including the state in the response?
> The API
> > > > > allows
> > > > > > > > to search for multiple states so it could be
> > > > > > > > convenient to have the state in the response to let the user
> > > > > differentiate
> > > > > > > > the groups.
> > > > > > > >
> > > > > > > > 3. If 2. makes sense, I would suggest to also include it in
> the
> > > > > information
> > > > > > > > printed out by the ConsumerGroupCommand tool. Putting
> > > > > > > > myself in the shoes of an operator, I would like to see the
> state of
> > > > > each
> > > > > > > > group if I select specific states. Perhaps we could
> > > > > > > > use a table instead of the simple list used today. What do
> you think?
> > > > > > > >
> > > > > > > > Thanks for the KIP!
> > > > > > > >
> > > > > > > > Best,
> > > > > > > > David
> > > > > > > >
> > > > > > > > On Mon, Nov 11, 2019 at 12:40 PM Mickael Maison <
> > > > > mickael.maison@gmail.com>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi all,
> > > > > > > > >
> > > > > > > > > If there's no more feedback, I'll open a vote in the next
> few days.
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Fri, Nov 1, 2019 at 4:27 PM Mickael Maison <
> > > > > mickael.maison@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Tom,
> > > > > > > > > >
> > > > > > > > > > Thanks for taking a look at the KIP!
> > > > > > > > > > You are right, even if we serialize the field as a
> String, we
> > > > > should
> > > > > > > > > > use ConsumerGroupState in the API.
> > > > > > > > > > As suggested, I've also updated the API so a list of
> states is
> > > > > specified.
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Tue, Oct 22, 2019 at 10:03 AM Tom Bentley <
> > > > > tbentley@redhat.com>
> > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Mickael,
> > > > > > > > > > >
> > > > > > > > > > > Thanks for the KIP.
> > > > > > > > > > >
> > > > > > > > > > > The use of String to represent the desired state in
> the API
> > > > > seems less
> > > > > > > > > > > typesafe than would be ideal. Is there a reason not to
> use the
> > > > > existing
> > > > > > > > > > > ConsumerGroupState enum (even if the state is
> serialized as a
> > > > > String)?
> > > > > > > > > > >
> > > > > > > > > > > While you say that the list-of-names result from
> > > > > listConsumerGroups is
> > > > > > > > > a
> > > > > > > > > > > reason not to support supplying a set of desired
> states I
> > > > > don't find
> > > > > > > > > that
> > > > > > > > > > > argument entirely convincing. Sure, if the results are
> going
> > > > > to be
> > > > > > > > > shown to
> > > > > > > > > > > a user then it would be ambiguous and multiple queries
> would be
> > > > > > > > > needed. But
> > > > > > > > > > > it seems quite possible that the returned list of
> groups will
> > > > > > > > > immediately
> > > > > > > > > > > be used in a describeConsumerGroups query (for
> example, so
> > > > > show a user
> > > > > > > > > > > additional information about the groups of interest,
> for
> > > > > example). In
> > > > > > > > > that
> > > > > > > > > > > case the grouping by state could be done on the
> descriptions,
> > > > > and some
> > > > > > > > > RPCs
> > > > > > > > > > > could be avoided. It would also avoid the race
> inherent in
> > > > > making
> > > > > > > > > multiple
> > > > > > > > > > > listConsumerGroups requests. So supporting a set of
> states
> > > > > isn't
> > > > > > > > > entirely
> > > > > > > > > > > worthless and it wouldn't really add very much
> complexity.
> > > > > > > > > > >
> > > > > > > > > > > Kind regards,
> > > > > > > > > > >
> > > > > > > > > > > Tom
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Oct 21, 2019 at 5:54 PM Mickael Maison <
> > > > > > > > > mickael.maison@gmail.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Bump
> > > > > > > > > > > > Now that the rush for 2.4.0 is ending I hope to get
> some
> > > > > feedback
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Sep 9, 2019 at 5:44 PM Mickael Maison <
> > > > > > > > > mickael.maison@gmail.com>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > I have created a KIP to allow listing groups per
> state:
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > >
> > > > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-518%3A+Allow+listing+consumer+groups+per+state
> > > > > > > > > > > > >
> > > > > > > > > > > > > Have a look and let me know what you think.
> > > > > > > > > > > > > Thank you
> > > > > > > > > > > >
> > > > > > > > >
> > > > > >
> > > > >
> > >
>
Re: [DISCUSS] KIP-518: Allow listing consumer groups per state
Posted by Mickael Maison <mi...@gmail.com>.
Thanks Colin,
> I guess I don't feel that strongly about it. However, if we are going to rely on inAnyState() / inStates(...) then let's make sure that the latter throws an exception when getting an empty list as an argument (since it will do something other than the obvious behavior of listing nothing).
Yes I've made that explicit in the KIP
On Wed, Feb 19, 2020 at 7:21 PM Colin McCabe <cm...@apache.org> wrote:
>
> On Thu, Feb 13, 2020, at 08:24, Mickael Maison wrote:
> > Hi Colin,
> >
> > > Can you please specify what the result is when a newer client tries to use
> > > this on an older broker? Does that result in an
> > > UnsupportedVersionException?
> > Yes, in case this feature is not supported by the broker,
> > UnsupportedVersionException should be raised. Your question made me
> > look at it again and to properly handle all cases, I decided to bump
> > the ListGroups API version. That way, we can't end up in cases where
> > brokers know about the protocol version but not about the new optional
> > field. I've updated the KIP accordingly
>
> Yes, a protocol version bump seems fine here.
>
> >
> > > I would prefer an Optional in the Java API so that “show all groups” can
> > > be EMPTY.
> > I think it's nicer to provide a separate method in
> > ListConsumerGroupsOptions to request all states without listing them
> > rather than relying on empty Optional. I added a new method to the
> > proposal: "inAnyState()".
>
> I guess I don't feel that strongly about it. However, if we are going to rely on inAnyState() / inStates(...) then let's make sure that the latter throws an exception when getting an empty list as an argument (since it will do something other than the obvious behavior of listing nothing).
>
> >
> > Hi David,
> >
> > > 1. We already have the "--state" option in the command line tool which can
> > > be used
> > > with "--describe" and we will have "--states" which can be used with
> > > "--list". I feel this
> > > is going to be confusing for users. I wonder if we could combine both cases
> > > to reduce
> > > the confusion but I am not sure it would be better. What do you think?
> > Yes absolutely, it makes sense to reuse this existing flag. Good catch!
> >
> > > 2. Regarding the output of the command line when "--states" is used, I
> > > wonder if it
> > > wouldn't be better to use a proper table with a header. We could use only
> > > when
> > > filters such as "--states" are used.
> > Yes that's a good idea.
> >
> > I've updated the KIP to take all these suggestions into account.
> > Thanks again for the feedback
>
> Thanks, Mickael. It looks good.
>
> best,
> Colin
>
>
> >
> > On Mon, Feb 10, 2020 at 1:13 PM David Jacot <dj...@confluent.io> wrote:
> > >
> > > Hi Michael,
> > >
> > > Thank you for the updated KIP. I have few comments/questions:
> > >
> > > 1. We already have the "--state" option in the command line tool which can
> > > be used
> > > with "--describe" and we will have "--states" which can be used with
> > > "--list". I feel this
> > > is going to be confusing for users. I wonder if we could combine both cases
> > > to reduce
> > > the confusion but I am not sure it would be better. What do you think?
> > >
> > > 2. Regarding the output of the command line when "--states" is used, I
> > > wonder if it
> > > wouldn't be better to use a proper table with a header. We could use only
> > > when
> > > filters such as "--states" are used.
> > >
> > > Best,
> > > David
> > >
> > > On Thu, Feb 6, 2020 at 10:44 PM Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > Hi Mickael,
> > > >
> > > > Can you please specify what the result is when a newer client tries to use
> > > > this on an older broker? Does that result in an
> > > > UnsupportedVersionException?
> > > >
> > > > I would prefer an Optional in the Java API so that “show all groups” can
> > > > be EMPTY.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Mon, Jan 27, 2020, at 07:53, Mickael Maison wrote:
> > > > > Hi David,
> > > > >
> > > > > Did that answer your questions? or do you have any further feedback?
> > > > >
> > > > > Thanks
> > > > >
> > > > > On Thu, Jan 16, 2020 at 4:11 PM Mickael Maison <mi...@gmail.com>
> > > > wrote:
> > > > > >
> > > > > > Hi David,
> > > > > >
> > > > > > Thanks for taking a look.
> > > > > > 1) Yes, updated
> > > > > >
> > > > > > 2) I had not considered that but indeed this would be useful if the
> > > > > > request contained multiple states and would avoid doing another call.
> > > > > > The ListGroups response already includes the group ProtocolType, so I
> > > > > > guess we could add the State as well. The response will still be
> > > > > > significantly smaller than DescribeGroups. With this change, one thing
> > > > > > to note is that having Describe on the Cluster resource will allow
> > > > > > retrieving the state of all groups. Currently retrieving the state of
> > > > > > a group requires Describe on the Group.
> > > > > >
> > > > > > 3) Yes if ListGroups response includes the state, it makes sense to
> > > > > > expose it via the command line tool and the AdminClient. With
> > > > > > ConsumerGroupCommand, to avoid compatibility issues we can only print
> > > > > > states when the --states flag is specified.
> > > > > >
> > > > > > I've updated the KIP accordingly.
> > > > > >
> > > > > > On Mon, Jan 13, 2020 at 12:20 PM David Jacot <dj...@confluent.io>
> > > > wrote:
> > > > > > >
> > > > > > > Hi Michael,
> > > > > > >
> > > > > > > Please, excuse me for my late feedback. I've got a few
> > > > questions/comments
> > > > > > > while reviewing the KIP.
> > > > > > >
> > > > > > > 1. I would suggest to clearly state in the documentation of the
> > > > state field
> > > > > > > that omitting it or providing an empty list means "all".
> > > > > > >
> > > > > > > 2. Have you considered including the state in the response? The API
> > > > allows
> > > > > > > to search for multiple states so it could be
> > > > > > > convenient to have the state in the response to let the user
> > > > differentiate
> > > > > > > the groups.
> > > > > > >
> > > > > > > 3. If 2. makes sense, I would suggest to also include it in the
> > > > information
> > > > > > > printed out by the ConsumerGroupCommand tool. Putting
> > > > > > > myself in the shoes of an operator, I would like to see the state of
> > > > each
> > > > > > > group if I select specific states. Perhaps we could
> > > > > > > use a table instead of the simple list used today. What do you think?
> > > > > > >
> > > > > > > Thanks for the KIP!
> > > > > > >
> > > > > > > Best,
> > > > > > > David
> > > > > > >
> > > > > > > On Mon, Nov 11, 2019 at 12:40 PM Mickael Maison <
> > > > mickael.maison@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > If there's no more feedback, I'll open a vote in the next few days.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, Nov 1, 2019 at 4:27 PM Mickael Maison <
> > > > mickael.maison@gmail.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > Thanks for taking a look at the KIP!
> > > > > > > > > You are right, even if we serialize the field as a String, we
> > > > should
> > > > > > > > > use ConsumerGroupState in the API.
> > > > > > > > > As suggested, I've also updated the API so a list of states is
> > > > specified.
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Tue, Oct 22, 2019 at 10:03 AM Tom Bentley <
> > > > tbentley@redhat.com>
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Mickael,
> > > > > > > > > >
> > > > > > > > > > Thanks for the KIP.
> > > > > > > > > >
> > > > > > > > > > The use of String to represent the desired state in the API
> > > > seems less
> > > > > > > > > > typesafe than would be ideal. Is there a reason not to use the
> > > > existing
> > > > > > > > > > ConsumerGroupState enum (even if the state is serialized as a
> > > > String)?
> > > > > > > > > >
> > > > > > > > > > While you say that the list-of-names result from
> > > > listConsumerGroups is
> > > > > > > > a
> > > > > > > > > > reason not to support supplying a set of desired states I
> > > > don't find
> > > > > > > > that
> > > > > > > > > > argument entirely convincing. Sure, if the results are going
> > > > to be
> > > > > > > > shown to
> > > > > > > > > > a user then it would be ambiguous and multiple queries would be
> > > > > > > > needed. But
> > > > > > > > > > it seems quite possible that the returned list of groups will
> > > > > > > > immediately
> > > > > > > > > > be used in a describeConsumerGroups query (for example, so
> > > > show a user
> > > > > > > > > > additional information about the groups of interest, for
> > > > example). In
> > > > > > > > that
> > > > > > > > > > case the grouping by state could be done on the descriptions,
> > > > and some
> > > > > > > > RPCs
> > > > > > > > > > could be avoided. It would also avoid the race inherent in
> > > > making
> > > > > > > > multiple
> > > > > > > > > > listConsumerGroups requests. So supporting a set of states
> > > > isn't
> > > > > > > > entirely
> > > > > > > > > > worthless and it wouldn't really add very much complexity.
> > > > > > > > > >
> > > > > > > > > > Kind regards,
> > > > > > > > > >
> > > > > > > > > > Tom
> > > > > > > > > >
> > > > > > > > > > On Mon, Oct 21, 2019 at 5:54 PM Mickael Maison <
> > > > > > > > mickael.maison@gmail.com>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Bump
> > > > > > > > > > > Now that the rush for 2.4.0 is ending I hope to get some
> > > > feedback
> > > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Sep 9, 2019 at 5:44 PM Mickael Maison <
> > > > > > > > mickael.maison@gmail.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > I have created a KIP to allow listing groups per state:
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-518%3A+Allow+listing+consumer+groups+per+state
> > > > > > > > > > > >
> > > > > > > > > > > > Have a look and let me know what you think.
> > > > > > > > > > > > Thank you
> > > > > > > > > > >
> > > > > > > >
> > > > >
> > > >
> >
Re: [DISCUSS] KIP-518: Allow listing consumer groups per state
Posted by Colin McCabe <cm...@apache.org>.
On Thu, Feb 13, 2020, at 08:24, Mickael Maison wrote:
> Hi Colin,
>
> > Can you please specify what the result is when a newer client tries to use
> > this on an older broker? Does that result in an
> > UnsupportedVersionException?
> Yes, in case this feature is not supported by the broker,
> UnsupportedVersionException should be raised. Your question made me
> look at it again and to properly handle all cases, I decided to bump
> the ListGroups API version. That way, we can't end up in cases where
> brokers know about the protocol version but not about the new optional
> field. I've updated the KIP accordingly
Yes, a protocol version bump seems fine here.
>
> > I would prefer an Optional in the Java API so that “show all groups” can
> > be EMPTY.
> I think it's nicer to provide a separate method in
> ListConsumerGroupsOptions to request all states without listing them
> rather than relying on empty Optional. I added a new method to the
> proposal: "inAnyState()".
I guess I don't feel that strongly about it. However, if we are going to rely on inAnyState() / inStates(...) then let's make sure that the latter throws an exception when getting an empty list as an argument (since it will do something other than the obvious behavior of listing nothing).
>
> Hi David,
>
> > 1. We already have the "--state" option in the command line tool which can
> > be used
> > with "--describe" and we will have "--states" which can be used with
> > "--list". I feel this
> > is going to be confusing for users. I wonder if we could combine both cases
> > to reduce
> > the confusion but I am not sure it would be better. What do you think?
> Yes absolutely, it makes sense to reuse this existing flag. Good catch!
>
> > 2. Regarding the output of the command line when "--states" is used, I
> > wonder if it
> > wouldn't be better to use a proper table with a header. We could use only
> > when
> > filters such as "--states" are used.
> Yes that's a good idea.
>
> I've updated the KIP to take all these suggestions into account.
> Thanks again for the feedback
Thanks, Mickael. It looks good.
best,
Colin
>
> On Mon, Feb 10, 2020 at 1:13 PM David Jacot <dj...@confluent.io> wrote:
> >
> > Hi Michael,
> >
> > Thank you for the updated KIP. I have few comments/questions:
> >
> > 1. We already have the "--state" option in the command line tool which can
> > be used
> > with "--describe" and we will have "--states" which can be used with
> > "--list". I feel this
> > is going to be confusing for users. I wonder if we could combine both cases
> > to reduce
> > the confusion but I am not sure it would be better. What do you think?
> >
> > 2. Regarding the output of the command line when "--states" is used, I
> > wonder if it
> > wouldn't be better to use a proper table with a header. We could use only
> > when
> > filters such as "--states" are used.
> >
> > Best,
> > David
> >
> > On Thu, Feb 6, 2020 at 10:44 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Hi Mickael,
> > >
> > > Can you please specify what the result is when a newer client tries to use
> > > this on an older broker? Does that result in an
> > > UnsupportedVersionException?
> > >
> > > I would prefer an Optional in the Java API so that “show all groups” can
> > > be EMPTY.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Mon, Jan 27, 2020, at 07:53, Mickael Maison wrote:
> > > > Hi David,
> > > >
> > > > Did that answer your questions? or do you have any further feedback?
> > > >
> > > > Thanks
> > > >
> > > > On Thu, Jan 16, 2020 at 4:11 PM Mickael Maison <mi...@gmail.com>
> > > wrote:
> > > > >
> > > > > Hi David,
> > > > >
> > > > > Thanks for taking a look.
> > > > > 1) Yes, updated
> > > > >
> > > > > 2) I had not considered that but indeed this would be useful if the
> > > > > request contained multiple states and would avoid doing another call.
> > > > > The ListGroups response already includes the group ProtocolType, so I
> > > > > guess we could add the State as well. The response will still be
> > > > > significantly smaller than DescribeGroups. With this change, one thing
> > > > > to note is that having Describe on the Cluster resource will allow
> > > > > retrieving the state of all groups. Currently retrieving the state of
> > > > > a group requires Describe on the Group.
> > > > >
> > > > > 3) Yes if ListGroups response includes the state, it makes sense to
> > > > > expose it via the command line tool and the AdminClient. With
> > > > > ConsumerGroupCommand, to avoid compatibility issues we can only print
> > > > > states when the --states flag is specified.
> > > > >
> > > > > I've updated the KIP accordingly.
> > > > >
> > > > > On Mon, Jan 13, 2020 at 12:20 PM David Jacot <dj...@confluent.io>
> > > wrote:
> > > > > >
> > > > > > Hi Michael,
> > > > > >
> > > > > > Please, excuse me for my late feedback. I've got a few
> > > questions/comments
> > > > > > while reviewing the KIP.
> > > > > >
> > > > > > 1. I would suggest to clearly state in the documentation of the
> > > state field
> > > > > > that omitting it or providing an empty list means "all".
> > > > > >
> > > > > > 2. Have you considered including the state in the response? The API
> > > allows
> > > > > > to search for multiple states so it could be
> > > > > > convenient to have the state in the response to let the user
> > > differentiate
> > > > > > the groups.
> > > > > >
> > > > > > 3. If 2. makes sense, I would suggest to also include it in the
> > > information
> > > > > > printed out by the ConsumerGroupCommand tool. Putting
> > > > > > myself in the shoes of an operator, I would like to see the state of
> > > each
> > > > > > group if I select specific states. Perhaps we could
> > > > > > use a table instead of the simple list used today. What do you think?
> > > > > >
> > > > > > Thanks for the KIP!
> > > > > >
> > > > > > Best,
> > > > > > David
> > > > > >
> > > > > > On Mon, Nov 11, 2019 at 12:40 PM Mickael Maison <
> > > mickael.maison@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi all,
> > > > > > >
> > > > > > > If there's no more feedback, I'll open a vote in the next few days.
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Nov 1, 2019 at 4:27 PM Mickael Maison <
> > > mickael.maison@gmail.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Tom,
> > > > > > > >
> > > > > > > > Thanks for taking a look at the KIP!
> > > > > > > > You are right, even if we serialize the field as a String, we
> > > should
> > > > > > > > use ConsumerGroupState in the API.
> > > > > > > > As suggested, I've also updated the API so a list of states is
> > > specified.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > >
> > > > > > > >
> > > > > > > > On Tue, Oct 22, 2019 at 10:03 AM Tom Bentley <
> > > tbentley@redhat.com>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi Mickael,
> > > > > > > > >
> > > > > > > > > Thanks for the KIP.
> > > > > > > > >
> > > > > > > > > The use of String to represent the desired state in the API
> > > seems less
> > > > > > > > > typesafe than would be ideal. Is there a reason not to use the
> > > existing
> > > > > > > > > ConsumerGroupState enum (even if the state is serialized as a
> > > String)?
> > > > > > > > >
> > > > > > > > > While you say that the list-of-names result from
> > > listConsumerGroups is
> > > > > > > a
> > > > > > > > > reason not to support supplying a set of desired states I
> > > don't find
> > > > > > > that
> > > > > > > > > argument entirely convincing. Sure, if the results are going
> > > to be
> > > > > > > shown to
> > > > > > > > > a user then it would be ambiguous and multiple queries would be
> > > > > > > needed. But
> > > > > > > > > it seems quite possible that the returned list of groups will
> > > > > > > immediately
> > > > > > > > > be used in a describeConsumerGroups query (for example, so
> > > show a user
> > > > > > > > > additional information about the groups of interest, for
> > > example). In
> > > > > > > that
> > > > > > > > > case the grouping by state could be done on the descriptions,
> > > and some
> > > > > > > RPCs
> > > > > > > > > could be avoided. It would also avoid the race inherent in
> > > making
> > > > > > > multiple
> > > > > > > > > listConsumerGroups requests. So supporting a set of states
> > > isn't
> > > > > > > entirely
> > > > > > > > > worthless and it wouldn't really add very much complexity.
> > > > > > > > >
> > > > > > > > > Kind regards,
> > > > > > > > >
> > > > > > > > > Tom
> > > > > > > > >
> > > > > > > > > On Mon, Oct 21, 2019 at 5:54 PM Mickael Maison <
> > > > > > > mickael.maison@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Bump
> > > > > > > > > > Now that the rush for 2.4.0 is ending I hope to get some
> > > feedback
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > > On Mon, Sep 9, 2019 at 5:44 PM Mickael Maison <
> > > > > > > mickael.maison@gmail.com>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > I have created a KIP to allow listing groups per state:
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-518%3A+Allow+listing+consumer+groups+per+state
> > > > > > > > > > >
> > > > > > > > > > > Have a look and let me know what you think.
> > > > > > > > > > > Thank you
> > > > > > > > > >
> > > > > > >
> > > >
> > >
>
Re: [DISCUSS] KIP-518: Allow listing consumer groups per state
Posted by Mickael Maison <mi...@gmail.com>.
Hi Colin,
> Can you please specify what the result is when a newer client tries to use
> this on an older broker? Does that result in an
> UnsupportedVersionException?
Yes, in case this feature is not supported by the broker,
UnsupportedVersionException should be raised. Your question made me
look at it again and to properly handle all cases, I decided to bump
the ListGroups API version. That way, we can't end up in cases where
brokers know about the protocol version but not about the new optional
field. I've updated the KIP accordingly
> I would prefer an Optional in the Java API so that “show all groups” can
> be EMPTY.
I think it's nicer to provide a separate method in
ListConsumerGroupsOptions to request all states without listing them
rather than relying on empty Optional. I added a new method to the
proposal: "inAnyState()".
Hi David,
> 1. We already have the "--state" option in the command line tool which can
> be used
> with "--describe" and we will have "--states" which can be used with
> "--list". I feel this
> is going to be confusing for users. I wonder if we could combine both cases
> to reduce
> the confusion but I am not sure it would be better. What do you think?
Yes absolutely, it makes sense to reuse this existing flag. Good catch!
> 2. Regarding the output of the command line when "--states" is used, I
> wonder if it
> wouldn't be better to use a proper table with a header. We could use only
> when
> filters such as "--states" are used.
Yes that's a good idea.
I've updated the KIP to take all these suggestions into account.
Thanks again for the feedback
On Mon, Feb 10, 2020 at 1:13 PM David Jacot <dj...@confluent.io> wrote:
>
> Hi Michael,
>
> Thank you for the updated KIP. I have few comments/questions:
>
> 1. We already have the "--state" option in the command line tool which can
> be used
> with "--describe" and we will have "--states" which can be used with
> "--list". I feel this
> is going to be confusing for users. I wonder if we could combine both cases
> to reduce
> the confusion but I am not sure it would be better. What do you think?
>
> 2. Regarding the output of the command line when "--states" is used, I
> wonder if it
> wouldn't be better to use a proper table with a header. We could use only
> when
> filters such as "--states" are used.
>
> Best,
> David
>
> On Thu, Feb 6, 2020 at 10:44 PM Colin McCabe <cm...@apache.org> wrote:
>
> > Hi Mickael,
> >
> > Can you please specify what the result is when a newer client tries to use
> > this on an older broker? Does that result in an
> > UnsupportedVersionException?
> >
> > I would prefer an Optional in the Java API so that “show all groups” can
> > be EMPTY.
> >
> > best,
> > Colin
> >
> >
> > On Mon, Jan 27, 2020, at 07:53, Mickael Maison wrote:
> > > Hi David,
> > >
> > > Did that answer your questions? or do you have any further feedback?
> > >
> > > Thanks
> > >
> > > On Thu, Jan 16, 2020 at 4:11 PM Mickael Maison <mi...@gmail.com>
> > wrote:
> > > >
> > > > Hi David,
> > > >
> > > > Thanks for taking a look.
> > > > 1) Yes, updated
> > > >
> > > > 2) I had not considered that but indeed this would be useful if the
> > > > request contained multiple states and would avoid doing another call.
> > > > The ListGroups response already includes the group ProtocolType, so I
> > > > guess we could add the State as well. The response will still be
> > > > significantly smaller than DescribeGroups. With this change, one thing
> > > > to note is that having Describe on the Cluster resource will allow
> > > > retrieving the state of all groups. Currently retrieving the state of
> > > > a group requires Describe on the Group.
> > > >
> > > > 3) Yes if ListGroups response includes the state, it makes sense to
> > > > expose it via the command line tool and the AdminClient. With
> > > > ConsumerGroupCommand, to avoid compatibility issues we can only print
> > > > states when the --states flag is specified.
> > > >
> > > > I've updated the KIP accordingly.
> > > >
> > > > On Mon, Jan 13, 2020 at 12:20 PM David Jacot <dj...@confluent.io>
> > wrote:
> > > > >
> > > > > Hi Michael,
> > > > >
> > > > > Please, excuse me for my late feedback. I've got a few
> > questions/comments
> > > > > while reviewing the KIP.
> > > > >
> > > > > 1. I would suggest to clearly state in the documentation of the
> > state field
> > > > > that omitting it or providing an empty list means "all".
> > > > >
> > > > > 2. Have you considered including the state in the response? The API
> > allows
> > > > > to search for multiple states so it could be
> > > > > convenient to have the state in the response to let the user
> > differentiate
> > > > > the groups.
> > > > >
> > > > > 3. If 2. makes sense, I would suggest to also include it in the
> > information
> > > > > printed out by the ConsumerGroupCommand tool. Putting
> > > > > myself in the shoes of an operator, I would like to see the state of
> > each
> > > > > group if I select specific states. Perhaps we could
> > > > > use a table instead of the simple list used today. What do you think?
> > > > >
> > > > > Thanks for the KIP!
> > > > >
> > > > > Best,
> > > > > David
> > > > >
> > > > > On Mon, Nov 11, 2019 at 12:40 PM Mickael Maison <
> > mickael.maison@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > If there's no more feedback, I'll open a vote in the next few days.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > >
> > > > > > On Fri, Nov 1, 2019 at 4:27 PM Mickael Maison <
> > mickael.maison@gmail.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > Thanks for taking a look at the KIP!
> > > > > > > You are right, even if we serialize the field as a String, we
> > should
> > > > > > > use ConsumerGroupState in the API.
> > > > > > > As suggested, I've also updated the API so a list of states is
> > specified.
> > > > > > >
> > > > > > > Regards,
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Oct 22, 2019 at 10:03 AM Tom Bentley <
> > tbentley@redhat.com>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Mickael,
> > > > > > > >
> > > > > > > > Thanks for the KIP.
> > > > > > > >
> > > > > > > > The use of String to represent the desired state in the API
> > seems less
> > > > > > > > typesafe than would be ideal. Is there a reason not to use the
> > existing
> > > > > > > > ConsumerGroupState enum (even if the state is serialized as a
> > String)?
> > > > > > > >
> > > > > > > > While you say that the list-of-names result from
> > listConsumerGroups is
> > > > > > a
> > > > > > > > reason not to support supplying a set of desired states I
> > don't find
> > > > > > that
> > > > > > > > argument entirely convincing. Sure, if the results are going
> > to be
> > > > > > shown to
> > > > > > > > a user then it would be ambiguous and multiple queries would be
> > > > > > needed. But
> > > > > > > > it seems quite possible that the returned list of groups will
> > > > > > immediately
> > > > > > > > be used in a describeConsumerGroups query (for example, so
> > show a user
> > > > > > > > additional information about the groups of interest, for
> > example). In
> > > > > > that
> > > > > > > > case the grouping by state could be done on the descriptions,
> > and some
> > > > > > RPCs
> > > > > > > > could be avoided. It would also avoid the race inherent in
> > making
> > > > > > multiple
> > > > > > > > listConsumerGroups requests. So supporting a set of states
> > isn't
> > > > > > entirely
> > > > > > > > worthless and it wouldn't really add very much complexity.
> > > > > > > >
> > > > > > > > Kind regards,
> > > > > > > >
> > > > > > > > Tom
> > > > > > > >
> > > > > > > > On Mon, Oct 21, 2019 at 5:54 PM Mickael Maison <
> > > > > > mickael.maison@gmail.com>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Bump
> > > > > > > > > Now that the rush for 2.4.0 is ending I hope to get some
> > feedback
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > On Mon, Sep 9, 2019 at 5:44 PM Mickael Maison <
> > > > > > mickael.maison@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > I have created a KIP to allow listing groups per state:
> > > > > > > > > >
> > > > > > > > >
> > > > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-518%3A+Allow+listing+consumer+groups+per+state
> > > > > > > > > >
> > > > > > > > > > Have a look and let me know what you think.
> > > > > > > > > > Thank you
> > > > > > > > >
> > > > > >
> > >
> >
Re: [DISCUSS] KIP-518: Allow listing consumer groups per state
Posted by David Jacot <dj...@confluent.io>.
Hi Michael,
Thank you for the updated KIP. I have few comments/questions:
1. We already have the "--state" option in the command line tool which can
be used
with "--describe" and we will have "--states" which can be used with
"--list". I feel this
is going to be confusing for users. I wonder if we could combine both cases
to reduce
the confusion but I am not sure it would be better. What do you think?
2. Regarding the output of the command line when "--states" is used, I
wonder if it
wouldn't be better to use a proper table with a header. We could use only
when
filters such as "--states" are used.
Best,
David
On Thu, Feb 6, 2020 at 10:44 PM Colin McCabe <cm...@apache.org> wrote:
> Hi Mickael,
>
> Can you please specify what the result is when a newer client tries to use
> this on an older broker? Does that result in an
> UnsupportedVersionException?
>
> I would prefer an Optional in the Java API so that “show all groups” can
> be EMPTY.
>
> best,
> Colin
>
>
> On Mon, Jan 27, 2020, at 07:53, Mickael Maison wrote:
> > Hi David,
> >
> > Did that answer your questions? or do you have any further feedback?
> >
> > Thanks
> >
> > On Thu, Jan 16, 2020 at 4:11 PM Mickael Maison <mi...@gmail.com>
> wrote:
> > >
> > > Hi David,
> > >
> > > Thanks for taking a look.
> > > 1) Yes, updated
> > >
> > > 2) I had not considered that but indeed this would be useful if the
> > > request contained multiple states and would avoid doing another call.
> > > The ListGroups response already includes the group ProtocolType, so I
> > > guess we could add the State as well. The response will still be
> > > significantly smaller than DescribeGroups. With this change, one thing
> > > to note is that having Describe on the Cluster resource will allow
> > > retrieving the state of all groups. Currently retrieving the state of
> > > a group requires Describe on the Group.
> > >
> > > 3) Yes if ListGroups response includes the state, it makes sense to
> > > expose it via the command line tool and the AdminClient. With
> > > ConsumerGroupCommand, to avoid compatibility issues we can only print
> > > states when the --states flag is specified.
> > >
> > > I've updated the KIP accordingly.
> > >
> > > On Mon, Jan 13, 2020 at 12:20 PM David Jacot <dj...@confluent.io>
> wrote:
> > > >
> > > > Hi Michael,
> > > >
> > > > Please, excuse me for my late feedback. I've got a few
> questions/comments
> > > > while reviewing the KIP.
> > > >
> > > > 1. I would suggest to clearly state in the documentation of the
> state field
> > > > that omitting it or providing an empty list means "all".
> > > >
> > > > 2. Have you considered including the state in the response? The API
> allows
> > > > to search for multiple states so it could be
> > > > convenient to have the state in the response to let the user
> differentiate
> > > > the groups.
> > > >
> > > > 3. If 2. makes sense, I would suggest to also include it in the
> information
> > > > printed out by the ConsumerGroupCommand tool. Putting
> > > > myself in the shoes of an operator, I would like to see the state of
> each
> > > > group if I select specific states. Perhaps we could
> > > > use a table instead of the simple list used today. What do you think?
> > > >
> > > > Thanks for the KIP!
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Mon, Nov 11, 2019 at 12:40 PM Mickael Maison <
> mickael.maison@gmail.com>
> > > > wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > If there's no more feedback, I'll open a vote in the next few days.
> > > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > On Fri, Nov 1, 2019 at 4:27 PM Mickael Maison <
> mickael.maison@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > Hi Tom,
> > > > > >
> > > > > > Thanks for taking a look at the KIP!
> > > > > > You are right, even if we serialize the field as a String, we
> should
> > > > > > use ConsumerGroupState in the API.
> > > > > > As suggested, I've also updated the API so a list of states is
> specified.
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > >
> > > > > > On Tue, Oct 22, 2019 at 10:03 AM Tom Bentley <
> tbentley@redhat.com>
> > > > > wrote:
> > > > > > >
> > > > > > > Hi Mickael,
> > > > > > >
> > > > > > > Thanks for the KIP.
> > > > > > >
> > > > > > > The use of String to represent the desired state in the API
> seems less
> > > > > > > typesafe than would be ideal. Is there a reason not to use the
> existing
> > > > > > > ConsumerGroupState enum (even if the state is serialized as a
> String)?
> > > > > > >
> > > > > > > While you say that the list-of-names result from
> listConsumerGroups is
> > > > > a
> > > > > > > reason not to support supplying a set of desired states I
> don't find
> > > > > that
> > > > > > > argument entirely convincing. Sure, if the results are going
> to be
> > > > > shown to
> > > > > > > a user then it would be ambiguous and multiple queries would be
> > > > > needed. But
> > > > > > > it seems quite possible that the returned list of groups will
> > > > > immediately
> > > > > > > be used in a describeConsumerGroups query (for example, so
> show a user
> > > > > > > additional information about the groups of interest, for
> example). In
> > > > > that
> > > > > > > case the grouping by state could be done on the descriptions,
> and some
> > > > > RPCs
> > > > > > > could be avoided. It would also avoid the race inherent in
> making
> > > > > multiple
> > > > > > > listConsumerGroups requests. So supporting a set of states
> isn't
> > > > > entirely
> > > > > > > worthless and it wouldn't really add very much complexity.
> > > > > > >
> > > > > > > Kind regards,
> > > > > > >
> > > > > > > Tom
> > > > > > >
> > > > > > > On Mon, Oct 21, 2019 at 5:54 PM Mickael Maison <
> > > > > mickael.maison@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Bump
> > > > > > > > Now that the rush for 2.4.0 is ending I hope to get some
> feedback
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > On Mon, Sep 9, 2019 at 5:44 PM Mickael Maison <
> > > > > mickael.maison@gmail.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > I have created a KIP to allow listing groups per state:
> > > > > > > > >
> > > > > > > >
> > > > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-518%3A+Allow+listing+consumer+groups+per+state
> > > > > > > > >
> > > > > > > > > Have a look and let me know what you think.
> > > > > > > > > Thank you
> > > > > > > >
> > > > >
> >
>