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
