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