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é > > > > > >> > >>>>>> > > > > > >> > >>>>> > > > > > >> > >>>> > > > > > >> > >>> > > > > > >> > >> > > > > > >> > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > >
