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 <dengziming1...@gmail.com> 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 <davidart...@apache.org> 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
