Thanks for the reviews, everyone. I’ve started a VOTE thread -David
On Fri, Sep 23, 2022 at 00:55 deng ziming <[email protected]> wrote: > David, > Thanks for the feedback about #2 and #3, I'm OK with them. > I also mentioned the visibility in the MetadataShell in #1, do you have any > thoughts? > > -- > Best, > Ziming > > On Wed, Sep 21, 2022 at 10:56 PM David Arthur <[email protected]> wrote: > > > Ziming, thanks for the feedback! Let me know your thoughts on #2 and #3 > > > > 1. Good idea. I consolidated all the details of record visibility into > > that section. > > > > 2. I'm not sure we can always know the number of records ahead of time > > for a transaction. One future use case is likely for the ZK data > > migration which will have an undetermined number of records. I would > > be okay with some short textual fields like "name" for the Begin > > record and "reason" for the Abort record. These could also be tagged > > fields if we don't want to always include them in the records. > > > > 3. The metadata records end up in org.apache.kafka.common.metadata, so > > maybe we can avoid Metadata in the name since it's kind of implicit. > > I'd be okay with [Begin|End|Abort]TransactionRecord. > > > > -David > > > > On Mon, Sep 19, 2022 at 10:58 PM deng ziming <[email protected]> > > wrote: > > > > > > Hello David, > > > Thanks for the KIP, certainly it makes sense, I left some minor > > questions. > > > > > > 1. In “Record Visibility” section you declare visibility in the > > controller, in “Broker Support” you mention visibility in the broker, we > > can put them together, and I think we can also describe visibility in the > > MetadataShell since it is also a public interface. > > > > > > 2. In “Public interfaces” section, I found that the “BeginMarkerRecord” > > has no fields, should we include some auxiliary attributes to help parse > > the transaction, for example, number of records in this transaction. > > > > > > 3. The record name seems vague, and we already have a > > `EndTransactionMarker` class in `org.apache.kafka.common.record`, how > about > > `BeginMetadataTransactionRecord`? > > > > > > - - > > > Best, > > > Ziming > > > > > > > On Sep 10, 2022, at 1:13 AM, David Arthur <[email protected]> > > wrote: > > > > > > > > Starting a new thread to avoid issues with mail client threading. > > > > > > > > Original thread follows: > > > > > > > > Hey folks, I'd like to start a discussion on the idea of adding > > > > transactions in the KRaft controller. This will allow us to overcome > > > > the current limitation of atomic batch sizes in Raft which lets us do > > > > things like create topics with a huge number of partitions. > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-868+Metadata+Transactions > > > > > > > > Thanks! > > > > > > > > --- > > > > > > > > Colin McCabe said: > > > > > > > > Thanks for this KIP, David! > > > > > > > > In the "motivation" section, it might help to give a concrete example > > > > of an operation we want to be atomic. My favorite one is probably > > > > CreateTopics since it's easy to see that we want to create all of a > > > > topic or none of it, and a topic could be a potentially unbounded > > > > number of records (although hopefully people have reasonable create > > > > topic policy classes in place...) > > > > > > > > In "broker support", it would be good to clarify that we will buffer > > > > the records in the MetadataDelta and not publish a new MetadataImage > > > > until the transaction is over. This is an implementation detail, but > > > > it's a simple one and I think it will make it easier to understand > how > > > > this works. > > > > > > > > In the "Raft Transactions" section of "Rejected Alternatives," I'd > add > > > > that managing buffering in the Raft layer would be a lot less > > > > efficient than doing it in the controller / broker layer. We would > end > > > > up accumulating big lists of records which would then have to be > > > > applied when the transaction completed, rather than building up a > > > > MetadataDelta (or updating the controller state) incrementally. > > > > > > > > Maybe we want to introduce the concept of "last stable offset" to be > > > > the last committed offset that is NOT part of an ongoing transaction? > > > > Just a nomenclature suggestion... > > > > > > > > best, > > > > Colin > > > > > > > > > -- > > David Arthur > > >
