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