Hi Mickael, Yes, I briefly considered it. I was trying to stay inline with the `enforceRebalance` method. On second thought, it was likely the wrong way to look at it as we already have the optional members in `RemoveMembersFromConsumerGroupOptions` so adding another optional `reason` field there makes more sense.
I'll update the KIP. Thanks, David On Thu, Nov 25, 2021 at 2:31 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > > Hi David, > > Thanks for the KIP, it looks like a useful addition. > > You propose adding a new method removeMembersFromConsumerGroup(String, > String, RemoveMembersFromConsumerGroupOptions) to Admin. > Have you considered keeping the existing method and instead add the > reason to RemoveMembersFromConsumerGroupOptions? > > Thanks, > Mickael > > On Thu, Nov 25, 2021 at 1:48 PM Luke Chen <show...@gmail.com> wrote: > > > > Hi David, > > Thanks for the update. > > > > Just voted! :) > > > > Thank you. > > Luke > > > > On Thu, Nov 25, 2021 at 8:42 PM David Jacot <dja...@confluent.io.invalid> > > wrote: > > > > > Hi Luke, > > > > > > Good point. I have renamed the KIP. > > > > > > Thanks, > > > David > > > > > > On Thu, Nov 25, 2021 at 4:28 AM Luke Chen <show...@gmail.com> wrote: > > > > > > > > Hi David, > > > > Sorry for the late reply. > > > > Thanks for the update. It looks good now. > > > > I love the idea to add joinGroup request reason, as well as the > > > > `enforceRebalance` API! > > > > > > > > One minor comment, since we extended the original KIP from leaveGroup to > > > > joinGroup request, Could you please update the KIP title and also the > > > > motivation section? > > > > It makes the KIP much clearer for future reference. > > > > > > > > Otherwise, LGTM! > > > > > > > > Thank you. > > > > Luke > > > > > > > > On Fri, Nov 19, 2021 at 11:23 PM David Jacot > > > > <dja...@confluent.io.invalid > > > > > > > > wrote: > > > > > > > > > I have updated the KIP. > > > > > > > > > > Best, > > > > > David > > > > > > > > > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot <dja...@confluent.io> > > > wrote: > > > > > > > > > > > > Thank you all for your feedback. Let me address all your points > > > below. > > > > > > > > > > > > Luke, > > > > > > 1. I use a tag field so bumping the request version is not > > > necessary. In > > > > > > this case, using a tag field does not seem to be the best approach > > > > > > so > > > > > > I will use a regular one and bump the version. > > > > > > 2. Sounds good. I will fix that comment. > > > > > > 3. That is a good question. My intent was to avoid getting weird or > > > > > cryptic > > > > > > reasons logged on the broker so I thought that having a standard one > > > is > > > > > > better. As Sophie suggested something similar for the > > > `enforceRebalance` > > > > > > API, we could do it for both, I suppose. > > > > > > > > > > > > Ismael, > > > > > > 1. That's a good point. I chose to use a tag field to avoid having > > > > > > to > > > > > bump > > > > > > the request version. In this particular case, it seems that bumping > > > the > > > > > > version does not cost much so it is perhaps better. I will change > > > this. > > > > > > > > > > > > Sophie, > > > > > > 1. That's a pretty good idea, thanks. Let me update the KIP to > > > include > > > > > > a reason in the JoinGroup request. Regarding the Consumer API, do > > > > > > you think that there is value for KStreams to expose the possibility > > > to > > > > > > provide a reason? Otherwise, it might be better to use a default > > > > > > reason in this case. > > > > > > 2. I don't fully get your point about providing the reason to the > > > group > > > > > > leader assignor on the client. Do you think that we should propagate > > > > > > it to all the consumers or to the leader as well? The user usually > > > has > > > > > > access to all its client logs so I am not sure that it is really > > > > > necessary. > > > > > > Could you elaborate? > > > > > > > > > > > > I will update the KIP soon. > > > > > > > > > > > > Best, > > > > > > David > > > > > > > > > > > > On Sat, Nov 13, 2021 at 6:00 AM Sophie Blee-Goldman > > > > > > <sop...@confluent.io.invalid> wrote: > > > > > > > > > > > > > > This sounds great, thanks David. > > > > > > > > > > > > > > One thought: what do you think about doing something similar for > > > the > > > > > > > JoinGroup request? > > > > > > > > > > > > > > When you only have broker logs and not client logs, it's somewhere > > > > > between > > > > > > > challenging and > > > > > > > impossible to determine the reason for a rebalance that was > > > triggered > > > > > > > explicitly by the client or > > > > > > > even the user. For example, when a followup rebalance is requested > > > to > > > > > > > assign the revoked > > > > > > > partitions after a cooperative rebalance. Or any of the many > > > reasons we > > > > > > > trigger a rebalance > > > > > > > in Kafka Streams, via the #enforceRebalance API. > > > > > > > > > > > > > > Perhaps we could add a parameter to that method as such: > > > > > > > > > > > > > > public void enforceRebalance(final String reason); > > > > > > > > > > > > > > Then we can add that to the JoinGroup request/ConsumerProtocol. > > > > > > > Not > > > > > only > > > > > > > would it help to > > > > > > > log this reason on the broker side, the information about who > > > > > requested the > > > > > > > rebalance and why > > > > > > > could be extremely useful to have available to the group > > > > > leader/partition > > > > > > > assignor on the client > > > > > > > side. > > > > > > > > > > > > > > Cheers, > > > > > > > Sophie > > > > > > > > > > > > > > On Fri, Nov 12, 2021 at 10:05 AM Ismael Juma <ism...@juma.me.uk> > > > > > wrote: > > > > > > > > > > > > > > > Thanks David, this is a worthwhile improvement. Quick question, > > > why > > > > > did we > > > > > > > > pick a tagged field here? > > > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > On Thu, Nov 11, 2021, 8:32 AM David Jacot > > > > > <dja...@confluent.io.invalid> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > > > > > > > I'd like to discuss this very small KIP which proposes to add > > > > > > > > > a > > > > > reason > > > > > > > > > field > > > > > > > > > to the LeaveGroupRequest in order to let the broker know why a > > > > > member > > > > > > > > > left the group. This would be really handy for administrators. > > > > > > > > > > > > > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > >
