I had missed the David's message yesterday about the naming for transaction version vs transaction protocol version.
After some offline discussion with Jun, Artem, and David, we agreed that transaction version is simpler and conveys more than just protocol changes (flexible records for example) I will update the KIP as well as KIP-890 Thanks, Justine On Tue, Apr 2, 2024 at 2:50 PM Justine Olshan <jols...@confluent.io> wrote: > Updated! > > Justine > > On Tue, Apr 2, 2024 at 2:40 PM Jun Rao <j...@confluent.io.invalid> wrote: > >> Hi, Justine, >> >> Thanks for the reply. >> >> 21. Sounds good. It would be useful to document that. >> >> 22. Should we add the IV in "metadata.version=17 has no dependencies" too? >> >> Jun >> >> >> On Tue, Apr 2, 2024 at 11:31 AM Justine Olshan >> <jols...@confluent.io.invalid> >> wrote: >> >> > Jun, >> > >> > 21. Next producer ID field doesn't need to be populated for TV 1. We >> don't >> > have the same need to retain this since it is written directly to the >> > transaction log in InitProducerId. It is only needed for KIP-890 part 2 >> / >> > TV 2. >> > >> > 22. We can do that. >> > >> > Justine >> > >> > On Tue, Apr 2, 2024 at 10:41 AM Jun Rao <j...@confluent.io.invalid> >> wrote: >> > >> > > Hi, Justine, >> > > >> > > Thanks for the reply. >> > > >> > > 21. What about the new NextProducerId field? Will that be populated >> with >> > TV >> > > 1? >> > > >> > > 22. In the dependencies output, should we show both IV and level for >> > > metadata.version too? >> > > >> > > Jun >> > > >> > > On Mon, Apr 1, 2024 at 4:43 PM Justine Olshan >> > <jols...@confluent.io.invalid >> > > > >> > > wrote: >> > > >> > > > Hi Jun, >> > > > >> > > > 20. I can update the KIP. >> > > > >> > > > 21. This is used to complete some of the work with KIP-360. (We use >> > > > previous producer ID there, but never persisted it which was in the >> KIP >> > > > >> > > >> > >> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89068820 >> ) >> > > > The KIP also mentions including previous epoch but we explained in >> this >> > > KIP >> > > > how we can figure this out. >> > > > >> > > > Justine >> > > > >> > > > >> > > > >> > > > On Mon, Apr 1, 2024 at 3:56 PM Jun Rao <j...@confluent.io.invalid> >> > wrote: >> > > > >> > > > > Hi, Justine, >> > > > > >> > > > > Thanks for the updated KIP. A couple of more comments. >> > > > > >> > > > > 20. Could we show the output of version-mapping? >> > > > > >> > > > > 21. "Transaction version 1 will include the flexible fields in the >> > > > > transaction state log, and transaction version 2 will include the >> > > changes >> > > > > to the transactional protocol as described by KIP-890 (epoch bumps >> > and >> > > > > implicit add partitions.)" >> > > > > So TV 1 enables the writing of new tagged fields like >> > PrevProducerId? >> > > > But >> > > > > those fields are only usable after the epoch bump, right? What >> > > > > functionality does TV 1 achieve? >> > > > > >> > > > > Jun >> > > > > >> > > > > >> > > > > On Mon, Apr 1, 2024 at 2:06 PM Justine Olshan >> > > > <jols...@confluent.io.invalid >> > > > > > >> > > > > wrote: >> > > > > >> > > > > > I have also updated the KIP to mention the feature tool's >> > --metadata >> > > > flag >> > > > > > will be deprecated. >> > > > > > It will still work for users as they learn the new flag, but a >> > > warning >> > > > > > indicating the alternatives will be shown. >> > > > > > >> > > > > > Justine >> > > > > > >> > > > > > On Thu, Mar 28, 2024 at 11:03 AM Justine Olshan < >> > > jols...@confluent.io> >> > > > > > wrote: >> > > > > > >> > > > > > > Hi Jun, >> > > > > > > >> > > > > > > For both transaction state and group coordinator state, there >> are >> > > > only >> > > > > > > version 0 records. >> > > > > > > KIP-915 introduced flexible versions, but it was never put to >> > use. >> > > MV >> > > > > has >> > > > > > > never gated these. This KIP will do that. I can include this >> > > context >> > > > in >> > > > > > the >> > > > > > > KIP. >> > > > > > > >> > > > > > > I'm happy to modify his 1 and 2 to 0 and 1. >> > > > > > > >> > > > > > > Justine >> > > > > > > >> > > > > > > On Thu, Mar 28, 2024 at 10:57 AM Jun Rao >> > <j...@confluent.io.invalid >> > > > >> > > > > > wrote: >> > > > > > > >> > > > > > >> Hi, David, >> > > > > > >> >> > > > > > >> Thanks for the reply. >> > > > > > >> >> > > > > > >> Historically, the format of all records were controlled by >> MV. >> > > Now, >> > > > > > >> records >> > > > > > >> in _offset_commit will be controlled by >> > > `group.coordinator.version`, >> > > > > is >> > > > > > >> that right? It would be useful to document that. >> > > > > > >> >> > > > > > >> Also, we should align on the version numbering. >> "kafka-feature >> > > > > disable" >> > > > > > >> says "Disable one or more feature flags. This is the same as >> > > > > downgrading >> > > > > > >> the version to zero". So, in the `group.coordinator.version' >> > case, >> > > > we >> > > > > > >> probably should use version 0 for the old consumer protocol. >> > > > > > >> >> > > > > > >> Jun >> > > > > > >> >> > > > > > >> On Thu, Mar 28, 2024 at 2:13 AM Andrew Schofield < >> > > > > > >> andrew_schofield_j...@outlook.com> wrote: >> > > > > > >> >> > > > > > >> > Hi David, >> > > > > > >> > I agree that we should use the same mechanism to gate >> KIP-932 >> > > once >> > > > > > that >> > > > > > >> > feature reaches production readiness. The precise details >> of >> > the >> > > > > > values >> > > > > > >> > will >> > > > > > >> > depend upon the current state of all these flags when that >> > > release >> > > > > > >> comes. >> > > > > > >> > >> > > > > > >> > Thanks, >> > > > > > >> > Andrew >> > > > > > >> > >> > > > > > >> > > On 28 Mar 2024, at 07:11, David Jacot >> > > > <dja...@confluent.io.INVALID >> > > > > > >> > > > > > >> > wrote: >> > > > > > >> > > >> > > > > > >> > > Hi, Jun, Justine, >> > > > > > >> > > >> > > > > > >> > > Regarding `group.coordinator.version`, the idea is to >> use it >> > > to >> > > > > gate >> > > > > > >> > > records and APIs of the group coordinator. The first use >> > case >> > > > will >> > > > > > be >> > > > > > >> > > KIP-848. We will use version 2 of the flag to gate all >> the >> > new >> > > > > > records >> > > > > > >> > and >> > > > > > >> > > the new ConsumerGroupHeartbeat/Describe APIs present in >> AK >> > > 3.8. >> > > > So >> > > > > > >> > version >> > > > > > >> > > 1 will be the only the old protocol and version 2 will be >> > the >> > > > > > >> currently >> > > > > > >> > > implemented new protocol. I don't think that we have any >> > > > > dependency >> > > > > > on >> > > > > > >> > the >> > > > > > >> > > metadata version at the moment. The changes are >> orthogonal. >> > I >> > > > > think >> > > > > > >> that >> > > > > > >> > we >> > > > > > >> > > could mention KIP-848 as the first usage of this flag in >> the >> > > > KIP. >> > > > > I >> > > > > > >> will >> > > > > > >> > > also update KIP-848 to include it when this KIP is >> accepted. >> > > > > Another >> > > > > > >> use >> > > > > > >> > > case is the Queues KIP. I think that we should also use >> this >> > > new >> > > > > > flag >> > > > > > >> to >> > > > > > >> > > gate it. >> > > > > > >> > > >> > > > > > >> > > Best, >> > > > > > >> > > David >> > > > > > >> > > >> > > > > > >> > > On Thu, Mar 28, 2024 at 1:14 AM Jun Rao >> > > > <j...@confluent.io.invalid >> > > > > > >> > > > > > >> > wrote: >> > > > > > >> > > >> > > > > > >> > >> Hi, Justine, >> > > > > > >> > >> >> > > > > > >> > >> Thanks for the reply. >> > > > > > >> > >> >> > > > > > >> > >> So, "dependencies" and "version-mapping" will be added >> to >> > > both >> > > > > > >> > >> kafka-feature and kafka-storage? Could we document that >> in >> > > the >> > > > > tool >> > > > > > >> > format >> > > > > > >> > >> section? >> > > > > > >> > >> >> > > > > > >> > >> Jun >> > > > > > >> > >> >> > > > > > >> > >> On Wed, Mar 27, 2024 at 4:01 PM Justine Olshan >> > > > > > >> > >> <jols...@confluent.io.invalid> >> > > > > > >> > >> wrote: >> > > > > > >> > >> >> > > > > > >> > >>> Ok. I can remove the info from the describe output. >> > > > > > >> > >>> >> > > > > > >> > >>> Dependencies is needed for the storage tool because we >> > want >> > > to >> > > > > > make >> > > > > > >> > sure >> > > > > > >> > >>> the desired versions we are setting will be valid. >> Version >> > > > > mapping >> > > > > > >> > should >> > > > > > >> > >>> be for both tools since we have --release-version for >> both >> > > > > tools. >> > > > > > >> > >>> >> > > > > > >> > >>> I was considering changing the IV strings, but I wasn't >> > sure >> > > > if >> > > > > > >> there >> > > > > > >> > >> would >> > > > > > >> > >>> be some disagreement with the decision. Not sure if >> that >> > > > breaks >> > > > > > >> > >>> compatibility etc. Happy to hear everyone's thoughts. >> > > > > > >> > >>> >> > > > > > >> > >>> Justine >> > > > > > >> > >>> >> > > > > > >> > >>> On Wed, Mar 27, 2024 at 3:36 PM Jun Rao >> > > > > <j...@confluent.io.invalid >> > > > > > > >> > > > > > >> > >> wrote: >> > > > > > >> > >>> >> > > > > > >> > >>>> Hi, Justine, >> > > > > > >> > >>>> >> > > > > > >> > >>>> Thanks for the reply. >> > > > > > >> > >>>> >> > > > > > >> > >>>> Having "kafka-feature dependencies" seems enough to >> me. >> > We >> > > > > don't >> > > > > > >> need >> > > > > > >> > >> to >> > > > > > >> > >>>> include the dependencies in the output of >> "kafka-feature >> > > > > > describe". >> > > > > > >> > >>>> >> > > > > > >> > >>>> We only support "dependencies" in kafka-feature, not >> > > > > > >> kafka-storage. We >> > > > > > >> > >>>> probably should do the same for "version-mapping". >> > > > > > >> > >>>> >> > > > > > >> > >>>> bin/kafka-features.sh downgrade --feature >> > > metadata.version=16 >> > > > > > >> > >>>> --transaction.protocol.version=2 >> > > > > > >> > >>>> We need to add the --feature flag for the second >> feature, >> > > > > right? >> > > > > > >> > >>>> >> > > > > > >> > >>>> In "kafka-features.sh describe", we only show the IV >> > string >> > > > for >> > > > > > >> > >>>> metadata.version. Should we also show the level >> number? >> > > > > > >> > >>>> >> > > > > > >> > >>>> Thanks, >> > > > > > >> > >>>> >> > > > > > >> > >>>> Jun >> > > > > > >> > >>>> >> > > > > > >> > >>>> On Wed, Mar 27, 2024 at 1:52 PM Justine Olshan >> > > > > > >> > >>>> <jols...@confluent.io.invalid> >> > > > > > >> > >>>> wrote: >> > > > > > >> > >>>> >> > > > > > >> > >>>>> I had already included this example >> > > > > > >> > >>>>> bin/kafka-features.sh downgrade --feature >> > > > metadata.version=16 >> > > > > > >> > >>>>> --transaction.protocol.version=2 // Throws error if >> > > metadata >> > > > > > >> version >> > > > > > >> > >>> is < >> > > > > > >> > >>>>> 16, and this would be an upgrade >> > > > > > >> > >>>>> But I have updated the KIP to explicitly say the text >> > you >> > > > > > >> mentioned. >> > > > > > >> > >>>>> >> > > > > > >> > >>>>> Justine >> > > > > > >> > >>>>> >> > > > > > >> > >>>>> On Wed, Mar 27, 2024 at 1:41 PM José Armando García >> > Sancio >> > > > > > >> > >>>>> <jsan...@confluent.io.invalid> wrote: >> > > > > > >> > >>>>> >> > > > > > >> > >>>>>> Hi Justine, >> > > > > > >> > >>>>>> >> > > > > > >> > >>>>>> See my comment below. >> > > > > > >> > >>>>>> >> > > > > > >> > >>>>>> On Wed, Mar 27, 2024 at 1:31 PM Justine Olshan >> > > > > > >> > >>>>>> <jols...@confluent.io.invalid> wrote: >> > > > > > >> > >>>>>>> The feature command includes the upgrade or >> downgrade >> > > > > command >> > > > > > >> > >> along >> > > > > > >> > >>>>> with >> > > > > > >> > >>>>>>> the --release-version flag. If some features are >> not >> > > > moving >> > > > > in >> > > > > > >> > >> the >> > > > > > >> > >>>>>>> direction mentioned (upgrade or downgrade) the >> command >> > > > will >> > > > > > fail >> > > > > > >> > >> -- >> > > > > > >> > >>>>>> perhaps >> > > > > > >> > >>>>>>> with an error of which features were going in the >> > wrong >> > > > > > >> > >> direction. >> > > > > > >> > >>>>>> >> > > > > > >> > >>>>>> How about updating the KIP to show and document this >> > > > > behavior? >> > > > > > >> > >>>>>> >> > > > > > >> > >>>>>> Thanks, >> > > > > > >> > >>>>>> -- >> > > > > > >> > >>>>>> -José >> > > > > > >> > >>>>>> >> > > > > > >> > >>>>> >> > > > > > >> > >>>> >> > > > > > >> > >>> >> > > > > > >> > >> >> > > > > > >> > >> > > > > > >> > >> > > > > > >> >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> >
