Hi Luke, Thanks for updating the KIP. I just have a minor request. Could you fully describe the changes to the Subscription public class in the KIP? I think that it would be better than just saying that the generation will be added to id.
Otherwise, the KIP LGTM. Thanks, David On Mon, Nov 29, 2021 at 4:29 AM Luke Chen <show...@gmail.com> wrote: > > 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 > >>> > >>
