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