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 <cmcc...@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.mai...@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 <dja...@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.mai...@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.mai...@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 < > tbent...@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.mai...@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.mai...@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 > > > > > > > > > > > > > > > >
