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 >
