Hi devs, Welcome to provide feedback. If there are no other comments, I'll start a vote tomorrow.
Thank you. Luke On Mon, Nov 22, 2021 at 4:16 PM Luke Chen <show...@gmail.com> wrote: > Hello David, > > For (3): > > > > * I suppose that we could add a `generation` field to the JoinGroupRequest > instead to do the fencing that you describe while handling the sentinel in > the assignor directly. If we would add the `generation` to the request > itself, would we need the `generation` in the subscription protocol as > well?* > > On second thought, I think this is not better than adding `generation` > field in the subscription protocol, because I think we don't have to do any > generation validation on joinGroup request. The purpose of > `joinGroupRequest` is to accept any members to join this group, even if the > member is new or ever joined or what. As long as we have the generationId > in the subscription metadata, the consumer lead can leverage the info to > ignore the old ownedPartitions (or do other handling), and the rebalance > can still complete successfully and correctly. On the other hand, if we did > the generation check on JoinGroupRequest, and return `ILLEGAL_GENERATION` > back to consumer, the consumer needs to clear its generation info and > rejoin the group to continue the rebalance. It needs more request/response > network and slow down the rebalance. > > So I think we should add the `generationId` field into Subscription > protocol to achieve what we want. > > Thank you. > Luke > > On Thu, Nov 18, 2021 at 8:51 PM Luke Chen <show...@gmail.com> wrote: > >> Hi David, >> Thanks for your feedback. >> >> I've updated the KIP for your comments (1)(2). >> For (3), it's a good point! Yes, we didn't deserialize the subscription >> metadata on broker side, and it's not necessary to add overhead on broker >> side. And, yes, I think we can fix the original issue by adding a >> "generation" field into `JoinGroupRequest` instead, and also add a field >> into `JoinGroupResponse` in `JoinGroupResponseMember` field. That way, the >> broker can identify the old member from `JoinGroupRequest`. And the >> assignor can also get the "generation" info via the `Subscription` instance. >> >> I'll update the KIP to add "generation" field into `JoinGroupRequest` and >> `JoinGroupResponse`, if there is no other options. >> >> Thank you. >> Luke >> >> >> On Tue, Nov 16, 2021 at 12:31 AM David Jacot <dja...@confluent.io.invalid> >> wrote: >> >>> Hi Luke, >>> >>> Thanks for the KIP. Overall, I think that the motivation makes sense. I >>> have a couple of comments/questions: >>> >>> 1. In the Public Interfaces section, it would be great if you could put >>> the >>> end state not the current one. >>> >>> 2. Do we need to update the Subscription class to expose the >>> generation? If so, it would be great to mention it in the Public >>> Interfaces section as well. >>> >>> 3. You mention that the broker will set the generation if the >>> subscription >>> contains a sentinel value (-1). As of today, the broker does not parse >>> the subscription so I am not sure how/why we would do this. I suppose >>> that we could add a `generation` field to the JoinGroupRequest instead >>> to do the fencing that you describe while handling the sentinel in the >>> assignor directly. If we would add the `generation` to the request >>> itself, >>> would we need the `generation` in the subscription protocol as well? >>> >>> Best, >>> David >>> >>> On Fri, Nov 12, 2021 at 3:31 AM Luke Chen <show...@gmail.com> wrote: >>> > >>> > Hi all, >>> > >>> > I'd like to start the discussion for KIP-792: Add "generation" field >>> into >>> > consumer protocol. >>> > >>> > The goal of this KIP is to allow assignor/consumer coordinator/group >>> > coordinator to have a way to identify the out-of-date >>> members/assignments. >>> > >>> > Detailed description can be found here: >>> > >>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=191336614 >>> > >>> > Any feedback is welcome. >>> > >>> > Thank you. >>> > Luke >>> >>
