Hi, Sophie
Thanks a lot!
I have initiated a vote
Thanks!
Feyman
------------------------------------------------------------------
发件人：Sophie Blee-Goldman <sop...@confluent.io>
发送时间：2020年2月27日(星期四) 08:04
收件人：feyman2009 <feyman2...@aliyun.com>
主 题：Re: [Discuss] KIP-571: Add option to force remove members in StreamsResetter
Hi guys,
Just to clarify, I meant a batch API on the admin not for the StreamsResetter,
to avoid
extra round trips and a simpler API. But I suppose it might be useful to be
able to
remove individual (dynamic) members and not the whole group for other use cases
that could then benefit from this as well.
Anyways, I'm fine with the current plan if that makes sense to you. Feel free
to call
for a vote if the KIP is ready
Cheers,
Sophie
On Mon, Feb 24, 2020 at 4:16 AM feyman2009 <feyman2...@aliyun.com> wrote:
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 !
> > > >
> > > >
> > > >
> > >
> >
>
>
>
