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 <sop...@confluent.io> > 发送时间：2020年2月27日(星期四) 08:04 > 收件人：feyman2009 <feyman2...@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 <feyman2...@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 <reluctanthero...@gmail.com> > 发送时间：2020年2月24日(星期一) 00:42 > 收件人：dev <dev@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 <feyman2...@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 <reluctanthero...@gmail.com> > > 发送时间：2020年2月19日(星期三) 11:57 > > 收件人：dev <dev@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 <sop...@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 <reluctanthero...@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 <feyman2...@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 <reluctanthero...@gmail.com> > > > > > 发送时间：2020年2月14日(星期五) 08:35 > > > > > 收件人：dev <dev@kafka.apache.org>; feyman2009 <feyman2...@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 > > > <feyman2...@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 ! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
