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