I'm hoping this covers the majority of comments. I will go ahead and open the vote in the next day or so.
Thanks, Justine On Wed, Apr 3, 2024 at 3:31 PM Justine Olshan <jols...@confluent.io> wrote: > Find and replace has failed me :( > > Group version seems a little vague, but we can update it. Hopefully find > and replace won't fail me again, otherwise I will get another email on this. > > Justine > > On Wed, Apr 3, 2024 at 12:15 PM David Jacot <dja...@confluent.io.invalid> > wrote: > >> Thanks, Justine. >> >> * Should we also use `group.version` (GV) as I suggested in my previous >> message in order to be consistent? >> * Should we add both names to the `Public Interfaces` section? >> * There is still at least one usage of `transaction.protocol.verison` in >> the KIP too. >> >> Best, >> David >> >> On Wed, Apr 3, 2024 at 6:29 PM Justine Olshan >> <jols...@confluent.io.invalid> >> wrote: >> >> > 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é >> > >> > > > > > >> > >>>>>> >> > >> > > > > > >> > >>>>> >> > >> > > > > > >> > >>>> >> > >> > > > > > >> > >>> >> > >> > > > > > >> > >> >> > >> > > > > > >> > >> > >> > > > > > >> > >> > >> > > > > > >> >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > >> >> > > >> > >> >
