Thanks, David. I don't feel strongly about preserving the extra records in the snapshot, so let's just leave it as is. +1 (binding)
cheers, Colin On Fri, May 7, 2021, at 09:51, David Arthur wrote: > Colin, thanks for the feedback. I like the record name you proposed. I've > also updated the first paragraph in the Controller section to: > > In both ZK and KRaft modes, the controller will now be responsible for > > generating new blocks of IDs and persisting the latest generated block. In > > ZK mode, the controller will use the existing ZNode and JSON format for > > persistence. In KRaft mode, the controller will commit a new record to the > > metadata log. Since the controller (in either mode) uses a single threaded > > event model, we can simply calculate the next block of IDs based on what is > > currently in memory. The controller will need to persist generated PID > > block so it can be “consumed” and never used again. > > > > This doesn't change any semantics of the KIP, just clarifies the > implementation (as you suggested). > > As for including extra records in the snapshot, I agree it might be > useful, but I wonder if it might violate assumptions about snapshots. If > snapshots are meant to contain a minimal set of records to represent the > state of the metadata, including additional records (one per broker in this > case) might go against that. Either way, can we consider this an > implementation detail and defer the decision? > > Thanks to all who have voted so far! > -David > > On Thu, May 6, 2021 at 4:01 PM Colin McCabe <cmcc...@apache.org> wrote: > > > Sorry, I meant to write "AllocateProducerIdsRecord" in the previous > > message. -C. > > > > On Thu, May 6, 2021, at 12:58, Colin McCabe wrote: > > > Hi David, > > > > > > Thanks for the KIP -- it looks good. > > > > > > It seems like we should be clear that the new RPC should be used for > > > both the ZK and KRaft cases. I think that is implied, but it would be > > > good to spell it out just to be clear. As the KIP explains, this is > > > needed for the bridge release. > > > > > > I think AllocateProducersIdRecord would be a nicer name than > > > ProducerIdRecord -- what do you think? > > > > > > In the snapshot, does it make sense to store the latest producer ID > > > allocation record for every broker? This might be useful for debugging > > > purposes, and it's unlikely to be that many records.... On the other > > > hand, as you mention, we only really need the highest one for > > > correctness. > > > > > > best, > > > Colin > > > > > > > > > On Thu, May 6, 2021, at 11:53, Tom Bentley wrote: > > > > Hi David, > > > > > > > > Thanks for the KIP, +1 binding. > > > > > > > > Tom > > > > > > > > On Thu, May 6, 2021 at 7:16 PM Guozhang Wang <wangg...@gmail.com> > > wrote: > > > > > > > > > LGTM! Thanks David. > > > > > > > > > > On Thu, May 6, 2021 at 10:03 AM Ron Dagostino <rndg...@gmail.com> > > wrote: > > > > > > > > > > > Thanks again for the KIP, David. +1 (non-binding) from me. > > > > > > > > > > > > Ron > > > > > > > > > > > > On Tue, May 4, 2021 at 11:21 AM David Arthur <mum...@gmail.com> > > wrote: > > > > > > > > > > > > > Hello everyone, I'd like to start the vote on KIP-730 which adds > > a new > > > > > > RPC > > > > > > > for producer ID generation in KRaft mode. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-730%3A+Producer+ID+generation+in+KRaft+mode > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > David Arthur > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > -- Guozhang > > > > > > > > > > > > > > > > > -- > David Arthur >
