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 ! > > > > > > > > > > > > > > > > > > > >
