Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
Closing this vote. With four binding +1s and zero -1s, this vote passes. Binding +1: * David Arthur * Guozhang Wang * Tom Bentley * Colin McCabe Non-binding +1: * Ron Dagostino Thanks everyone! David On Tue, May 11, 2021 at 1:07 PM Colin McCabe wrote: > 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 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 > > > wrote: > > > > > > > > > > > LGTM! Thanks David. > > > > > > > > > > > > On Thu, May 6, 2021 at 10:03 AM Ron Dagostino > > > > wrote: > > > > > > > > > > > > > Thanks again for the KIP, David. +1 (non-binding) from me. > > > > > > > > > > > > > > Ron > > > > > > > > > > > > > > On Tue, May 4, 2021 at 11:21 AM David Arthur > > > > 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 > > > -- David Arthur
Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
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 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 > > wrote: > > > > > > > > > LGTM! Thanks David. > > > > > > > > > > On Thu, May 6, 2021 at 10:03 AM Ron Dagostino > > wrote: > > > > > > > > > > > Thanks again for the KIP, David. +1 (non-binding) from me. > > > > > > > > > > > > Ron > > > > > > > > > > > > On Tue, May 4, 2021 at 11:21 AM David Arthur > > 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 >
Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
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 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 > wrote: > > > > > > > LGTM! Thanks David. > > > > > > > > On Thu, May 6, 2021 at 10:03 AM Ron Dagostino > wrote: > > > > > > > > > Thanks again for the KIP, David. +1 (non-binding) from me. > > > > > > > > > > Ron > > > > > > > > > > On Tue, May 4, 2021 at 11:21 AM David Arthur > 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
Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
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 wrote: > > > > > LGTM! Thanks David. > > > > > > On Thu, May 6, 2021 at 10:03 AM Ron Dagostino wrote: > > > > > > > Thanks again for the KIP, David. +1 (non-binding) from me. > > > > > > > > Ron > > > > > > > > On Tue, May 4, 2021 at 11:21 AM David Arthur 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 > > > > > >
Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
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 wrote: > > > LGTM! Thanks David. > > > > On Thu, May 6, 2021 at 10:03 AM Ron Dagostino wrote: > > > > > Thanks again for the KIP, David. +1 (non-binding) from me. > > > > > > Ron > > > > > > On Tue, May 4, 2021 at 11:21 AM David Arthur 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 > > >
Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
Hi David, Thanks for the KIP, +1 binding. Tom On Thu, May 6, 2021 at 7:16 PM Guozhang Wang wrote: > LGTM! Thanks David. > > On Thu, May 6, 2021 at 10:03 AM Ron Dagostino wrote: > > > Thanks again for the KIP, David. +1 (non-binding) from me. > > > > Ron > > > > On Tue, May 4, 2021 at 11:21 AM David Arthur 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 >
Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
LGTM! Thanks David. On Thu, May 6, 2021 at 10:03 AM Ron Dagostino wrote: > Thanks again for the KIP, David. +1 (non-binding) from me. > > Ron > > On Tue, May 4, 2021 at 11:21 AM David Arthur 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
Re: [VOTE] KIP-730: Producer ID generation in KRaft mode
Thanks again for the KIP, David. +1 (non-binding) from me. Ron On Tue, May 4, 2021 at 11:21 AM David Arthur 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 >
[VOTE] KIP-730: Producer ID generation in KRaft mode
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
