Hi Colin, > 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? Yes, in case this feature is not supported by the broker, UnsupportedVersionException should be raised. Your question made me look at it again and to properly handle all cases, I decided to bump the ListGroups API version. That way, we can't end up in cases where brokers know about the protocol version but not about the new optional field. I've updated the KIP accordingly
> I would prefer an Optional in the Java API so that “show all groups” can > be EMPTY. I think it's nicer to provide a separate method in ListConsumerGroupsOptions to request all states without listing them rather than relying on empty Optional. I added a new method to the proposal: "inAnyState()". Hi David, > 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? Yes absolutely, it makes sense to reuse this existing flag. Good catch! > 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. Yes that's a good idea. I've updated the KIP to take all these suggestions into account. Thanks again for the feedback On Mon, Feb 10, 2020 at 1:13 PM David Jacot <dja...@confluent.io> wrote: > > 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 > > > > > > > > > > > > > > > > > > > >
