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 !
