You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by David Jacot <dj...@confluent.io.INVALID> on 2021/11/11 16:32:00 UTC
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
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by David Jacot <dj...@confluent.io.INVALID>.
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 <mi...@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 <sh...@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 <dj...@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 <sh...@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 <djacot@confluent.io.invalid
> > > >
> > > > wrote:
> > > >
> > > > > I have updated the KIP.
> > > > >
> > > > > Best,
> > > > > David
> > > > >
> > > > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot <dj...@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
> > > > > > <so...@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 <is...@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
> > > > > <dj...@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
> > > > > > > > >
> > > > > > > >
> > > > >
> > >
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by Mickael Maison <mi...@gmail.com>.
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 <sh...@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 <dj...@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 <sh...@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 <djacot@confluent.io.invalid
> > >
> > > wrote:
> > >
> > > > I have updated the KIP.
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot <dj...@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
> > > > > <so...@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 <is...@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
> > > > <dj...@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
> > > > > > > >
> > > > > > >
> > > >
> >
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by Luke Chen <sh...@gmail.com>.
Hi David,
Thanks for the update.
Just voted! :)
Thank you.
Luke
On Thu, Nov 25, 2021 at 8:42 PM David Jacot <dj...@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 <sh...@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 <djacot@confluent.io.invalid
> >
> > wrote:
> >
> > > I have updated the KIP.
> > >
> > > Best,
> > > David
> > >
> > > On Fri, Nov 19, 2021 at 3:00 PM David Jacot <dj...@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
> > > > <so...@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 <is...@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
> > > <dj...@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
> > > > > > >
> > > > > >
> > >
>
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by David Jacot <dj...@confluent.io.INVALID>.
Hi Luke,
Good point. I have renamed the KIP.
Thanks,
David
On Thu, Nov 25, 2021 at 4:28 AM Luke Chen <sh...@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 <dj...@confluent.io.invalid>
> wrote:
>
> > I have updated the KIP.
> >
> > Best,
> > David
> >
> > On Fri, Nov 19, 2021 at 3:00 PM David Jacot <dj...@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
> > > <so...@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 <is...@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
> > <dj...@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
> > > > > >
> > > > >
> >
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by Luke Chen <sh...@gmail.com>.
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 <dj...@confluent.io.invalid>
wrote:
> I have updated the KIP.
>
> Best,
> David
>
> On Fri, Nov 19, 2021 at 3:00 PM David Jacot <dj...@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
> > <so...@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 <is...@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
> <dj...@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
> > > > >
> > > >
>
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by David Jacot <dj...@confluent.io.INVALID>.
I have updated the KIP.
Best,
David
On Fri, Nov 19, 2021 at 3:00 PM David Jacot <dj...@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
> <so...@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 <is...@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 <dj...@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
> > > >
> > >
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by David Jacot <dj...@confluent.io.INVALID>.
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
<so...@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 <is...@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 <dj...@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
> > >
> >
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by Sophie Blee-Goldman <so...@confluent.io.INVALID>.
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 <is...@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 <dj...@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
> >
>
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by Ismael Juma <is...@juma.me.uk>.
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 <dj...@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
>
Re: KIP-800: Add reason to LeaveGroupRequest
Posted by Luke Chen <sh...@gmail.com>.
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 <dj...@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
>