You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by feyman2009 <fe...@aliyun.com.INVALID> on 2020/02/15 05:28:55 UTC
回复：[Discuss] KIP-571: Add option to force remove members in StreamsResetter
Hi, Boyang
You can call me Feyman :)
Thanks for your quick reply with great advices!
I have updated the KIP-571 , would you mind to see if it looks good ?
Thanks !
------------------------------------------------------------------
发件人：Boyang Chen <re...@gmail.com>
发送时间：2020年2月14日(星期五) 08:35
收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Thanks for driving this change Feyman! Hope this is a good name to call you :)
The motivation of the KIP looks good, and I have a couple of initial thoughts:
1. I guess the reason to use setters instead of adding a new constructor to MemberToRemove class is because we have two String members. Could you point that out upfront so that people are not asking why not adding new constructor?
2. KIP discussion usually focuses on the public side changes, so you don't need to copy-paste the entire class. Just the new APIs you are adding should be suffice
3. Add the description of new flag inside Public API change, whose name could be simplified as `--force` and people would just read the description. An edge case I could think of is that some ongoing applications are not closed when the reset tool applies, which causes more unexpected rebalances. So it's important to warn users to use the flag wisely and be responsible to shutdown old applications first.
4. It would be good to mention in the Compatibility section which version of broker and admin client we need to be able to use this new feature. Also what's the expected behavior when the broker is not supporting the new API.
5. What additional feedback for users using the new flag? Are we going to include a list of successfully deleted members, and some failed members?
6. We could separate the proposed change and public API section. In the proposed change section, we could have a mention of which API we are going to use to get the members of the stream application.
And some minor style advices:
1. Remove the link on `member.id` inside Motivation section;
2. Use a code block for the new MemberToRemove and avoid unnecessary coloring;
3. Please pay more attention to style, for example `ability to force removing` has double spaces.
Boyang
On Thu, Feb 13, 2020 at 1:48 AM feyman2009 <fe...@aliyun.com.invalid> wrote:
Hi, all
In order to make it possible for StreamsResetter to reset stream even when there are active members, since we currently only have the ability to remove static members, so we basically need the ability to remove dynamic members, this involves some public interfaces change in org.apache.kafka.clients.admin.MemberToRemove.
KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
Any comments would be highly appreciated~
Thanks !
Re: 回复：[Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by John Roesler <vv...@apache.org>.
Hey Feyman,
Thanks for starting the vote. While reviewing the discussion I saw
one thing that should be in the KIP:
> If it is used upon the older clusters like 2.3, UnsupportedVersionException will be thrown.
I'll cast my vote now.
Thanks,
-John
On Wed, Feb 26, 2020, at 19:40, feyman2009 wrote:
> Hi, Sophie
> Thanks a lot!
> I have initiated a vote
>
> Thanks!
> Feyman
>
>
> ------------------------------------------------------------------
> 发件人：Sophie Blee-Goldman <so...@confluent.io>
> 发送时间：2020年2月27日(星期四) 08:04
> 收件人：feyman2009 <fe...@aliyun.com>
> 主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
>
> Hi guys,
>
> Just to clarify, I meant a batch API on the admin not for the
> StreamsResetter, to avoid
> extra round trips and a simpler API. But I suppose it might be useful
> to be able to
> remove individual (dynamic) members and not the whole group for other
> use cases
> that could then benefit from this as well.
>
> Anyways, I'm fine with the current plan if that makes sense to you.
> Feel free to call
> for a vote if the KIP is ready
>
> Cheers,
> Sophie
> On Mon, Feb 24, 2020 at 4:16 AM feyman2009 <fe...@aliyun.com> wrote:
>
> Hi, Boyang
> Thanks! I have updated the KIP :)
> If Sophie also thinks it's ok, I will start a vote soon.
>
> Thanks!
> Feyman
>
> ------------------------------------------------------------------
> 发件人：Boyang Chen <re...@gmail.com>
> 发送时间：2020年2月24日(星期一) 00:42
> 收件人：dev <de...@kafka.apache.org>
> 主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
>
> Hey Feyman,
>
> thanks a lot for the update, the KIP LGTM now. Will let Sophie take a look
> again, also a minor API change:
> s/setGroupInstanceId/withGroupInstanceId, and similar to setMemberId, as
> usually setters are not expected to return an actual object.
>
> Boyang
>
> On Sat, Feb 22, 2020 at 11:05 PM feyman2009 <fe...@aliyun.com> wrote:
>
> > Hi, Boyang
> > Thanks for your review, I have updated the KIP page :)
> >
> > Hi, Sophie
> > Thanks for your suggestions!
> > 1) Did you consider an API that just removes *all* remaining members
> > from a group?
> > We plan to implement the batch removal in StreamsResetter as below:
> > 1) adminClient#describeConsumerGroups to get members in each
> > group, this part needs no change.
> > 2) adminClient#removeMembersFromConsumerGroup to remove all the
> > members got from the above call (This involves API change to support the
> > dynamic member removal)
> > I think your suggestion is feasible but maybe not necessary currently
> > since it is a subset of the combination of the above two APIs. Looking at
> > the APIs in KafkaAdminClient, the adminClient.deleteXXX always takes a
> > collection as the input parameter and the caller does the "query and
> > delete" if "delete all" is needed, this leaves more burden on the caller
> > side but increases flexibility. Since the KafkaAdminClient's API is still
> > evolving, I think it would be reasonable to follow the convention and not
> > adding a "removal all members" API.
> >
> > 2) Thanks to Boyang's correction, broker version >= 2.4 is needed
> > since batch members removal is introduced since then(please check KIP-345
> > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-345%3A+Introduce+static+membership+protocol+to+reduce+consumer+rebalances> for
> > details).
> > If it is used upon the older clusters like 2.3, *UnsupportedVersionException
> > *will be thrown.
> >
> > Thanks!
> > Haoran
> >
> > ------------------------------------------------------------------
> > 发件人：Boyang Chen <re...@gmail.com>
> > 发送时间：2020年2月19日(星期三) 11:57
> > 收件人：dev <de...@kafka.apache.org>
> > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > StreamsResetter
> >
> > Also Feyman, there is one thing I forget which is that the leave group
> > change was introduced in 2.4 broker instead of 2.3. Feel free to correct it
> > on the KIP.
> >
> > On Tue, Feb 18, 2020 at 5:44 PM Sophie Blee-Goldman <so...@confluent.io>
> > wrote:
> >
> > > Hey Feyman,
> > >
> > > Thanks for the KIP! I had two high-level questions:
> > >
> >
> > > It seems like, in the specific case motivating this KIP, we would only ever
> > > want to remove *all* the members remaining in the group (and never just a
> > > single member at a time). As you mention there is already an admin API to
> >
> > > remove static members, but we'd still need something new to handle dynamic
> > > ones. Did you consider an API that just removes *all* remaining members
> > > from a group, rather than requiring the caller to determine and then
> > > specify the
> > > group.id (static) or member.id (dynamic) for each one? This way we can
> > > just
> >
> > > have a single API exposed that will handle what we need to do regardless of
> > > whether static membership is used or not.
> > >
> >
> > > My other question is, will this new option only work for clusters that are
> > > on 2.3
> > > or higher? Do you have any thoughts about whether it would be possible to
> > > implement this feature for older clusters as well, or are we dependent on
> > > changes only introduced in 2.3?
> > >
> > > If so, we should make it absolutely clear what will happen if this used
> > > with
> > > an older cluster. That is, will the reset tool exit with a clear error
> > > message right
> > > away, or will it potentially leave the app in a partially reset state?
> > >
> > > Thanks!
> > > Sophie
> > >
> > > On Tue, Feb 18, 2020 at 4:30 PM Boyang Chen <re...@gmail.com>
> > > wrote:
> > >
> >
> > > > Thanks for the update Feyman. The updates look great, except one thing I
> > > > would like to be more specific is error cases display. In the "*2)* Add
> >
> > > > cmdline option" you mention throwing exception when request failed, does
> > > > that suggest partial failure or a full failure? How do we deal with
> > > > different scenarios?
> > > >
> > > > Also some minor syntax fix:
> >
> > > > 1. it only support remove static members -> it only supports the removal
> > > of
> > > > static members
> >
> > > > 2. "new constructor is added and the old constructor will be deprecated"
> > > > you mean the `new helper` right? Should be `new helper is added`
> > > > 3. users should make sure all the stream applications should be are
> > > > shutdown
> > > >
> > > > Other than the above suggestions, I think the KIP is in pretty good
> > > shape.
> > > >
> > > > Boyang
> > > >
> > > > On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com>
> > > wrote:
> > > >
> > > > > Hi, Boyang
> > > > > You can call me Feyman :)
> > > > > Thanks for your quick reply with great advices!
> > > > > I have updated the KIP-571 , would you mind to see if it looks
> > > good ?
> > > > > Thanks !
> > > > >
> > > > > ------------------------------------------------------------------
> > > > > 发件人：Boyang Chen <re...@gmail.com>
> > > > > 发送时间：2020年2月14日(星期五) 08:35
> > > > > 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> > > > > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > > > > StreamsResetter
> > > > >
> >
> > > > > Thanks for driving this change Feyman! Hope this is a good name to call
> > > > > you :)
> > > > >
> > > > > The motivation of the KIP looks good, and I have a couple of initial
> > > > > thoughts:
> > > > > 1. I guess the reason to use setters instead of adding a new
> > > constructor
> > > > > to MemberToRemove class is because we have two String members. Could
> > > you
> >
> > > > > point that out upfront so that people are not asking why not adding new
> > > > > constructor?
> > > > > 2. KIP discussion usually focuses on the public side changes, so you
> > > > don't
> > > > > need to copy-paste the entire class. Just the new APIs you are adding
> > > > > should be suffice
> >
> > > > > 3. Add the description of new flag inside Public API change, whose name
> > > > > could be simplified as `--force` and people would just read the
> > > > > description. An edge case I could think of is that some ongoing
> > > > > applications are not closed when the reset tool applies, which causes
> > > > more
> >
> > > > > unexpected rebalances. So it's important to warn users to use the flag
> > > > > wisely and be responsible to shutdown old applications first.
> > > > > 4. It would be good to mention in the Compatibility section which
> > > version
> >
> > > > > of broker and admin client we need to be able to use this new feature.
> > > > Also
> >
> > > > > what's the expected behavior when the broker is not supporting the new
> > > > API.
> >
> > > > > 5. What additional feedback for users using the new flag? Are we going
> > > to
> > > > > include a list of successfully deleted members, and some failed
> > > members?
> >
> > > > > 6. We could separate the proposed change and public API section. In the
> > > > > proposed change section, we could have a mention of which API we are
> > > > going
> > > > > to use to get the members of the stream application.
> > > > >
> > > > > And some minor style advices:
> > > > > 1. Remove the link on `member.id` inside Motivation section;
> > > > > 2. Use a code block for the new MemberToRemove and avoid unnecessary
> > > > > coloring;
> > > > > 3. Please pay more attention to style, for example `ability to force
> > > > > removing` has double spaces.
> > > > >
> > > > > Boyang
> > > > >
> > > > >
> > > > > On Thu, Feb 13, 2020 at 1:48 AM feyman2009
> > > <feyman2009@aliyun.com.invalid
> > > > >
> > > > > wrote:
> > > > > Hi, all
> > > > > In order to make it possible for StreamsResetter to reset stream
> > > even
> >
> > > > > when there are active members, since we currently only have the ability
> > > > to
> > > > > remove static members, so we basically need the ability to remove
> > > dynamic
> > > > > members, this involves some public interfaces change in
> > > > > org.apache.kafka.clients.admin.MemberToRemove.
> > > > >
> > > > > KIP:
> > > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> > > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
> > > > >
> > > > > Any comments would be highly appreciated~
> > > > > Thanks !
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> >
> >
>
>
>
回复：[Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by feyman2009 <fe...@aliyun.com.INVALID>.
Hi, Sophie
Thanks a lot!
I have initiated a vote
Thanks!
Feyman
------------------------------------------------------------------
发件人：Sophie Blee-Goldman <so...@confluent.io>
发送时间：2020年2月27日(星期四) 08:04
收件人：feyman2009 <fe...@aliyun.com>
主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Hi guys,
Just to clarify, I meant a batch API on the admin not for the StreamsResetter, to avoid
extra round trips and a simpler API. But I suppose it might be useful to be able to
remove individual (dynamic) members and not the whole group for other use cases
that could then benefit from this as well.
Anyways, I'm fine with the current plan if that makes sense to you. Feel free to call
for a vote if the KIP is ready
Cheers,
Sophie
On Mon, Feb 24, 2020 at 4:16 AM feyman2009 <fe...@aliyun.com> wrote:
Hi, Boyang
Thanks! I have updated the KIP :)
If Sophie also thinks it's ok, I will start a vote soon.
Thanks!
Feyman
------------------------------------------------------------------
发件人：Boyang Chen <re...@gmail.com>
发送时间：2020年2月24日(星期一) 00:42
收件人：dev <de...@kafka.apache.org>
主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Hey Feyman,
thanks a lot for the update, the KIP LGTM now. Will let Sophie take a look
again, also a minor API change:
s/setGroupInstanceId/withGroupInstanceId, and similar to setMemberId, as
usually setters are not expected to return an actual object.
Boyang
On Sat, Feb 22, 2020 at 11:05 PM feyman2009 <fe...@aliyun.com> wrote:
> Hi, Boyang
> Thanks for your review, I have updated the KIP page :)
>
> Hi, Sophie
> Thanks for your suggestions!
> 1) Did you consider an API that just removes *all* remaining members
> from a group?
> We plan to implement the batch removal in StreamsResetter as below:
> 1) adminClient#describeConsumerGroups to get members in each
> group, this part needs no change.
> 2) adminClient#removeMembersFromConsumerGroup to remove all the
> members got from the above call (This involves API change to support the
> dynamic member removal)
> I think your suggestion is feasible but maybe not necessary currently
> since it is a subset of the combination of the above two APIs. Looking at
> the APIs in KafkaAdminClient, the adminClient.deleteXXX always takes a
> collection as the input parameter and the caller does the "query and
> delete" if "delete all" is needed, this leaves more burden on the caller
> side but increases flexibility. Since the KafkaAdminClient's API is still
> evolving, I think it would be reasonable to follow the convention and not
> adding a "removal all members" API.
>
> 2) Thanks to Boyang's correction, broker version >= 2.4 is needed
> since batch members removal is introduced since then(please check KIP-345
> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-345%3A+Introduce+static+membership+protocol+to+reduce+consumer+rebalances> for
> details).
> If it is used upon the older clusters like 2.3, *UnsupportedVersionException
> *will be thrown.
>
> Thanks!
> Haoran
>
> ------------------------------------------------------------------
> 发件人：Boyang Chen <re...@gmail.com>
> 发送时间：2020年2月19日(星期三) 11:57
> 收件人：dev <de...@kafka.apache.org>
> 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> StreamsResetter
>
> Also Feyman, there is one thing I forget which is that the leave group
> change was introduced in 2.4 broker instead of 2.3. Feel free to correct it
> on the KIP.
>
> On Tue, Feb 18, 2020 at 5:44 PM Sophie Blee-Goldman <so...@confluent.io>
> wrote:
>
> > Hey Feyman,
> >
> > Thanks for the KIP! I had two high-level questions:
> >
>
> > It seems like, in the specific case motivating this KIP, we would only ever
> > want to remove *all* the members remaining in the group (and never just a
> > single member at a time). As you mention there is already an admin API to
>
> > remove static members, but we'd still need something new to handle dynamic
> > ones. Did you consider an API that just removes *all* remaining members
> > from a group, rather than requiring the caller to determine and then
> > specify the
> > group.id (static) or member.id (dynamic) for each one? This way we can
> > just
>
> > have a single API exposed that will handle what we need to do regardless of
> > whether static membership is used or not.
> >
>
> > My other question is, will this new option only work for clusters that are
> > on 2.3
> > or higher? Do you have any thoughts about whether it would be possible to
> > implement this feature for older clusters as well, or are we dependent on
> > changes only introduced in 2.3?
> >
> > If so, we should make it absolutely clear what will happen if this used
> > with
> > an older cluster. That is, will the reset tool exit with a clear error
> > message right
> > away, or will it potentially leave the app in a partially reset state?
> >
> > Thanks!
> > Sophie
> >
> > On Tue, Feb 18, 2020 at 4:30 PM Boyang Chen <re...@gmail.com>
> > wrote:
> >
>
> > > Thanks for the update Feyman. The updates look great, except one thing I
> > > would like to be more specific is error cases display. In the "*2)* Add
>
> > > cmdline option" you mention throwing exception when request failed, does
> > > that suggest partial failure or a full failure? How do we deal with
> > > different scenarios?
> > >
> > > Also some minor syntax fix:
>
> > > 1. it only support remove static members -> it only supports the removal
> > of
> > > static members
>
> > > 2. "new constructor is added and the old constructor will be deprecated"
> > > you mean the `new helper` right? Should be `new helper is added`
> > > 3. users should make sure all the stream applications should be are
> > > shutdown
> > >
> > > Other than the above suggestions, I think the KIP is in pretty good
> > shape.
> > >
> > > Boyang
> > >
> > > On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com>
> > wrote:
> > >
> > > > Hi, Boyang
> > > > You can call me Feyman :)
> > > > Thanks for your quick reply with great advices!
> > > > I have updated the KIP-571 , would you mind to see if it looks
> > good ?
> > > > Thanks !
> > > >
> > > > ------------------------------------------------------------------
> > > > 发件人：Boyang Chen <re...@gmail.com>
> > > > 发送时间：2020年2月14日(星期五) 08:35
> > > > 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> > > > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > > > StreamsResetter
> > > >
>
> > > > Thanks for driving this change Feyman! Hope this is a good name to call
> > > > you :)
> > > >
> > > > The motivation of the KIP looks good, and I have a couple of initial
> > > > thoughts:
> > > > 1. I guess the reason to use setters instead of adding a new
> > constructor
> > > > to MemberToRemove class is because we have two String members. Could
> > you
>
> > > > point that out upfront so that people are not asking why not adding new
> > > > constructor?
> > > > 2. KIP discussion usually focuses on the public side changes, so you
> > > don't
> > > > need to copy-paste the entire class. Just the new APIs you are adding
> > > > should be suffice
>
> > > > 3. Add the description of new flag inside Public API change, whose name
> > > > could be simplified as `--force` and people would just read the
> > > > description. An edge case I could think of is that some ongoing
> > > > applications are not closed when the reset tool applies, which causes
> > > more
>
> > > > unexpected rebalances. So it's important to warn users to use the flag
> > > > wisely and be responsible to shutdown old applications first.
> > > > 4. It would be good to mention in the Compatibility section which
> > version
>
> > > > of broker and admin client we need to be able to use this new feature.
> > > Also
>
> > > > what's the expected behavior when the broker is not supporting the new
> > > API.
>
> > > > 5. What additional feedback for users using the new flag? Are we going
> > to
> > > > include a list of successfully deleted members, and some failed
> > members?
>
> > > > 6. We could separate the proposed change and public API section. In the
> > > > proposed change section, we could have a mention of which API we are
> > > going
> > > > to use to get the members of the stream application.
> > > >
> > > > And some minor style advices:
> > > > 1. Remove the link on `member.id` inside Motivation section;
> > > > 2. Use a code block for the new MemberToRemove and avoid unnecessary
> > > > coloring;
> > > > 3. Please pay more attention to style, for example `ability to force
> > > > removing` has double spaces.
> > > >
> > > > Boyang
> > > >
> > > >
> > > > On Thu, Feb 13, 2020 at 1:48 AM feyman2009
> > <feyman2009@aliyun.com.invalid
> > > >
> > > > wrote:
> > > > Hi, all
> > > > In order to make it possible for StreamsResetter to reset stream
> > even
>
> > > > when there are active members, since we currently only have the ability
> > > to
> > > > remove static members, so we basically need the ability to remove
> > dynamic
> > > > members, this involves some public interfaces change in
> > > > org.apache.kafka.clients.admin.MemberToRemove.
> > > >
> > > > KIP:
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
> > > >
> > > > Any comments would be highly appreciated~
> > > > Thanks !
> > > >
> > > >
> > > >
> > >
> >
>
>
>
回复：[Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by feyman2009 <fe...@aliyun.com.INVALID>.
Hi, Boyang
Thanks! I have updated the KIP :)
If Sophie also thinks it's ok, I will start a vote soon.
Thanks!
Feyman
------------------------------------------------------------------
发件人：Boyang Chen <re...@gmail.com>
发送时间：2020年2月24日(星期一) 00:42
收件人：dev <de...@kafka.apache.org>
主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Hey Feyman,
thanks a lot for the update, the KIP LGTM now. Will let Sophie take a look
again, also a minor API change:
s/setGroupInstanceId/withGroupInstanceId, and similar to setMemberId, as
usually setters are not expected to return an actual object.
Boyang
On Sat, Feb 22, 2020 at 11:05 PM feyman2009 <fe...@aliyun.com> wrote:
> Hi, Boyang
> Thanks for your review, I have updated the KIP page :)
>
> Hi, Sophie
> Thanks for your suggestions!
> 1) Did you consider an API that just removes *all* remaining members
> from a group?
> We plan to implement the batch removal in StreamsResetter as below:
> 1) adminClient#describeConsumerGroups to get members in each
> group, this part needs no change.
> 2) adminClient#removeMembersFromConsumerGroup to remove all the
> members got from the above call (This involves API change to support the
> dynamic member removal)
> I think your suggestion is feasible but maybe not necessary currently
> since it is a subset of the combination of the above two APIs. Looking at
> the APIs in KafkaAdminClient, the adminClient.deleteXXX always takes a
> collection as the input parameter and the caller does the "query and
> delete" if "delete all" is needed, this leaves more burden on the caller
> side but increases flexibility. Since the KafkaAdminClient's API is still
> evolving, I think it would be reasonable to follow the convention and not
> adding a "removal all members" API.
>
> 2) Thanks to Boyang's correction, broker version >= 2.4 is needed
> since batch members removal is introduced since then(please check KIP-345
> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-345%3A+Introduce+static+membership+protocol+to+reduce+consumer+rebalances> for
> details).
> If it is used upon the older clusters like 2.3, *UnsupportedVersionException
> *will be thrown.
>
> Thanks!
> Haoran
>
> ------------------------------------------------------------------
> 发件人：Boyang Chen <re...@gmail.com>
> 发送时间：2020年2月19日(星期三) 11:57
> 收件人：dev <de...@kafka.apache.org>
> 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> StreamsResetter
>
> Also Feyman, there is one thing I forget which is that the leave group
> change was introduced in 2.4 broker instead of 2.3. Feel free to correct it
> on the KIP.
>
> On Tue, Feb 18, 2020 at 5:44 PM Sophie Blee-Goldman <so...@confluent.io>
> wrote:
>
> > Hey Feyman,
> >
> > Thanks for the KIP! I had two high-level questions:
> >
>
> > It seems like, in the specific case motivating this KIP, we would only ever
> > want to remove *all* the members remaining in the group (and never just a
> > single member at a time). As you mention there is already an admin API to
>
> > remove static members, but we'd still need something new to handle dynamic
> > ones. Did you consider an API that just removes *all* remaining members
> > from a group, rather than requiring the caller to determine and then
> > specify the
> > group.id (static) or member.id (dynamic) for each one? This way we can
> > just
>
> > have a single API exposed that will handle what we need to do regardless of
> > whether static membership is used or not.
> >
>
> > My other question is, will this new option only work for clusters that are
> > on 2.3
> > or higher? Do you have any thoughts about whether it would be possible to
> > implement this feature for older clusters as well, or are we dependent on
> > changes only introduced in 2.3?
> >
> > If so, we should make it absolutely clear what will happen if this used
> > with
> > an older cluster. That is, will the reset tool exit with a clear error
> > message right
> > away, or will it potentially leave the app in a partially reset state?
> >
> > Thanks!
> > Sophie
> >
> > On Tue, Feb 18, 2020 at 4:30 PM Boyang Chen <re...@gmail.com>
> > wrote:
> >
>
> > > Thanks for the update Feyman. The updates look great, except one thing I
> > > would like to be more specific is error cases display. In the "*2)* Add
>
> > > cmdline option" you mention throwing exception when request failed, does
> > > that suggest partial failure or a full failure? How do we deal with
> > > different scenarios?
> > >
> > > Also some minor syntax fix:
>
> > > 1. it only support remove static members -> it only supports the removal
> > of
> > > static members
>
> > > 2. "new constructor is added and the old constructor will be deprecated"
> > > you mean the `new helper` right? Should be `new helper is added`
> > > 3. users should make sure all the stream applications should be are
> > > shutdown
> > >
> > > Other than the above suggestions, I think the KIP is in pretty good
> > shape.
> > >
> > > Boyang
> > >
> > > On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com>
> > wrote:
> > >
> > > > Hi, Boyang
> > > > You can call me Feyman :)
> > > > Thanks for your quick reply with great advices!
> > > > I have updated the KIP-571 , would you mind to see if it looks
> > good ?
> > > > Thanks !
> > > >
> > > > ------------------------------------------------------------------
> > > > 发件人：Boyang Chen <re...@gmail.com>
> > > > 发送时间：2020年2月14日(星期五) 08:35
> > > > 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> > > > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > > > StreamsResetter
> > > >
>
> > > > Thanks for driving this change Feyman! Hope this is a good name to call
> > > > you :)
> > > >
> > > > The motivation of the KIP looks good, and I have a couple of initial
> > > > thoughts:
> > > > 1. I guess the reason to use setters instead of adding a new
> > constructor
> > > > to MemberToRemove class is because we have two String members. Could
> > you
>
> > > > point that out upfront so that people are not asking why not adding new
> > > > constructor?
> > > > 2. KIP discussion usually focuses on the public side changes, so you
> > > don't
> > > > need to copy-paste the entire class. Just the new APIs you are adding
> > > > should be suffice
>
> > > > 3. Add the description of new flag inside Public API change, whose name
> > > > could be simplified as `--force` and people would just read the
> > > > description. An edge case I could think of is that some ongoing
> > > > applications are not closed when the reset tool applies, which causes
> > > more
>
> > > > unexpected rebalances. So it's important to warn users to use the flag
> > > > wisely and be responsible to shutdown old applications first.
> > > > 4. It would be good to mention in the Compatibility section which
> > version
>
> > > > of broker and admin client we need to be able to use this new feature.
> > > Also
>
> > > > what's the expected behavior when the broker is not supporting the new
> > > API.
>
> > > > 5. What additional feedback for users using the new flag? Are we going
> > to
> > > > include a list of successfully deleted members, and some failed
> > members?
>
> > > > 6. We could separate the proposed change and public API section. In the
> > > > proposed change section, we could have a mention of which API we are
> > > going
> > > > to use to get the members of the stream application.
> > > >
> > > > And some minor style advices:
> > > > 1. Remove the link on `member.id` inside Motivation section;
> > > > 2. Use a code block for the new MemberToRemove and avoid unnecessary
> > > > coloring;
> > > > 3. Please pay more attention to style, for example `ability to force
> > > > removing` has double spaces.
> > > >
> > > > Boyang
> > > >
> > > >
> > > > On Thu, Feb 13, 2020 at 1:48 AM feyman2009
> > <feyman2009@aliyun.com.invalid
> > > >
> > > > wrote:
> > > > Hi, all
> > > > In order to make it possible for StreamsResetter to reset stream
> > even
>
> > > > when there are active members, since we currently only have the ability
> > > to
> > > > remove static members, so we basically need the ability to remove
> > dynamic
> > > > members, this involves some public interfaces change in
> > > > org.apache.kafka.clients.admin.MemberToRemove.
> > > >
> > > > KIP:
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
> > > >
> > > > Any comments would be highly appreciated~
> > > > Thanks !
> > > >
> > > >
> > > >
> > >
> >
>
>
>
Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by Boyang Chen <re...@gmail.com>.
Hey Feyman,
thanks a lot for the update, the KIP LGTM now. Will let Sophie take a look
again, also a minor API change:
s/setGroupInstanceId/withGroupInstanceId, and similar to setMemberId, as
usually setters are not expected to return an actual object.
Boyang
On Sat, Feb 22, 2020 at 11:05 PM feyman2009 <fe...@aliyun.com> wrote:
> Hi, Boyang
> Thanks for your review, I have updated the KIP page :)
>
> Hi, Sophie
> Thanks for your suggestions!
> 1) Did you consider an API that just removes *all* remaining members
> from a group?
> We plan to implement the batch removal in StreamsResetter as below:
> 1) adminClient#describeConsumerGroups to get members in each
> group, this part needs no change.
> 2) adminClient#removeMembersFromConsumerGroup to remove all the
> members got from the above call (This involves API change to support the
> dynamic member removal)
> I think your suggestion is feasible but maybe not necessary currently
> since it is a subset of the combination of the above two APIs. Looking at
> the APIs in KafkaAdminClient, the adminClient.deleteXXX always takes a
> collection as the input parameter and the caller does the "query and
> delete" if "delete all" is needed, this leaves more burden on the caller
> side but increases flexibility. Since the KafkaAdminClient's API is still
> evolving, I think it would be reasonable to follow the convention and not
> adding a "removal all members" API.
>
> 2) Thanks to Boyang's correction, broker version >= 2.4 is needed
> since batch members removal is introduced since then(please check KIP-345
> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-345%3A+Introduce+static+membership+protocol+to+reduce+consumer+rebalances> for
> details).
> If it is used upon the older clusters like 2.3, *UnsupportedVersionException
> *will be thrown.
>
> Thanks!
> Haoran
>
> ------------------------------------------------------------------
> 发件人：Boyang Chen <re...@gmail.com>
> 发送时间：2020年2月19日(星期三) 11:57
> 收件人：dev <de...@kafka.apache.org>
> 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> StreamsResetter
>
> Also Feyman, there is one thing I forget which is that the leave group
> change was introduced in 2.4 broker instead of 2.3. Feel free to correct it
> on the KIP.
>
> On Tue, Feb 18, 2020 at 5:44 PM Sophie Blee-Goldman <so...@confluent.io>
> wrote:
>
> > Hey Feyman,
> >
> > Thanks for the KIP! I had two high-level questions:
> >
>
> > It seems like, in the specific case motivating this KIP, we would only ever
> > want to remove *all* the members remaining in the group (and never just a
> > single member at a time). As you mention there is already an admin API to
>
> > remove static members, but we'd still need something new to handle dynamic
> > ones. Did you consider an API that just removes *all* remaining members
> > from a group, rather than requiring the caller to determine and then
> > specify the
> > group.id (static) or member.id (dynamic) for each one? This way we can
> > just
>
> > have a single API exposed that will handle what we need to do regardless of
> > whether static membership is used or not.
> >
>
> > My other question is, will this new option only work for clusters that are
> > on 2.3
> > or higher? Do you have any thoughts about whether it would be possible to
> > implement this feature for older clusters as well, or are we dependent on
> > changes only introduced in 2.3?
> >
> > If so, we should make it absolutely clear what will happen if this used
> > with
> > an older cluster. That is, will the reset tool exit with a clear error
> > message right
> > away, or will it potentially leave the app in a partially reset state?
> >
> > Thanks!
> > Sophie
> >
> > On Tue, Feb 18, 2020 at 4:30 PM Boyang Chen <re...@gmail.com>
> > wrote:
> >
>
> > > Thanks for the update Feyman. The updates look great, except one thing I
> > > would like to be more specific is error cases display. In the "*2)* Add
>
> > > cmdline option" you mention throwing exception when request failed, does
> > > that suggest partial failure or a full failure? How do we deal with
> > > different scenarios?
> > >
> > > Also some minor syntax fix:
>
> > > 1. it only support remove static members -> it only supports the removal
> > of
> > > static members
>
> > > 2. "new constructor is added and the old constructor will be deprecated"
> > > you mean the `new helper` right? Should be `new helper is added`
> > > 3. users should make sure all the stream applications should be are
> > > shutdown
> > >
> > > Other than the above suggestions, I think the KIP is in pretty good
> > shape.
> > >
> > > Boyang
> > >
> > > On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com>
> > wrote:
> > >
> > > > Hi, Boyang
> > > > You can call me Feyman :)
> > > > Thanks for your quick reply with great advices!
> > > > I have updated the KIP-571 , would you mind to see if it looks
> > good ?
> > > > Thanks !
> > > >
> > > > ------------------------------------------------------------------
> > > > 发件人：Boyang Chen <re...@gmail.com>
> > > > 发送时间：2020年2月14日(星期五) 08:35
> > > > 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> > > > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > > > StreamsResetter
> > > >
>
> > > > Thanks for driving this change Feyman! Hope this is a good name to call
> > > > you :)
> > > >
> > > > The motivation of the KIP looks good, and I have a couple of initial
> > > > thoughts:
> > > > 1. I guess the reason to use setters instead of adding a new
> > constructor
> > > > to MemberToRemove class is because we have two String members. Could
> > you
>
> > > > point that out upfront so that people are not asking why not adding new
> > > > constructor?
> > > > 2. KIP discussion usually focuses on the public side changes, so you
> > > don't
> > > > need to copy-paste the entire class. Just the new APIs you are adding
> > > > should be suffice
>
> > > > 3. Add the description of new flag inside Public API change, whose name
> > > > could be simplified as `--force` and people would just read the
> > > > description. An edge case I could think of is that some ongoing
> > > > applications are not closed when the reset tool applies, which causes
> > > more
>
> > > > unexpected rebalances. So it's important to warn users to use the flag
> > > > wisely and be responsible to shutdown old applications first.
> > > > 4. It would be good to mention in the Compatibility section which
> > version
>
> > > > of broker and admin client we need to be able to use this new feature.
> > > Also
>
> > > > what's the expected behavior when the broker is not supporting the new
> > > API.
>
> > > > 5. What additional feedback for users using the new flag? Are we going
> > to
> > > > include a list of successfully deleted members, and some failed
> > members?
>
> > > > 6. We could separate the proposed change and public API section. In the
> > > > proposed change section, we could have a mention of which API we are
> > > going
> > > > to use to get the members of the stream application.
> > > >
> > > > And some minor style advices:
> > > > 1. Remove the link on `member.id` inside Motivation section;
> > > > 2. Use a code block for the new MemberToRemove and avoid unnecessary
> > > > coloring;
> > > > 3. Please pay more attention to style, for example `ability to force
> > > > removing` has double spaces.
> > > >
> > > > Boyang
> > > >
> > > >
> > > > On Thu, Feb 13, 2020 at 1:48 AM feyman2009
> > <feyman2009@aliyun.com.invalid
> > > >
> > > > wrote:
> > > > Hi, all
> > > > In order to make it possible for StreamsResetter to reset stream
> > even
>
> > > > when there are active members, since we currently only have the ability
> > > to
> > > > remove static members, so we basically need the ability to remove
> > dynamic
> > > > members, this involves some public interfaces change in
> > > > org.apache.kafka.clients.admin.MemberToRemove.
> > > >
> > > > KIP:
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
> > > >
> > > > Any comments would be highly appreciated~
> > > > Thanks !
> > > >
> > > >
> > > >
> > >
> >
>
>
>
回复：[Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by feyman2009 <fe...@aliyun.com.INVALID>.
Hi, Boyang
Thanks for your review, I have updated the KIP page :)
Hi, Sophie
Thanks for your suggestions!
1) Did you consider an API that just removes *all* remaining members from a group?
We plan to implement the batch removal in StreamsResetter as below:
1) adminClient#describeConsumerGroups to get members in each group, this part needs no change.
2) adminClient#removeMembersFromConsumerGroup to remove all the members got from the above call (This involves API change to support the dynamic member removal)
I think your suggestion is feasible but maybe not necessary currently since it is a subset of the combination of the above two APIs. Looking at the APIs in KafkaAdminClient, the adminClient.deleteXXX always takes a collection as the input parameter and the caller does the "query and delete" if "delete all" is needed, this leaves more burden on the caller side but increases flexibility. Since the KafkaAdminClient's API is still evolving, I think it would be reasonable to follow the convention and not adding a "removal all members" API.
2) Thanks to Boyang's correction, broker version >= 2.4 is needed since batch members removal is introduced since then(please check KIP-345 for details).
If it is used upon the older clusters like 2.3, UnsupportedVersionException will be thrown.
Thanks!
Haoran
------------------------------------------------------------------
发件人：Boyang Chen <re...@gmail.com>
发送时间：2020年2月19日(星期三) 11:57
收件人：dev <de...@kafka.apache.org>
主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Also Feyman, there is one thing I forget which is that the leave group
change was introduced in 2.4 broker instead of 2.3. Feel free to correct it
on the KIP.
On Tue, Feb 18, 2020 at 5:44 PM Sophie Blee-Goldman <so...@confluent.io>
wrote:
> Hey Feyman,
>
> Thanks for the KIP! I had two high-level questions:
>
> It seems like, in the specific case motivating this KIP, we would only ever
> want to remove *all* the members remaining in the group (and never just a
> single member at a time). As you mention there is already an admin API to
> remove static members, but we'd still need something new to handle dynamic
> ones. Did you consider an API that just removes *all* remaining members
> from a group, rather than requiring the caller to determine and then
> specify the
> group.id (static) or member.id (dynamic) for each one? This way we can
> just
> have a single API exposed that will handle what we need to do regardless of
> whether static membership is used or not.
>
> My other question is, will this new option only work for clusters that are
> on 2.3
> or higher? Do you have any thoughts about whether it would be possible to
> implement this feature for older clusters as well, or are we dependent on
> changes only introduced in 2.3?
>
> If so, we should make it absolutely clear what will happen if this used
> with
> an older cluster. That is, will the reset tool exit with a clear error
> message right
> away, or will it potentially leave the app in a partially reset state?
>
> Thanks!
> Sophie
>
> On Tue, Feb 18, 2020 at 4:30 PM Boyang Chen <re...@gmail.com>
> wrote:
>
> > Thanks for the update Feyman. The updates look great, except one thing I
> > would like to be more specific is error cases display. In the "*2)* Add
> > cmdline option" you mention throwing exception when request failed, does
> > that suggest partial failure or a full failure? How do we deal with
> > different scenarios?
> >
> > Also some minor syntax fix:
> > 1. it only support remove static members -> it only supports the removal
> of
> > static members
> > 2. "new constructor is added and the old constructor will be deprecated"
> > you mean the `new helper` right? Should be `new helper is added`
> > 3. users should make sure all the stream applications should be are
> > shutdown
> >
> > Other than the above suggestions, I think the KIP is in pretty good
> shape.
> >
> > Boyang
> >
> > On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com>
> wrote:
> >
> > > Hi, Boyang
> > > You can call me Feyman :)
> > > Thanks for your quick reply with great advices!
> > > I have updated the KIP-571 , would you mind to see if it looks
> good ?
> > > Thanks !
> > >
> > > ------------------------------------------------------------------
> > > 发件人：Boyang Chen <re...@gmail.com>
> > > 发送时间：2020年2月14日(星期五) 08:35
> > > 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> > > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > > StreamsResetter
> > >
> > > Thanks for driving this change Feyman! Hope this is a good name to call
> > > you :)
> > >
> > > The motivation of the KIP looks good, and I have a couple of initial
> > > thoughts:
> > > 1. I guess the reason to use setters instead of adding a new
> constructor
> > > to MemberToRemove class is because we have two String members. Could
> you
> > > point that out upfront so that people are not asking why not adding new
> > > constructor?
> > > 2. KIP discussion usually focuses on the public side changes, so you
> > don't
> > > need to copy-paste the entire class. Just the new APIs you are adding
> > > should be suffice
> > > 3. Add the description of new flag inside Public API change, whose name
> > > could be simplified as `--force` and people would just read the
> > > description. An edge case I could think of is that some ongoing
> > > applications are not closed when the reset tool applies, which causes
> > more
> > > unexpected rebalances. So it's important to warn users to use the flag
> > > wisely and be responsible to shutdown old applications first.
> > > 4. It would be good to mention in the Compatibility section which
> version
> > > of broker and admin client we need to be able to use this new feature.
> > Also
> > > what's the expected behavior when the broker is not supporting the new
> > API.
> > > 5. What additional feedback for users using the new flag? Are we going
> to
> > > include a list of successfully deleted members, and some failed
> members?
> > > 6. We could separate the proposed change and public API section. In the
> > > proposed change section, we could have a mention of which API we are
> > going
> > > to use to get the members of the stream application.
> > >
> > > And some minor style advices:
> > > 1. Remove the link on `member.id` inside Motivation section;
> > > 2. Use a code block for the new MemberToRemove and avoid unnecessary
> > > coloring;
> > > 3. Please pay more attention to style, for example `ability to force
> > > removing` has double spaces.
> > >
> > > Boyang
> > >
> > >
> > > On Thu, Feb 13, 2020 at 1:48 AM feyman2009
> <feyman2009@aliyun.com.invalid
> > >
> > > wrote:
> > > Hi, all
> > > In order to make it possible for StreamsResetter to reset stream
> even
> > > when there are active members, since we currently only have the ability
> > to
> > > remove static members, so we basically need the ability to remove
> dynamic
> > > members, this involves some public interfaces change in
> > > org.apache.kafka.clients.admin.MemberToRemove.
> > >
> > > KIP:
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> > > JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
> > >
> > > Any comments would be highly appreciated~
> > > Thanks !
> > >
> > >
> > >
> >
>
Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by Boyang Chen <re...@gmail.com>.
Also Feyman, there is one thing I forget which is that the leave group
change was introduced in 2.4 broker instead of 2.3. Feel free to correct it
on the KIP.
On Tue, Feb 18, 2020 at 5:44 PM Sophie Blee-Goldman <so...@confluent.io>
wrote:
> Hey Feyman,
>
> Thanks for the KIP! I had two high-level questions:
>
> It seems like, in the specific case motivating this KIP, we would only ever
> want to remove *all* the members remaining in the group (and never just a
> single member at a time). As you mention there is already an admin API to
> remove static members, but we'd still need something new to handle dynamic
> ones. Did you consider an API that just removes *all* remaining members
> from a group, rather than requiring the caller to determine and then
> specify the
> group.id (static) or member.id (dynamic) for each one? This way we can
> just
> have a single API exposed that will handle what we need to do regardless of
> whether static membership is used or not.
>
> My other question is, will this new option only work for clusters that are
> on 2.3
> or higher? Do you have any thoughts about whether it would be possible to
> implement this feature for older clusters as well, or are we dependent on
> changes only introduced in 2.3?
>
> If so, we should make it absolutely clear what will happen if this used
> with
> an older cluster. That is, will the reset tool exit with a clear error
> message right
> away, or will it potentially leave the app in a partially reset state?
>
> Thanks!
> Sophie
>
> On Tue, Feb 18, 2020 at 4:30 PM Boyang Chen <re...@gmail.com>
> wrote:
>
> > Thanks for the update Feyman. The updates look great, except one thing I
> > would like to be more specific is error cases display. In the "*2)* Add
> > cmdline option" you mention throwing exception when request failed, does
> > that suggest partial failure or a full failure? How do we deal with
> > different scenarios?
> >
> > Also some minor syntax fix:
> > 1. it only support remove static members -> it only supports the removal
> of
> > static members
> > 2. "new constructor is added and the old constructor will be deprecated"
> > you mean the `new helper` right? Should be `new helper is added`
> > 3. users should make sure all the stream applications should be are
> > shutdown
> >
> > Other than the above suggestions, I think the KIP is in pretty good
> shape.
> >
> > Boyang
> >
> > On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com>
> wrote:
> >
> > > Hi, Boyang
> > > You can call me Feyman :)
> > > Thanks for your quick reply with great advices!
> > > I have updated the KIP-571 , would you mind to see if it looks
> good ?
> > > Thanks !
> > >
> > > ------------------------------------------------------------------
> > > 发件人：Boyang Chen <re...@gmail.com>
> > > 发送时间：2020年2月14日(星期五) 08:35
> > > 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> > > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > > StreamsResetter
> > >
> > > Thanks for driving this change Feyman! Hope this is a good name to call
> > > you :)
> > >
> > > The motivation of the KIP looks good, and I have a couple of initial
> > > thoughts:
> > > 1. I guess the reason to use setters instead of adding a new
> constructor
> > > to MemberToRemove class is because we have two String members. Could
> you
> > > point that out upfront so that people are not asking why not adding new
> > > constructor?
> > > 2. KIP discussion usually focuses on the public side changes, so you
> > don't
> > > need to copy-paste the entire class. Just the new APIs you are adding
> > > should be suffice
> > > 3. Add the description of new flag inside Public API change, whose name
> > > could be simplified as `--force` and people would just read the
> > > description. An edge case I could think of is that some ongoing
> > > applications are not closed when the reset tool applies, which causes
> > more
> > > unexpected rebalances. So it's important to warn users to use the flag
> > > wisely and be responsible to shutdown old applications first.
> > > 4. It would be good to mention in the Compatibility section which
> version
> > > of broker and admin client we need to be able to use this new feature.
> > Also
> > > what's the expected behavior when the broker is not supporting the new
> > API.
> > > 5. What additional feedback for users using the new flag? Are we going
> to
> > > include a list of successfully deleted members, and some failed
> members?
> > > 6. We could separate the proposed change and public API section. In the
> > > proposed change section, we could have a mention of which API we are
> > going
> > > to use to get the members of the stream application.
> > >
> > > And some minor style advices:
> > > 1. Remove the link on `member.id` inside Motivation section;
> > > 2. Use a code block for the new MemberToRemove and avoid unnecessary
> > > coloring;
> > > 3. Please pay more attention to style, for example `ability to force
> > > removing` has double spaces.
> > >
> > > Boyang
> > >
> > >
> > > On Thu, Feb 13, 2020 at 1:48 AM feyman2009
> <feyman2009@aliyun.com.invalid
> > >
> > > wrote:
> > > Hi, all
> > > In order to make it possible for StreamsResetter to reset stream
> even
> > > when there are active members, since we currently only have the ability
> > to
> > > remove static members, so we basically need the ability to remove
> dynamic
> > > members, this involves some public interfaces change in
> > > org.apache.kafka.clients.admin.MemberToRemove.
> > >
> > > KIP:
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> > > JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
> > >
> > > Any comments would be highly appreciated~
> > > Thanks !
> > >
> > >
> > >
> >
>
Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by Sophie Blee-Goldman <so...@confluent.io>.
Hey Feyman,
Thanks for the KIP! I had two high-level questions:
It seems like, in the specific case motivating this KIP, we would only ever
want to remove *all* the members remaining in the group (and never just a
single member at a time). As you mention there is already an admin API to
remove static members, but we'd still need something new to handle dynamic
ones. Did you consider an API that just removes *all* remaining members
from a group, rather than requiring the caller to determine and then
specify the
group.id (static) or member.id (dynamic) for each one? This way we can just
have a single API exposed that will handle what we need to do regardless of
whether static membership is used or not.
My other question is, will this new option only work for clusters that are
on 2.3
or higher? Do you have any thoughts about whether it would be possible to
implement this feature for older clusters as well, or are we dependent on
changes only introduced in 2.3?
If so, we should make it absolutely clear what will happen if this used with
an older cluster. That is, will the reset tool exit with a clear error
message right
away, or will it potentially leave the app in a partially reset state?
Thanks!
Sophie
On Tue, Feb 18, 2020 at 4:30 PM Boyang Chen <re...@gmail.com>
wrote:
> Thanks for the update Feyman. The updates look great, except one thing I
> would like to be more specific is error cases display. In the "*2)* Add
> cmdline option" you mention throwing exception when request failed, does
> that suggest partial failure or a full failure? How do we deal with
> different scenarios?
>
> Also some minor syntax fix:
> 1. it only support remove static members -> it only supports the removal of
> static members
> 2. "new constructor is added and the old constructor will be deprecated"
> you mean the `new helper` right? Should be `new helper is added`
> 3. users should make sure all the stream applications should be are
> shutdown
>
> Other than the above suggestions, I think the KIP is in pretty good shape.
>
> Boyang
>
> On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com> wrote:
>
> > Hi, Boyang
> > You can call me Feyman :)
> > Thanks for your quick reply with great advices!
> > I have updated the KIP-571 , would you mind to see if it looks good ?
> > Thanks !
> >
> > ------------------------------------------------------------------
> > 发件人：Boyang Chen <re...@gmail.com>
> > 发送时间：2020年2月14日(星期五) 08:35
> > 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> > 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> > StreamsResetter
> >
> > Thanks for driving this change Feyman! Hope this is a good name to call
> > you :)
> >
> > The motivation of the KIP looks good, and I have a couple of initial
> > thoughts:
> > 1. I guess the reason to use setters instead of adding a new constructor
> > to MemberToRemove class is because we have two String members. Could you
> > point that out upfront so that people are not asking why not adding new
> > constructor?
> > 2. KIP discussion usually focuses on the public side changes, so you
> don't
> > need to copy-paste the entire class. Just the new APIs you are adding
> > should be suffice
> > 3. Add the description of new flag inside Public API change, whose name
> > could be simplified as `--force` and people would just read the
> > description. An edge case I could think of is that some ongoing
> > applications are not closed when the reset tool applies, which causes
> more
> > unexpected rebalances. So it's important to warn users to use the flag
> > wisely and be responsible to shutdown old applications first.
> > 4. It would be good to mention in the Compatibility section which version
> > of broker and admin client we need to be able to use this new feature.
> Also
> > what's the expected behavior when the broker is not supporting the new
> API.
> > 5. What additional feedback for users using the new flag? Are we going to
> > include a list of successfully deleted members, and some failed members?
> > 6. We could separate the proposed change and public API section. In the
> > proposed change section, we could have a mention of which API we are
> going
> > to use to get the members of the stream application.
> >
> > And some minor style advices:
> > 1. Remove the link on `member.id` inside Motivation section;
> > 2. Use a code block for the new MemberToRemove and avoid unnecessary
> > coloring;
> > 3. Please pay more attention to style, for example `ability to force
> > removing` has double spaces.
> >
> > Boyang
> >
> >
> > On Thu, Feb 13, 2020 at 1:48 AM feyman2009 <feyman2009@aliyun.com.invalid
> >
> > wrote:
> > Hi, all
> > In order to make it possible for StreamsResetter to reset stream even
> > when there are active members, since we currently only have the ability
> to
> > remove static members, so we basically need the ability to remove dynamic
> > members, this involves some public interfaces change in
> > org.apache.kafka.clients.admin.MemberToRemove.
> >
> > KIP:
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> > JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
> >
> > Any comments would be highly appreciated~
> > Thanks !
> >
> >
> >
>
Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Posted by Boyang Chen <re...@gmail.com>.
Thanks for the update Feyman. The updates look great, except one thing I
would like to be more specific is error cases display. In the "*2)* Add
cmdline option" you mention throwing exception when request failed, does
that suggest partial failure or a full failure? How do we deal with
different scenarios?
Also some minor syntax fix:
1. it only support remove static members -> it only supports the removal of
static members
2. "new constructor is added and the old constructor will be deprecated"
you mean the `new helper` right? Should be `new helper is added`
3. users should make sure all the stream applications should be are shutdown
Other than the above suggestions, I think the KIP is in pretty good shape.
Boyang
On Fri, Feb 14, 2020 at 9:29 PM feyman2009 <fe...@aliyun.com> wrote:
> Hi, Boyang
> You can call me Feyman :)
> Thanks for your quick reply with great advices!
> I have updated the KIP-571 , would you mind to see if it looks good ?
> Thanks !
>
> ------------------------------------------------------------------
> 发件人：Boyang Chen <re...@gmail.com>
> 发送时间：2020年2月14日(星期五) 08:35
> 收件人：dev <de...@kafka.apache.org>; feyman2009 <fe...@aliyun.com>
> 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> StreamsResetter
>
> Thanks for driving this change Feyman! Hope this is a good name to call
> you :)
>
> The motivation of the KIP looks good, and I have a couple of initial
> thoughts:
> 1. I guess the reason to use setters instead of adding a new constructor
> to MemberToRemove class is because we have two String members. Could you
> point that out upfront so that people are not asking why not adding new
> constructor?
> 2. KIP discussion usually focuses on the public side changes, so you don't
> need to copy-paste the entire class. Just the new APIs you are adding
> should be suffice
> 3. Add the description of new flag inside Public API change, whose name
> could be simplified as `--force` and people would just read the
> description. An edge case I could think of is that some ongoing
> applications are not closed when the reset tool applies, which causes more
> unexpected rebalances. So it's important to warn users to use the flag
> wisely and be responsible to shutdown old applications first.
> 4. It would be good to mention in the Compatibility section which version
> of broker and admin client we need to be able to use this new feature. Also
> what's the expected behavior when the broker is not supporting the new API.
> 5. What additional feedback for users using the new flag? Are we going to
> include a list of successfully deleted members, and some failed members?
> 6. We could separate the proposed change and public API section. In the
> proposed change section, we could have a mention of which API we are going
> to use to get the members of the stream application.
>
> And some minor style advices:
> 1. Remove the link on `member.id` inside Motivation section;
> 2. Use a code block for the new MemberToRemove and avoid unnecessary
> coloring;
> 3. Please pay more attention to style, for example `ability to force
> removing` has double spaces.
>
> Boyang
>
>
> On Thu, Feb 13, 2020 at 1:48 AM feyman2009 <fe...@aliyun.com.invalid>
> wrote:
> Hi, all
> In order to make it possible for StreamsResetter to reset stream even
> when there are active members, since we currently only have the ability to
> remove static members, so we basically need the ability to remove dynamic
> members, this involves some public interfaces change in
> org.apache.kafka.clients.admin.MemberToRemove.
>
> KIP:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+option+to+force+remove+members+in+StreamsResetter
> JIRA: https://issues.apache.org/jira/browse/KAFKA-9146
>
> Any comments would be highly appreciated~
> Thanks !
>
>
>