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