Hi Justine, Thanks for driving this forward. Comments below.
On Wed, Mar 13, 2024, at 10:51, Justine Olshan wrote: > Hey folks -- let me summarize my understanding > > Artem requested we change the name of transaction version (tv) to > transaction protocol version (tpv). If I do that, I will also probably > change to group coordinator protocol version (gcpv). > > > Folks were concerned about upgrades/downgrades with different feature > versions when using the feature tool and --release version. I think we can > address some concerns here: > > - don't allow a command that moves a version in the wrong direction > (when upgrade actually downgrades a feature for example) > - allow a command to describe all the feature versions for a given > metadata version > > Note that Colin expressed some concern with having the --release-version > flag at all. I wonder if we can reach a compromise with having the upgrade > command have a "latest" command only. > > > Jun proposed this "latest" functionality can be the default when no version > or feature is specified. > > Jose requested that we make --release-version and --feature mutually > exclusive for both the storage tool and the features tool. We should also > specify each feature in the storage tool as a separate flag. > > Finally, Jun and Jose requested deprecation of the --metadata flag, but > Colin felt we should keep it. I mentioned the --feature flag does the same > but no reply yet. > > * So here are the updates I propose:* > 1. Update the feature names as Artem described -- transaction protocol > version and group coordinator protocol version > > 2. Rename --features in the storage tool to --feature. In the case where we > use this flag, we must repeat the flag for the number of features to set > all of them. > +1 > 3. No argument behavior on the storage and upgrade tools is to set all the > features to the latest (stable) version > +1 > 4. Include an API to list the features for a given metadata version > At the risk of expanding the scope... if we have an API to list feature levels, we should have a brief description of what they do. Something like: 13 3.6-IV1 add metadata transactions Maybe a simple way to do this would just be to output the full list and let people grep for what they want. You could also add filters like --release-version, etc. The big question is would you store this data on the servers or in the tool? Right now the tool is a bit "smart" in the sense that it can translate between an MV string and an MV level. Arguably it should be dumber, though, and rely on the server for these things. Anyway your idea of putting the info on the server side is probably for the best. > 5. I'm going back and forth on whether we should support the > --release-version flag still. If we do, we need to include validation so we > only upgrade on upgrade. I'd rather not, but I'm curious what others think. I really feel like the use-case is not there (compared with the more manual, but safer process described above) best, Colin > > Let me know if folks have any issues with the updates I proposed or think > there is something I missed. > > Justine > > On Fri, Mar 8, 2024 at 11:59 AM Artem Livshits > <alivsh...@confluent.io.invalid> wrote: > >> Hi Justine, >> >> > Are you suggesting it should be called "transaction protocol version" or >> "TPV"? I don't mind that, but just wanted to clarify if we want to include >> protocol or if simply "transaction version" is enough. >> >> My understanding is that "metadata version" is the version of metadata >> records, which is fairly straightforward. "Transaction version" may be >> ambiguous. >> >> -Artem >> >> On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan >> <jols...@confluent.io.invalid> >> wrote: >> >> > Hey folks, >> > >> > Thanks for the discussion. Let me try to cover everyone's comments. >> > >> > Artem -- >> > I can add the examples you mentioned. As for naming, right now the >> feature >> > is called "transaction version" or "TV". Are you suggesting it should be >> > called "transaction protocol version" or "TPV"? I don't mind that, but >> just >> > wanted to clarify if we want to include protocol or if simply >> "transaction >> > version" is enough. >> > >> > Jun -- >> > >> > 10. *With **more features, would each of those be controlled by a >> separate >> > feature or* >> > >> > *multiple features. For example, is the new transaction record format* >> > >> > *controlled only by MV with TV having a dependency on MV or is it >> > controlled* >> > >> > *by both MV and TV.* >> > >> > >> > I think this will need to be decided on a case by case basis. There >> should >> > be a mechanism to set dependencies among features. >> > For transaction version specifically, I have no metadata version >> > dependencies besides requiring 3.3 to write the feature records and use >> the >> > feature tools. I would suspect all new features would have this >> > requirement. >> > >> > >> > 11. *Basically, if **--release-version is not used, the command will just >> > use the latest* >> > >> > *production version of every feature. Should we apply that logic to both* >> > >> > *tools?* >> > >> > >> > How would this work with the upgrade tool? I think we want a way to set a >> > new feature version for one feature and not touch any of the others. >> > >> > >> > *12. Should we remove --metadata METADATA from kafka-features? It does >> the* >> > >> > *same thing as --release-version.* >> > >> > >> > When I previously discussed with Colin McCabe offline about this tool, he >> > was strongly against deprecation or changing flags. I personally think it >> > could good >> > >> > to unify and not support a ton of flags, but I would want to make sure he >> > is aligned. >> > >> > >> > *13. KIP-853 also extends the tools to support a new feature >> > kraft.version.* >> > >> > *It would be useful to have alignment between that KIP and this one.* >> > >> > >> > Sounds good. Looks like Jose is in on the discussion so we can continue >> > here. :) >> > >> > >> > >> > Jose -- >> > >> > >> > *1. KIP-853 uses --feature for kafka-storage instead of --features.* >> > >> > *This is consistent with the use of --feature in the "kafka-feature.sh* >> > >> > *upgrade" command.* >> > >> > >> > I wanted to include multiple features in one command, so it seems like >> > features is a better name. I discuss more below about why I think we >> should >> > allow setting multiple features at once. >> > >> > >> > *2. I find it a bit inconsistent that --feature and --release-version* >> > >> > *are mutually exclusive in the kafka-feature CLI but not in the* >> > >> > *kafka-storage CLI. What is the reason for this decision?* >> > >> > >> > For the storage tool, we are setting all the features for the cluster. By >> > default, all are set. For the upgrade tool, the default is to set one >> > feature. In the storage tool, it is natural for the --release-version to >> > set the remaining features that --features didn't cover since otherwise >> we >> > would need to set them all >> > >> > If we use the flag. In the feature upgrade case, it is less necessary for >> > all the features to be set at once and the tool can be run multiple >> times. >> > I'm not opposed to allowing both flags, but it is less necessary in my >> > opinion. >> > >> > >> > *3. KIP-853 deprecates --metadata in the kafka-features and makes it an* >> > >> > *alias for --release-version. In KIP-1022, what happens if the user* >> > >> > *specified both --metadata and --feature?* >> > >> > >> > See my note above (Jun's comment 12) about deprecating old commands. I >> > would think as the KIP stands now, we would not accept both commands. >> > >> > >> > *4. I would suggest keeping this* >> > >> > *consistent with kafka-features. It would avoid having to implement one* >> > >> > *more parser in Kafka.* >> > >> > >> > I sort of already implemented it as such, so I don't think it is too >> > tricky. I'm not sure of an alternative. Kafka features currently only >> > supports one feature at a time. >> > I would like to support more than one for the storage tool. Do you have >> > another suggestion for multiple features in the storage tool? >> > >> > >> > *5. As currently described, trial and error seem to be the* >> > >> > *only mechanism. Should the Kafka documentation describe these* >> > >> > *dependencies? Is that good enough?* >> > >> > >> > The plan so far is documentation. The idea is that this is an advanced >> > feature, so I think it is reasonable to ask folks use documentation >> > >> > >> > *6. Did you mean that 3.8-IV4 would map to TV2? If* >> > >> > *not, 3.8-IV3 would map to two different TV values.* >> > >> > >> > It was a typo. Each MV maps to a single other feature version. >> > >> > >> > *7. For --release-version in "kafka-features upgrade", does* >> > >> > *--release-version mean that all of the feature versions will either* >> > >> > *upgrade or stay the same? Meaning that downgrades will be rejected by* >> > >> > *the Kafka controller. How about when --release-version is used with* >> > >> > *"kafka-features downgrade"?* >> > >> > >> > The idea I had was that the cluster will check if all the versions are >> > supported. If any version is not supported the tool will throw an error. >> We >> > can also issue a warning if the given command (if supported by the >> cluster) >> > will downgrade a feature. One option is also to require a yes/no prompt >> or >> > flag to allow downgrades. The downgrade tool would allow the same. >> > Alternatively we could just fail the command if a feature is downgraded >> on >> > upgrade command or vice versa. I don't have a strong preference. >> > >> > >> > Thanks again for the discussion, >> > Justine >> > >> > On Thu, Feb 29, 2024 at 10:44 AM José Armando García Sancio >> > <jsan...@confluent.io.invalid> wrote: >> > >> > > Hi Justine and Jun, >> > > >> > > Thanks for the KIP Justine. See my comments below. >> > > >> > > On Wed, Feb 28, 2024 at 3:09 PM Jun Rao <j...@confluent.io.invalid> >> > wrote: >> > > > 13. KIP-853 also extends the tools to support a new feature >> > > kraft.version. >> > > > It would be useful to have alignment between that KIP and this one. >> > > >> > > I agree. I took a look at this KIP and these are the differences that >> > > I can spot. >> > > >> > > 1. KIP-853 uses --feature for kafka-storage instead of --features. >> > > This is consistent with the use of --feature in the "kafka-feature.sh >> > > upgrade" command. >> > > >> > > 2. I find it a bit inconsistent that --feature and --release-version >> > > are mutually exclusive in the kafka-feature CLI but not in the >> > > kafka-storage CLI. What is the reason for this decision? >> > > >> > > 3. KIP-853 deprecates --metadata in the kafka-features and makes it an >> > > alias for --release-version. In KIP-1022, what happens if the user >> > > specified both --metadata and --feature? >> > > >> > > 4. There is an example: "kafka-storage format --features >> > > metadata.version=16,transaction.version=2,group.coordinator.version=1". >> > > This is different from the --feature flag in kafka-features which is >> > > an append flag. Meaning that multiple features are specified as >> > > "--feature metadata.version=16 --feature transaction.version=2 >> > > --feature group.coordinator.version=1". I would suggest keeping this >> > > consistent with kafka-features. It would avoid having to implement one >> > > more parser in Kafka. >> > > >> > > 5. In the section "A Note on Feature Interdependence", you mention "an >> > > error should be thrown if trying to format with X version 13 and Y >> > > version 12". How would the user discover (or describe) these >> > > dependencies? As currently described, trial and error seem to be the >> > > only mechanism. Should the Kafka documentation describe these >> > > dependencies? Is that good enough? >> > > >> > > 6. In "3.8-IV2 that could map to TV1, 3.8-IV3 could also be TV1, and >> > > 3.8-IV3 could be TV2" did you mean that 3.8-IV4 would map to TV2? If >> > > not, 3.8-IV3 would map to two different TV values. >> > > >> > > 7. For --release-version in "kafka-features upgrade", does >> > > --release-version mean that all of the feature versions will either >> > > upgrade or stay the same? Meaning that downgrades will be rejected by >> > > the Kafka controller. How about when --release-version is used with >> > > "kafka-features downgrade"? >> > > >> > > -José >> > > >> > >>
