Hi, Justine, Thanks for the updated KIP.
One more question related to KIP-1014. It introduced a new config unstable.metadata.versions.enable. Does each new feature need to have a corresponding config to enable the testing of unstable features or should we have a generic config enabling the testing of all unstable features? Jun On Thu, Apr 4, 2024 at 8:24 PM Justine Olshan <jols...@confluent.io.invalid> wrote: > 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é > >> > >> > > > > > >> > >>>>>> > >> > >> > > > > > >> > >>>>> > >> > >> > > > > > >> > >>>> > >> > >> > > > > > >> > >>> > >> > >> > > > > > >> > >> > >> > >> > > > > > >> > > >> > >> > > > > > >> > > >> > >> > > > > > >> > >> > >> > > > > > > > >> > >> > > > > > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > > >> > > >> > > >
