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