Hi Jun, > 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.
Yes. This is correct. The idea is to replace the MV with this new flag. It will have the same semantics but with the benefit of being independent. > 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. This makes sense. We can definitely use version 0. Best, David On Tue, Apr 2, 2024 at 1:43 AM 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é > > > >> > >>>>>> > > > >> > >>>>> > > > >> > >>>> > > > >> > >>> > > > >> > >> > > > >> > > > > >> > > > > >> > > > > > > > > > >
