Thanks Luke, I failed to mention in my reply to Ziming, but I did add this sentence to the end of the "visibility" section:
The metadata shell must also avoid showing partially committed metadata > transactions. -David On Tue, Sep 27, 2022 at 10:20 PM Luke Chen <show...@gmail.com> wrote: > 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 > > > > > > > > > > -- -David