Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
Hi folks, We have made a small change to this KIP for the Apache Kafka 3.2 release. When we tried to use this in Kafka Streams, we noted a significant degradation of the performances, see [1]. It is not clear whether the prefixing is the root cause of the issue or not. To be on the safe side, we have removed the prefixing. It does not bring much anyway as we are still able to distinguish a custom reason from the default one. [1] https://github.com/apache/kafka/pull/11873 Best, David On Wed, Dec 1, 2021 at 2:17 PM David Jacot wrote: > > Hi all, > > With 4 binding +1 votes (Gwen, Mickael, Tom, Bill) and 1 > non-binding +1 vote (Luke), the vote passes. > > Thanks everyone! > > Best, > David > > On Fri, Nov 26, 2021 at 4:43 PM Bill Bejeck wrote: > > > > Thanks for the KIP, David this seems like it will be very helpful. > > > > +1(binding) > > > > -Bill > > > > On Thu, Nov 25, 2021 at 10:02 AM David Jacot > > wrote: > > > > > Hi Tom, > > > > > > I do agree with you. For context, this is the current reason/message > > > logged > > > by the consumer when enforceRebalance is called so I just kept it as it > > > is. > > > I guess that we can revise it during the implementation. > > > > > > Thanks, > > > David > > > > > > On Thu, Nov 25, 2021 at 3:52 PM Tom Bentley wrote: > > > > > > > > Thanks for the KIP David. > > > > > > > > It's a very trivial point, but I did wonder whether "caused" or > > > "requested" > > > > might be a better word than "enforced" in the default messages. > > > > > > > > In any case, +1 (binding). > > > > > > > > Kind regards, > > > > > > > > Tom > > > > > > > > > > > > > > > > On Thu, Nov 25, 2021 at 2:36 PM David Jacot > > > > > > > wrote: > > > > > > > > > Thanks, Mickael. I have removed it. > > > > > > > > > > On Thu, Nov 25, 2021 at 3:22 PM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > > wrote: > > > > > > > > > > > > +1 (binding) > > > > > > Thanks for the KIP > > > > > > > > > > > > Just a small suggestion: in other Options classes we don't seem to > > > > > > prefix setters with "set", so I think we could use void reason(final > > > > > > String reason). > > > > > > > > > > > > > > > > > > On Thu, Nov 25, 2021 at 1:48 PM Luke Chen wrote: > > > > > > > > > > > > > > Hi David, > > > > > > > Thanks for the KIP! > > > > > > > It is good to have the joinGroup/leaveGroup reason sent to brokers > > > for > > > > > > > better troubleshooting. > > > > > > > > > > > > > > +1 (non-binding) > > > > > > > > > > > > > > Thank you. > > > > > > > Luke > > > > > > > > > > > > > > On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > Thanks for driving David. Super useful. > > > > > > > > > > > > > > > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > > > > > > > I'd like to start a vote on KIP-800: Add reason to > > > > > LeaveGroupRequest. > > > > > > > > > > > > > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > > > > > > > > > > > > > Please let me know what you think. > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Gwen Shapira > > > > > > > > Engineering Manager | Confluent > > > > > > > > 650.450.2760 | @gwenshap > > > > > > > > Follow us: Twitter | blog > > > > > > > > > > > > > > > > > > > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
Hi all, With 4 binding +1 votes (Gwen, Mickael, Tom, Bill) and 1 non-binding +1 vote (Luke), the vote passes. Thanks everyone! Best, David On Fri, Nov 26, 2021 at 4:43 PM Bill Bejeck wrote: > > Thanks for the KIP, David this seems like it will be very helpful. > > +1(binding) > > -Bill > > On Thu, Nov 25, 2021 at 10:02 AM David Jacot > wrote: > > > Hi Tom, > > > > I do agree with you. For context, this is the current reason/message logged > > by the consumer when enforceRebalance is called so I just kept it as it is. > > I guess that we can revise it during the implementation. > > > > Thanks, > > David > > > > On Thu, Nov 25, 2021 at 3:52 PM Tom Bentley wrote: > > > > > > Thanks for the KIP David. > > > > > > It's a very trivial point, but I did wonder whether "caused" or > > "requested" > > > might be a better word than "enforced" in the default messages. > > > > > > In any case, +1 (binding). > > > > > > Kind regards, > > > > > > Tom > > > > > > > > > > > > On Thu, Nov 25, 2021 at 2:36 PM David Jacot > > > > > wrote: > > > > > > > Thanks, Mickael. I have removed it. > > > > > > > > On Thu, Nov 25, 2021 at 3:22 PM Mickael Maison < > > mickael.mai...@gmail.com> > > > > wrote: > > > > > > > > > > +1 (binding) > > > > > Thanks for the KIP > > > > > > > > > > Just a small suggestion: in other Options classes we don't seem to > > > > > prefix setters with "set", so I think we could use void reason(final > > > > > String reason). > > > > > > > > > > > > > > > On Thu, Nov 25, 2021 at 1:48 PM Luke Chen wrote: > > > > > > > > > > > > Hi David, > > > > > > Thanks for the KIP! > > > > > > It is good to have the joinGroup/leaveGroup reason sent to brokers > > for > > > > > > better troubleshooting. > > > > > > > > > > > > +1 (non-binding) > > > > > > > > > > > > Thank you. > > > > > > Luke > > > > > > > > > > > > On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira > > > > > > > > > > > > wrote: > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > Thanks for driving David. Super useful. > > > > > > > > > > > > > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > > > > > I'd like to start a vote on KIP-800: Add reason to > > > > LeaveGroupRequest. > > > > > > > > > > > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > > > > > > > > > > > Please let me know what you think. > > > > > > > > > > > > > > > > Best, > > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Gwen Shapira > > > > > > > Engineering Manager | Confluent > > > > > > > 650.450.2760 | @gwenshap > > > > > > > Follow us: Twitter | blog > > > > > > > > > > > > > > > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
Thanks for the KIP, David this seems like it will be very helpful. +1(binding) -Bill On Thu, Nov 25, 2021 at 10:02 AM David Jacot wrote: > Hi Tom, > > I do agree with you. For context, this is the current reason/message logged > by the consumer when enforceRebalance is called so I just kept it as it is. > I guess that we can revise it during the implementation. > > Thanks, > David > > On Thu, Nov 25, 2021 at 3:52 PM Tom Bentley wrote: > > > > Thanks for the KIP David. > > > > It's a very trivial point, but I did wonder whether "caused" or > "requested" > > might be a better word than "enforced" in the default messages. > > > > In any case, +1 (binding). > > > > Kind regards, > > > > Tom > > > > > > > > On Thu, Nov 25, 2021 at 2:36 PM David Jacot > > > wrote: > > > > > Thanks, Mickael. I have removed it. > > > > > > On Thu, Nov 25, 2021 at 3:22 PM Mickael Maison < > mickael.mai...@gmail.com> > > > wrote: > > > > > > > > +1 (binding) > > > > Thanks for the KIP > > > > > > > > Just a small suggestion: in other Options classes we don't seem to > > > > prefix setters with "set", so I think we could use void reason(final > > > > String reason). > > > > > > > > > > > > On Thu, Nov 25, 2021 at 1:48 PM Luke Chen wrote: > > > > > > > > > > Hi David, > > > > > Thanks for the KIP! > > > > > It is good to have the joinGroup/leaveGroup reason sent to brokers > for > > > > > better troubleshooting. > > > > > > > > > > +1 (non-binding) > > > > > > > > > > Thank you. > > > > > Luke > > > > > > > > > > On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira > > > > > > > > > wrote: > > > > > > > > > > > +1 > > > > > > > > > > > > Thanks for driving David. Super useful. > > > > > > > > > > > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > > > > > > > > > wrote: > > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > > > I'd like to start a vote on KIP-800: Add reason to > > > LeaveGroupRequest. > > > > > > > > > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > > > > > > > > > Please let me know what you think. > > > > > > > > > > > > > > Best, > > > > > > > David > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Gwen Shapira > > > > > > Engineering Manager | Confluent > > > > > > 650.450.2760 | @gwenshap > > > > > > Follow us: Twitter | blog > > > > > > > > > > > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
Hi Tom, I do agree with you. For context, this is the current reason/message logged by the consumer when enforceRebalance is called so I just kept it as it is. I guess that we can revise it during the implementation. Thanks, David On Thu, Nov 25, 2021 at 3:52 PM Tom Bentley wrote: > > Thanks for the KIP David. > > It's a very trivial point, but I did wonder whether "caused" or "requested" > might be a better word than "enforced" in the default messages. > > In any case, +1 (binding). > > Kind regards, > > Tom > > > > On Thu, Nov 25, 2021 at 2:36 PM David Jacot > wrote: > > > Thanks, Mickael. I have removed it. > > > > On Thu, Nov 25, 2021 at 3:22 PM Mickael Maison > > wrote: > > > > > > +1 (binding) > > > Thanks for the KIP > > > > > > Just a small suggestion: in other Options classes we don't seem to > > > prefix setters with "set", so I think we could use void reason(final > > > String reason). > > > > > > > > > On Thu, Nov 25, 2021 at 1:48 PM Luke Chen wrote: > > > > > > > > Hi David, > > > > Thanks for the KIP! > > > > It is good to have the joinGroup/leaveGroup reason sent to brokers for > > > > better troubleshooting. > > > > > > > > +1 (non-binding) > > > > > > > > Thank you. > > > > Luke > > > > > > > > On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira > > > > > > wrote: > > > > > > > > > +1 > > > > > > > > > > Thanks for driving David. Super useful. > > > > > > > > > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > > > > > > > wrote: > > > > > > > > > > > Hi folks, > > > > > > > > > > > > I'd like to start a vote on KIP-800: Add reason to > > LeaveGroupRequest. > > > > > > > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > > > > > > > Please let me know what you think. > > > > > > > > > > > > Best, > > > > > > David > > > > > > > > > > > > > > > > > > > > > -- > > > > > Gwen Shapira > > > > > Engineering Manager | Confluent > > > > > 650.450.2760 | @gwenshap > > > > > Follow us: Twitter | blog > > > > > > > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
Thanks for the KIP David. It's a very trivial point, but I did wonder whether "caused" or "requested" might be a better word than "enforced" in the default messages. In any case, +1 (binding). Kind regards, Tom On Thu, Nov 25, 2021 at 2:36 PM David Jacot wrote: > Thanks, Mickael. I have removed it. > > On Thu, Nov 25, 2021 at 3:22 PM Mickael Maison > wrote: > > > > +1 (binding) > > Thanks for the KIP > > > > Just a small suggestion: in other Options classes we don't seem to > > prefix setters with "set", so I think we could use void reason(final > > String reason). > > > > > > On Thu, Nov 25, 2021 at 1:48 PM Luke Chen wrote: > > > > > > Hi David, > > > Thanks for the KIP! > > > It is good to have the joinGroup/leaveGroup reason sent to brokers for > > > better troubleshooting. > > > > > > +1 (non-binding) > > > > > > Thank you. > > > Luke > > > > > > On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira > > > > wrote: > > > > > > > +1 > > > > > > > > Thanks for driving David. Super useful. > > > > > > > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > > > > > wrote: > > > > > > > > > Hi folks, > > > > > > > > > > I'd like to start a vote on KIP-800: Add reason to > LeaveGroupRequest. > > > > > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > > > > > Please let me know what you think. > > > > > > > > > > Best, > > > > > David > > > > > > > > > > > > > > > > > -- > > > > Gwen Shapira > > > > Engineering Manager | Confluent > > > > 650.450.2760 | @gwenshap > > > > Follow us: Twitter | blog > > > > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
Thanks, Mickael. I have removed it. On Thu, Nov 25, 2021 at 3:22 PM Mickael Maison wrote: > > +1 (binding) > Thanks for the KIP > > Just a small suggestion: in other Options classes we don't seem to > prefix setters with "set", so I think we could use void reason(final > String reason). > > > On Thu, Nov 25, 2021 at 1:48 PM Luke Chen wrote: > > > > Hi David, > > Thanks for the KIP! > > It is good to have the joinGroup/leaveGroup reason sent to brokers for > > better troubleshooting. > > > > +1 (non-binding) > > > > Thank you. > > Luke > > > > On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira > > wrote: > > > > > +1 > > > > > > Thanks for driving David. Super useful. > > > > > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > > > wrote: > > > > > > > Hi folks, > > > > > > > > I'd like to start a vote on KIP-800: Add reason to LeaveGroupRequest. > > > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > > > Please let me know what you think. > > > > > > > > Best, > > > > David > > > > > > > > > > > > > -- > > > Gwen Shapira > > > Engineering Manager | Confluent > > > 650.450.2760 | @gwenshap > > > Follow us: Twitter | blog > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
+1 (binding) Thanks for the KIP Just a small suggestion: in other Options classes we don't seem to prefix setters with "set", so I think we could use void reason(final String reason). On Thu, Nov 25, 2021 at 1:48 PM Luke Chen wrote: > > Hi David, > Thanks for the KIP! > It is good to have the joinGroup/leaveGroup reason sent to brokers for > better troubleshooting. > > +1 (non-binding) > > Thank you. > Luke > > On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira > wrote: > > > +1 > > > > Thanks for driving David. Super useful. > > > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > > wrote: > > > > > Hi folks, > > > > > > I'd like to start a vote on KIP-800: Add reason to LeaveGroupRequest. > > > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > > > Please let me know what you think. > > > > > > Best, > > > David > > > > > > > > > -- > > Gwen Shapira > > Engineering Manager | Confluent > > 650.450.2760 | @gwenshap > > Follow us: Twitter | blog > >
Re: KIP-800: Add reason to LeaveGroupRequest
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 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 wrote: > > > > Hi David, > > Thanks for the update. > > > > Just voted! :) > > > > Thank you. > > Luke > > > > On Thu, Nov 25, 2021 at 8:42 PM David Jacot > > wrote: > > > > > Hi Luke, > > > > > > Good point. I have renamed the KIP. > > > > > > Thanks, > > > David > > > > > > On Thu, Nov 25, 2021 at 4:28 AM Luke Chen 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 > > > > > > > > > > > wrote: > > > > > > > > > I have updated the KIP. > > > > > > > > > > Best, > > > > > David > > > > > > > > > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot > > > 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 > > > > > > 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
Re: KIP-800: Add reason to LeaveGroupRequest
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 wrote: > > Hi David, > Thanks for the update. > > Just voted! :) > > Thank you. > Luke > > On Thu, Nov 25, 2021 at 8:42 PM David Jacot > wrote: > > > Hi Luke, > > > > Good point. I have renamed the KIP. > > > > Thanks, > > David > > > > On Thu, Nov 25, 2021 at 4:28 AM Luke Chen 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 > > > > > wrote: > > > > > > > I have updated the KIP. > > > > > > > > Best, > > > > David > > > > > > > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot > > 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 > > > > > 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 > > > > wrote: > > > > > > > > > > > > > Thanks David, this is a worthwhile improvement. Quick question, > > why > > > > did we > > > > > > > pick a tagged field here? > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > On
Re: KIP-800: Add reason to LeaveGroupRequest
Hi David, Thanks for the update. Just voted! :) Thank you. Luke On Thu, Nov 25, 2021 at 8:42 PM David Jacot wrote: > Hi Luke, > > Good point. I have renamed the KIP. > > Thanks, > David > > On Thu, Nov 25, 2021 at 4:28 AM Luke Chen 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 > > > wrote: > > > > > I have updated the KIP. > > > > > > Best, > > > David > > > > > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot > 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 > > > > 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 > > > 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 > > > > > > > > > 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 > > > > > > > > > > > > > > > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
Hi David, Thanks for the KIP! It is good to have the joinGroup/leaveGroup reason sent to brokers for better troubleshooting. +1 (non-binding) Thank you. Luke On Thu, Nov 25, 2021 at 8:14 AM Gwen Shapira wrote: > +1 > > Thanks for driving David. Super useful. > > On Wed, Nov 24, 2021 at 8:53 AM David Jacot > wrote: > > > Hi folks, > > > > I'd like to start a vote on KIP-800: Add reason to LeaveGroupRequest. > > > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > > > Please let me know what you think. > > > > Best, > > David > > > > > -- > Gwen Shapira > Engineering Manager | Confluent > 650.450.2760 | @gwenshap > Follow us: Twitter | blog >
Re: KIP-800: Add reason to LeaveGroupRequest
Hi Luke, Good point. I have renamed the KIP. Thanks, David On Thu, Nov 25, 2021 at 4:28 AM Luke Chen 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 > wrote: > > > I have updated the KIP. > > > > Best, > > David > > > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot 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 > > > 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 > > 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 > > > > > > > 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 > > > > > > > > > > > > >
Re: KIP-800: Add reason to LeaveGroupRequest
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 wrote: > I have updated the KIP. > > Best, > David > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot 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 > > 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 > 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 > > > > > 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 > > > > > > > > > >
Re: [VOTE] KIP-800: Add reason to LeaveGroupRequest
+1 Thanks for driving David. Super useful. On Wed, Nov 24, 2021 at 8:53 AM David Jacot wrote: > Hi folks, > > I'd like to start a vote on KIP-800: Add reason to LeaveGroupRequest. > > KIP: https://cwiki.apache.org/confluence/x/eYyqCw > > Please let me know what you think. > > Best, > David > -- Gwen Shapira Engineering Manager | Confluent 650.450.2760 | @gwenshap Follow us: Twitter | blog
[VOTE] KIP-800: Add reason to LeaveGroupRequest
Hi folks, I'd like to start a vote on KIP-800: Add reason to LeaveGroupRequest. KIP: https://cwiki.apache.org/confluence/x/eYyqCw Please let me know what you think. Best, David
Re: KIP-800: Add reason to LeaveGroupRequest
I have updated the KIP. Best, David On Fri, Nov 19, 2021 at 3:00 PM David Jacot 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 > 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 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 > > > 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 > > > > > > >
Re: KIP-800: Add reason to LeaveGroupRequest
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 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 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 > > 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 > > > > >
Re: KIP-800: Add reason to LeaveGroupRequest
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 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 > 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 > > >
Re: KIP-800: Add reason to LeaveGroupRequest
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 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 >
Re: KIP-800: Add reason to LeaveGroupRequest
Hi David, Thanks for the KIP. It makes sense to me. Some comments: 1. Should we bump the `LeaveGroupRequest` protocol version to 5? 2. the description in the new field: "about": "The reason" -> "about": "The reason why the member left the group" 3. For the `removeMembersFromConsumerGroup` case, do you think we can add a new "reason" field in `RemoveMembersFromConsumerGroupOptions`, and default to "the consumer was removed by an admin." as you proposed? I think when getting the reason "removed by an admin" is not quite helpful for troubleshooting, right? So, with the new field, it allows users to be able to customize the reason if necessary. for troubleshooting, What do you think? Thank you. Luke On Fri, Nov 12, 2021 at 12:32 AM David Jacot 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 >
KIP-800: Add reason to LeaveGroupRequest
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
