Hi, Boyang
Thanks! I have updated the KIP :)
If Sophie also thinks it's ok, I will start a vote soon.
Thanks!
Feyman
------------------------------------------------------------------
发件人：Boyang Chen <reluctanthero...@gmail.com>
发送时间：2020年2月24日(星期一) 00:42
收件人：dev <dev@kafka.apache.org>
主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Hey Feyman,
thanks a lot for the update, the KIP LGTM now. Will let Sophie take a look
again, also a minor API change:
s/setGroupInstanceId/withGroupInstanceId, and similar to setMemberId, as
usually setters are not expected to return an actual object.
Boyang
On Sat, Feb 22, 2020 at 11:05 PM feyman2009 <feyman2...@aliyun.com> wrote:
> Hi, Boyang
> Thanks for your review, I have updated the KIP page :)
>
> Hi, Sophie
> Thanks for your suggestions!
> 1) Did you consider an API that just removes *all* remaining members
> from a group?
> We plan to implement the batch removal in StreamsResetter as below:
> 1) adminClient#describeConsumerGroups to get members in each
> group, this part needs no change.
> 2) adminClient#removeMembersFromConsumerGroup to remove all the
> members got from the above call (This involves API change to support the
> dynamic member removal)
> I think your suggestion is feasible but maybe not necessary currently
> since it is a subset of the combination of the above two APIs. Looking at
> the APIs in KafkaAdminClient, the adminClient.deleteXXX always takes a
> collection as the input parameter and the caller does the "query and
> delete" if "delete all" is needed, this leaves more burden on the caller
> side but increases flexibility. Since the KafkaAdminClient's API is still
> evolving, I think it would be reasonable to follow the convention and not
> adding a "removal all members" API.
>
> 2) Thanks to Boyang's correction, broker version >= 2.4 is needed
> since batch members removal is introduced since then(please check KIP-345
> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-345%3A+Introduce+static+membership+protocol+to+reduce+consumer+rebalances>
> for
> details).
> If it is used upon the older clusters like 2.3,
> *UnsupportedVersionException
> *will be thrown.
>
> Thanks!
> Haoran
>
> ------------------------------------------------------------------
> 发件人：Boyang Chen <reluctanthero...@gmail.com>
> 发送时间：2020年2月19日(星期三) 11:57
> 收件人：dev <dev@kafka.apache.org>
> 主 题：Re: [Discuss] KIP-571: Add option to force remove members in
> StreamsResetter
>
> 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 !
> > > >
> > > >
> > > >
> > >
> >
>
>
>
