Thanks Colin, > I guess I don't feel that strongly about it. However, if we are going to > rely on inAnyState() / inStates(...) then let's make sure that the latter > throws an exception when getting an empty list as an argument (since it will > do something other than the obvious behavior of listing nothing).
Yes I've made that explicit in the KIP On Wed, Feb 19, 2020 at 7:21 PM Colin McCabe <cmcc...@apache.org> wrote: > > On Thu, Feb 13, 2020, at 08:24, Mickael Maison wrote: > > 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 > > Yes, a protocol version bump seems fine here. > > > > > > 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()". > > I guess I don't feel that strongly about it. However, if we are going to > rely on inAnyState() / inStates(...) then let's make sure that the latter > throws an exception when getting an empty list as an argument (since it will > do something other than the obvious behavior of listing nothing). > > > > > 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 > > Thanks, Mickael. It looks good. > > best, > Colin > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
