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 <[email protected]>
发送时间：2020年2月14日(星期五) 08:35
收件人：dev <[email protected]>; feyman2009 <[email protected]>
主 题：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 <[email protected]>
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 !
