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