Bump, can anyone kindly take a look at the updated KIP-571? Thanks!
------------------------------------------------------------------
发件人：feyman2009 <feyman2...@aliyun.com.INVALID>
发送时间：2020年3月23日(星期一) 08:51
收件人：dev <dev@kafka.apache.org>
主 题：回复：回复：回复：回复：[Vote] KIP-571: Add option to force remove members in
StreamsResetter
Hi, team
I have updated the KIP-571 according to our latest discussion results,
would you mind to take a look? Thanks!
Feyman
------------------------------------------------------------------
发件人：Boyang Chen <reluctanthero...@gmail.com>
发送时间：2020年3月19日(星期四) 13:41
收件人：dev <dev@kafka.apache.org>; feyman2009 <feyman2...@aliyun.com>
主 题：Re: 回复：回复：回复：[Vote] KIP-571: Add option to force remove members in
StreamsResetter
Thanks for the insight Feyman. I personally feel adding another admin client
command is redundant, so picking option 1). The memberInfos struct is internal
and just used for result reference purposes. I think it could still work even
we overload with `removeAll` option, if that makes sense.
Boyang
On Wed, Mar 18, 2020 at 8:51 PM feyman2009 <feyman2...@aliyun.com.invalid>
wrote:
Hi, team
Before going too far on the KIP update, I would like to hear your opinions
about how we would change the interface of AdminClient, the two alternatives I
could think of are:
1) Extend adminClient.removeMembersFromConsumerGroup to support remove all
As Guochang suggested, we could add some flag param in
RemoveMembersFromConsumerGroupOptions to indicated the "remove all" logic.
2) Add a new API like
adminClient.removeAllMembersFromConsumerGroup(groupId, options)
I think 1) will be more compact from the API perspective, but looking at
the code, I found that the if we are going to remove all, then the
RemoveMembersFromConsumerGroupResult#memberInfos/memberResult()/all() should be
changed accordingly, and they seem not that meaningful under the "remove all"
scenario.
A minor thought about the adminClient.removeMembersFromConsumerGroup API
is:
Looking at some other deleteXX APIs, like deleteTopics, deleteRecords, the
results contains only a Map<A, Future<B>>, I think it's enough to describe the
related results, is it make sense that we may remove memberInfos in
RemoveMembersFromConsumerGroupResult ? This KIP has no dependency on this if we
choose alternative 2)
Could you advise? Thanks!
Feyman
送时间：2020年3月15日(星期日) 10:11
收件人：dev <dev@kafka.apache.org>
主 题：回复：回复：回复：回复：[Vote] KIP-571: Add option to force remove members in
StreamsResetter
Hi, all
Thanks a lot for your feedback!
According to the discussion, it seems we don't have some valid use cases
for removing specific dynamic members, I think it makes sense to encapsulate
the "get and delete" logic in adminClient. I will update the KIP shortly!
Thanks!
Feyman
------------------------------------------------------------------
发件人：Boyang Chen <reluctanthero...@gmail.com>
发送时间：2020年3月14日(星期六) 00:39
收件人：dev <dev@kafka.apache.org>
主 题：Re: 回复：回复：回复：[Vote] KIP-571: Add option to force remove members in
StreamsResetter
Thanks Matthias and Guozhang for the feedback. I'm not worrying too much
about the member.id exposure as we have done so in a couple of areas. As
for the recommended admin client change, I think it makes sense in an
encapsulation perspective. Maybe I'm still a bit hesitant as we are losing
the flexibility of closing only a subset of `dynamic members` potentially,
but we could always get back and address it if some user feels necessary to
have it.
My short answer would be, LGTM :)
Boyang
On Thu, Mar 12, 2020 at 5:26 PM Guozhang Wang <wangg...@gmail.com> wrote:
> Hi Matthias,
>
> About the AdminClient param API: that's a great point here. I think overall
> if users want to just "remove all members" they should not need to first
> get all the member.ids themselves, but instead internally the admin client
> can first issue a describe-group request to get all the member.ids, and
> then use them in the next issued leave-group request, all abstracted away
> from the users. With that in mind, maybe in
> RemoveMembersFromConsumerGroupOptions we can just introduce an overloaded
> flag param besides "members" that indicate "remove all"?
>
> Guozhang
>
> On Thu, Mar 12, 2020 at 2:59 PM Matthias J. Sax <mj...@apache.org> wrote:
>
> > Feyman,
> >
> > some more comments/questions:
> >
> > The description of `LeaveGroupRequest` is clear but it's unclear how
> > `MemberToRemove` should behave. Which parameter is required? Which is
> > optional? What is the relationship between both.
> >
> > The `LeaveGroupRequest` description clearly states that specifying a
> > `memberId` is optional if the `groupInstanceId` is provided. If
> > `MemberToRemove` applies the same pattern, it must be explicitly defined
> > in the KIP (and explained in the JavaDocs of `MemberToRemove`) because
> > we cannot expect that an admin-client users knows that internally a
> > `LeaveGroupRequest` is used nor what the semantics of a
> > `LeaveGroupRequest` are.
> >
> >
> > About Admin API:
> >
> > In general, I am also confused that we allow so specify a `memberId` at
> > all, because the `memberId` is an internal id that is not really exposed
> > to the user. Hence, from a AdminClient point of view, accepting a
> > `memberId` as input seems questionable to me? Of course, `memberId` can
> > be collected via `describeConsumerGroups()` but it will return the
> > `memberId` of _all_ consumer in the group and thus how would a user know
> > which member should be removed for a dynamic group (if an individual
> > member should be removed)?
> >
> > Hence, how can any user get to know the `memberId` of an individual
> > client in a programtic way?
> >
> > Also I am wondering in general, why the removal of single dynamic member
> > is important? In general, I would expect a short `session.timeout` for
> > dynamic groups and thus removing a specific member from the group seems
> > not to be an important feature -- for static groups we expect a long
> > `session.timeout` and a user can also identify individual clients via
> > `groupInstandId`, hence the feature makes sense for this case and is
> > straight forward to use.
> >
> >
> > About StreamsResetter:
> >
> > For this case we just say "remove all members" and thus the
> > `describeConsumerGroup` approach works. However, it seems to be a
> > special case?
> >
> > Or, if we expected that the "remove all members" use case is the norm,
> > why can't we make a change admin-client to directly accept a `group.id`?
> > The admin-client can internal first do a `DescribeGroupRequest` and
> > afterward corresponding `LeaveGroupRequest` -- i.e., instead of building
> > this pattern in `StreamsResetter` we build it directly into
> `AdminClient`.
> >
> > Last, for static group the main use case seems to be to remove an
> > individual member from the group but this feature is not covered by the
> > KIP: I think using `--force` to remove all members makes sense, but an
> > important second feature to remove an individual static member would
> > require it's own flag to specify a single `group.instance.id`.
> >
> >
> > Thoughts?
> >
> >
> > -Matthias
> >
> >
> >
> >
> >
> > On 3/11/20 8:43 PM, feyman2009 wrote:
> > > Hi, Sophie
> > > For 1) Sorry, I found that my expression is kind of misleading,
> > what I actually mean is: "if --force not specified, an exception saying
> > there are still active members on broker side will be thrown and
> > suggesting using StreamsResetter with --force", I just updated the KIP
> > page.
> > >
> > > For 2)
> > > I may also had some misleading expression previous, to clarify
> :
> > >
> > > Also, it's more efficient to just send a single "clear the group"
> > request vs sending a LeaveGroup
> > > request for every single member. What do you think?
> > > => the comparison is to send a single "clear the group" request vs
> > sending a "get members" + a "remove members" request since the
> > adminClient.removeMembersFromConsumerGroup support batch removal. We
> > don't need to send lots of leaveGroup requests for every single member.
> > >
> > > I can understand your point, but I think we could reuse the
> > current
> > > adminClient.removeMembersFromConsumerGroup interface effectively with
> > the KIP.
> > > What do you think?
> > >
> > > Thanks!
> > >
> > > Feyman
> > >
> > >
> > > ------------------------------------------------------------------
> > > 发件人：Sophie Blee-Goldman <sop...@confluent.io>
> > > 发送时间：2020年3月10日(星期二) 03:02
> > > 收件人：dev <dev@kafka.apache.org>; feyman2009 <feyman2...@aliyun.com>
> > > 主 题：Re: 回复：回复：[Vote] KIP-571: Add option to force remove
> > members in StreamsResetter
> > >
> > > Hey Feyman,
> > >
> > > 1) Regarding point 2 in your last email, if I understand correctly you
> > propose to change
> > > the current behavior of the reset tool when --force is not specified,
> > and wait for (up to)
> > > the session timeout for all members to be removed. I'm not sure we
> > should change this,
> > > especially now that we have a better way to handle the case when the
> > group is not empty:
> > > we should continue to throw an exception and fail fast, but can print
> > a message suggesting
> > > to use the new --force option to remove remaining group members. Why
> > make users wait
> > > for the session timeout when we've just added a new feature that means
> > they don't have to?
> > >
> > > 2) Regarding Matthias' question:
> > >
> > >> I am really wondering, if for a static group, we should allow users
> > toremove individual members? For a dynamic group this feature would not
> > > make much sense IMHO, because the `memberId` is not know by the user.
> > >
> > > I think his point is similar to what I was trying to get at earlier,
> > with the proposal to add a new
> > > #removeAllMembers API rather than an API to remove individual members
> > according to their
> > > memberId. As he explained, removing based on memberId is likely not
> > that useful in general.
> > > Also, it's not actually what we want to do here; maybe we should avoid
> > adding a new API
> > > that we think may be useful in other contexts (remove individual
> > member based on memberId),
> > > and just add the API we actually need (remove all members from group)
> > in this KIP? We can
> > > always add the "remove individual member by memberId" API at a later
> > point, if it turns out to
> > > actually be requested for specific reasons?
> > >
> > > Also, it's more efficient to just send a single "clear the group"
> > request vs sending a LeaveGroup
> > > request for every single member. What do you think?
> > >
> > >
> > >
> > >
> > > On Sat, Mar 7, 2020 at 1:41 AM feyman2009
> > <feyman2...@aliyun.com.invalid> wrote:
> > > Hi, Matthias
> > > Thanks, I updated the KIP to mention the deprecated and newly
> > added methods.
> > >
> > > 1. What happens is `groupInstanceId` is used for a dynamic group? What
> > > happens if both parameters are specified? What happens if `memberId`
> > > is specified for a static group?
> > >
> > > => my understanding is that the dynamic/static membership is member
> > level other than group level, and I think above questions could be
> > answered by the "Leave Group Logic Change" section in KIP-345:
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-345%3A+Introduce+static+membership+protocol+to+reduce+consumer+rebalances
> > ,
> > this KIP stays consistent with KIP-345.
> > >
> > > 2. About the `--force` option. Currently, StreamsResetter fails with
> an
> > > error if the consumer group is not empty. You state in your KIP that:
> > >
> > > > without --force, we need to wait for session timeout.
> > >
> > > Is this an intended behavior change if `--force` is not used or is the
> > > KIP description incorrect?
> > >
> > > => This is the intended behavior. For this part, I think there are
> > two ways to go:
> > > 1) (the implicit way) Not introducing the new "--force" option, with
> > this KIP, StreamsResetter will by default remove active members(with
> > long session timeout configured) on broker side
> > > 2) (the explicit way) Introduce the new "--force" option, users need
> > to explicitly specify --force to remove the active members. If --force
> > not specified, the StreamsResetter behaviour is as previous versions'.
> > >
> > > I think the two alternatives above are both feasible, personally I
> > prefer way 2.
> > >
> > > 3. For my own understanding: with the `--force` option, we intend to
> get
> > > all `memberIds` and send a "remove member" request for each with
> > > corresponding `memberId` to remove the member from the group
> > > (independent is the group is static or dynamic)?
> > >
> > > => Yeah, minor thing to mention is we will send the "remove member"
> > request for each member(could be dynamic member or static member) to
> > remove them from group
> > > for dynamic members, both "group.instance.id" and "member.id" will be
> > specified
> > > for dynamic members, only "member.id" will be specified
> > >
> > > 4. I am really wondering, if for a static group, we should allow users
> > to
> > > remove individual members? For a dynamic group this feature would not
> > > make much sense IMHO, because the `memberId` is not know by the user.
> > >
> > > => KIP-345 introduced the batch removal feature for both static
> > member and dynamic member, my understanding is that "allow users to
> > > remove individual members" could be useful for rolling bounce and
> > scale down accoding to KIP-345. KafkaAdminClient currently only support
> > static member removal and this KIP-571 enables dynamic member removal
> > for it, which is also consistent with the broker side logic. Users could
> > get the member.id (and group.instance.id if for static member) by
> > adminClient.describeConsumerGroups.
> > >
> > > Furthermore, I don't have an assumption that a consumer group should
> > contain only static or dynamic members, and I think KIP-345 and this KIP
> > don't need to be based on this assumption.
> > > You could correct me if I have the wrong understanding :)
> > >
> > > Thanks!
> > > Feyman
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > ------------------------------------------------------------------
> > > 发件人：Matthias J. Sax <mj...@apache.org>
> > > 发送时间：2020年3月6日(星期五) 02:20
> > > 收件人：dev <dev@kafka.apache.org>
> > > 主 题：Re: 回复：回复：[Vote] KIP-571: Add option to force remove
> > members in StreamsResetter
> > >
> > > Feyman,
> > >
> > > thanks a lot for the KIP. Overall LGTM. I have a few more comment and
> > > questions (sorry for the late reply):
> > >
> > >
> > > The KIP mentions that some constructors will be deprecated. Those
> should
> > > be listed explicitly. For example:
> > >
> > >
> > > public class MemberToRemove {
> > >
> > > // deprecated methods
> > >
> > > @Deprecated
> > > public MemberToRemove(String groupInstanceId);
> > >
> > > // new methods
> > >
> > > public MemberToRemove()
> > >
> > > public MemberToRemove withGroupInstanceId(String groupInstanceId)
> > >
> > > public MemberToRemove withMemberId(String memberId)
> > > }
> > >
> > > What happens is `groupInstanceId` is used for a dynamic group? What
> > > happens if both parameters are specified? What happens if `memberId`
> > > is specified for a static group?
> > >
> > >
> > > About the `--force` option. Currently, StreamsResetter fails with an
> > > error if the consumer group is not empty. You state in your KIP that:
> > >
> > >> without --force, we need to wait for session timeout.
> > >
> > > Is this an intended behavior change if `--force` is not used or is the
> > > KIP description incorrect?
> > >
> > > For my own understanding: with the `--force` option, we intend to get
> > > all `memberIds` and send a "remove member" request for each with
> > > corresponding `memberId` to remove the member from the group
> > > (independent is the group is static or dynamic)?
> > >
> > > I am really wondering, if for a static group, we should allow users to
> > > remove individual members? For a dynamic group this feature would not
> > > make much sense IMHO, because the `memberId` is not know by the user.
> > >
> > >
> > >
> > > -Matthias
> > >
> > >
> > > On 3/5/20 7:15 AM, Bill Bejeck wrote:
> > >> Thanks for the KIP. +1 (binding).
> > >
> > >> -Bill
> > >
> > >
> > >
> > >> On Wed, Mar 4, 2020 at 12:40 AM Guozhang Wang <wangg...@gmail.com>
> > >> wrote:
> > >
> > >>> Thanks, +1 from me (binding).
> > >>>
> > >>> On Tue, Mar 3, 2020 at 9:39 PM feyman2009 <feyman2...@aliyun.com>
> > >>> wrote:
> > >>>
> > >>>> Hi, Guozhang Thanks a lot for the advice, that make sense! I
> > >>>> have updated the KIP page with the operational steps of
> > >>>> StreamsResetter.
> > >>>>
> > >>>> Thanks! Feyman
> > >>>>
> > >>>> ------------------------------------------------------------------
> > >>>>
> > >>>>
> > > 发件人：Guozhang Wang <wangg...@gmail.com>
> > >>>> 发送时间：2020年3月3日(星期二) 14:22 收件人：dev <dev@kafka.apache.org>;
> > >>>> feyman2009 <feyman2...@aliyun.com> 主 题：Re: 回复：回复：[Vote]
> > >>>> KIP-571: Add option to force remove members in StreamsResetter
> > >>>>
> > >>>> Hello Feyman, thanks for the proposal!
> > >>>>
> > >>>> I read through the doc and overall it looks good to me.
> > >>>>
> > >>>> One minor thing I'd still like to point out is that, the
> > >>>> "removeMembersFromConsumerGroup" only sends a leave-group
> > >>>> request to the coordinator to let it remove the member,
> > >>>> however, if the member is still there alive and running then it
> > >>>> would soon be notified that it is no
> > >>> longer
> > >>>> a legal member of the group via heartbeats, and then
> > >>>> automatically tries
> > >>> to
> > >>>> re-join the group. So on the operational side, it is still
> > >>>> required that the following steps:
> > >>>>
> > >>>> 1) first stop the consumers (of streams instances), wait until
> > >>>> the shutdown is complete. 2) then use admin client in case the
> > >>>> stopped consumers are still registered at the broker side and
> > >>>> we do not want to wait for session timeout.
> > >>>>
> > >>>> Even with this KIP, people should still not skip step 1) above,
> > >>>> since otherwise the consumers would re-connect and re-join the
> > >>>> group
> > >>> immediately
> > >>>> still.
> > >>>>
> > >>>> In your doc you've already mentioned "Furthermore, users should
> > >>>> make sure all the stream applications are shutdown when running
> > >>>> StreamsResetter
> > >>> with
> > >>>> --force, otherwise it might trigger unexpected rebalance. "
> > >>>> What I'd want to clarify is that no matter if "--force" option
> > >>>> is enabled, this is
> > >>> always
> > >>>> the case that users should shutdown the streams instances
> > >>>> first, and then use the streams resetter :)
> > >>>>
> > >>>> As long as that is clarified in the proposal documentation, I'm
> > >>>> +1 on
> > >>> this
> > >>>> KIP.
> > >>>>
> > >>>>
> > >>>> Thanks again for the contribution, Guozhang
> > >>>>
> > >>>>
> > >>>> On Mon, Mar 2, 2020 at 6:31 AM feyman2009
> > >>>> <feyman2...@aliyun.com.invalid
> > >>>>
> > >>>> wrote: Hi, John Sorry, I have mistaken the KIP approval
> > >>>> standard, anyway, I will
> > >>> start
> > >>>> the PR soon and waiting for more binding approvals.
> > >>>>
> > >>>> Thanks! Feyman
> > >>>>
> > >>>>
> > >>>> ------------------------------------------------------------------
> > >>>>
> > >>>>
> > > 发件人：John Roesler <vvcep...@apache.org>
> > >>>> 发送时间：2020年3月2日(星期一) 22:00 收件人：dev
> > > <dev@kafka.apache.org> 主
> > >>>> 题：Re: 回复：回复：[Vote] KIP-571: Add option to force remove members
> > >>>> in StreamsResetter
> > >>>>
> > >>>> Hi Feyman,
> > >>>>
> > >>>> Sorry, but we actually need 3 binding votes for the KIP to
> > >>>> pass. Please feel free to keep bumping the thread until some
> > >>>> more committers can take
> > >>> a
> > >>>> look.
> > >>>>
> > >>>> By the way, you can totally start a PR, but we can’t merge it
> > >>>> until the KIP passes the vote.
> > >>>>
> > >>>> Thanks! John
> > >>>>
> > >>>> On Mon, Mar 2, 2020, at 00:24, feyman2009 wrote:
> > >>>>> Hi,all Since currently we have 1 binding and two non-binding
> > >>>>> +1, I will update the KIP-571 as adopted and initiate a PR
> > >>>>> shortly
> > >>>>>
> > >>>>> Thanks! Feyman
> > >>>>>
> > >>>>>
> > >>>>> ------------------------------------------------------------------
> > >>>>>
> > >>>>>
> > > 发件人：Sophie Blee-Goldman <sop...@confluent.io>
> > >>>>> 发送时间：2020年2月28日(星期五) 10:17 收件人：dev
> > > <dev@kafka.apache.org> 主
> > >>>>> 题：Re: 回复：[Vote] KIP-571: Add option to force remove members
> > >>>>> in
> > >>>> StreamsResetter
> > >>>>>
> > >>>>> Thanks for the KIP, +1 (non-binding)
> > >>>>>
> > >>>>> On Thu, Feb 27, 2020 at 12:40 PM Boyang Chen <
> > >>> reluctanthero...@gmail.com
> > >>>>>
> > >>>>> wrote:
> > >>>>>
> > >>>>>> Thanks Feyman, +1 (non-binding)
> > >>>>>>
> > >>>>>> On Thu, Feb 27, 2020 at 9:25 AM John Roesler
> > >>>>>> <vvcep...@apache.org>
> > >>>> wrote:
> > >>>>>>
> > >>>>>>> Thanks for the proposal!
> > >>>>>>>
> > >>>>>>> I'm +1 (binding) -John
> > >>>>>>>
> > >>>>>>> On Wed, Feb 26, 2020, at 19:41, feyman2009 wrote:
> > >>>>>>>> Updated with the KIP link:
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>
> > >>>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-571%3A+Add+opti
> > > on+to+force+remove+members+in+StreamsResetter
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>
> > >>>
> > > ------------------------------------------------------------------
> > >>>>>>>> 发件人：feyman2009 <feyman2...@aliyun.com.INVALID> 发送时
> > >>>>>>>> 间：2020年2月27日(星期四) 09:38 收件人：dev <dev@kafka.apache.org>
> > >>>>>>>> 主 题：[Vote] KIP-571: Add option to force remove members
> > >>>>>>>> in
> > >>>>>> StreamsResetter
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Hi, all I would like to start a vote on KIP-571: Add
> > >>>>>>>> option to force
> > >>>> remove
> > >>>>>>>> members in StreamsResetter .
> > >>>>>>>>
> > >>>>>>>> Thanks! Feyman
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>> -- -- Guozhang
> > >>>>
> > >>>>
> > >>>>
> > >>>
> > >>> -- -- Guozhang
> > >>>
> > >
> > >
> > >
> >
> >
>
> --
> -- Guozhang
>
