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 <mum...@gmail.com> 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 <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 >
