Hi David, I think there's still an unanswered question from Ziming: > I think we can also describe visibility in the MetadataShell since it is also a public interface.
I think this suggestion makes sense. But I'm still +1 for current proposal. Thanks. Luke On Tue, Sep 27, 2022 at 11:11 PM David Arthur <davidart...@apache.org> wrote: > Thanks for the reviews, everyone. I’ve started a VOTE thread > > -David > > On Fri, Sep 23, 2022 at 00:55 deng ziming <dengziming1...@gmail.com> > 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 <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 > > > > > >
