Hi, David, Thanks for the explanation. Sounds good to me.
Jun On Tue, Oct 25, 2022 at 12:19 PM David Jacot <david.ja...@gmail.com> wrote: > Hi Jun, > > That’s right. I propose to tackle this in a separate KIP because the > downgrade need some thinking. > > I would like to point out that the current group coordinator does not > support downgrades either so we don’t make it worst. I am personally in > favor of doing this KIP before we release the new protocol. Supporting > downgrades is important. > > We could of course extend the current KIP for this as well but I think that > the current KIP is large and complicated enough. > > Does this sound reasonable? > > Best, > David > > Le mar. 25 oct. 2022 à 19:41, Jun Rao <j...@confluent.io.invalid> a écrit : > > > Hi, David, > > > > Thanks for the reply. > > > > The KIP mentioned downgrade support in a future KIP. So, with this KIP, > > once the new records have been generated on the coordinator, there is no > > path to downgrade the broker, is that correct? > > > > Thanks, > > > > Jun > > > > On Tue, Oct 25, 2022 at 7:10 AM David Jacot <dja...@confluent.io.invalid > > > > wrote: > > > > > Hi Jun, > > > > > > 90.1 You're right. That's a miss on my side. > > > 90.2 That makes sense. Relying on > > > ConsumerGroupTargetAssignmentMetadataValue.AssignmentEpoch should be > > > enough here. > > > > > > 91. When a client side assignor is used, the assignment is computed > > > asynchronously. While it is computed for the group at epoch X, the > > > group may have already advanced to epoch X+1 due to another event > > > (e.g. new member joined). In this case, we have chosen to install the > > > assignment computed for epoch X and to trigger a new assignment > > > computation right away. So if the group topology changes rapidly, the > > > assignment may lag a bit behind but it will eventually converge. For > > > context, the alternative would have been to cancel the assignment and > > > to trigger a new one immediately but this approach is prone to a live > > > lock. For instance, if a member joins and leaves all the time, there > > > is a chance that a new assignment is never computed. I have extended > > > that phrase to be more explicit. > > > > > > Thanks for the comments. > > > > > > Best, > > > David > > > > > > On Mon, Oct 24, 2022 at 7:28 PM Jun Rao <j...@confluent.io.invalid> > > wrote: > > > > > > > > Hi, David, > > > > > > > > Thanks for the updated KIP. A few more comments. > > > > > > > > 90. ConsumerGroupTargetAssignmentMemberValue: > > > > 90.1 Do we need to include MemberId here given that it's in the key > > > already? > > > > 90.2 Since there is no new record if the new member assignment is the > > > same, > > > > it seems that AssignmentEpoch doesn't always reflect the correct > > > assignment > > > > epoch? If so, do we still need this field? Could we just depend > > > > on ConsumerGroupTargetAssignmentMetadataValue.AssignmentEpoch? > > > > > > > > 91. "The AssignmentEpoch corresponds to the group epoch used to > compute > > > the > > > > assignment. It is not necessarily the last one." Could you explain > what > > > "It > > > > is not necessarily the last one." means? > > > > > > > > Thanks, > > > > > > > > Jun > > > > > > > > > > > > On Mon, Oct 24, 2022 at 7:49 AM Magnus Edenhill <mag...@edenhill.se> > > > wrote: > > > > > > > > > Hi, one minor comment on the latest update: > > > > > > > > > > > > > > > Den mån 24 okt. 2022 kl 16:26 skrev David Jacot > > > > > <dja...@confluent.io.invalid > > > > > >: > > > > > > > > > > > * Jason pointed out that the member id handling is a tad weird. > The > > > > > > group coordinator generates the member id and then trusts the > > member > > > > > > when it rejoins the group. This also implies that the client > could > > > > > > directly generate its member id and the group coordinator will > > accept > > > > > > it. It seems better to directly let the client generate id > instead > > of > > > > > > relying on the group coordinator. I have updated the KIP in this > > > > > > direction. Note that the new APIs still use a string for the > member > > > id > > > > > > in order to remain consistent with the existing APIs. > > > > > > > > > > > > > > > > We had a similar discussion for id generation in KIP-714 and I'd > > advise > > > > > against client-side id generation for a couple of reasons: > > > > > - it is much more likely for the client side prng to be poorly > > > seeded, or > > > > > incorrectly implemented, than the server side. > > > > > This risks two different consumer instances generating the same > > id. > > > > > - it adds an extra dependency on the client, a uuid > library/module, > > > which > > > > > brings with it the usual plethora > > > > > of linking conflicts, package availability issues, etc. > > > > > - as for trusting the authenticity of the id; with server-side > > > generation > > > > > we at least have a (future) possibility for verifying the id, would > > it > > > ever > > > > > become an issue. > > > > > > > > > > > > > > > Regards, > > > > > Magnus > > > > > > > > > > >
