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