Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-25 Thread David Jacot
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

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-25 Thread Mickael Maison
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 RemoveMembersFromConsumerGro

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-25 Thread Luke Chen
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. > > Th

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-25 Thread David Jacot
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 m

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-24 Thread Luke Chen
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 an

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-19 Thread David Jacot
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 b

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-19 Thread David Jacot
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

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-12 Thread Sophie Blee-Goldman
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 t

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-12 Thread Ismael Juma
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

Re: KIP-800: Add reason to LeaveGroupRequest

2021-11-11 Thread Luke Chen
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