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 > > > > > > >
