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 >>> > > >
signature.asc
Description: OpenPGP digital signature
