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