Hi Jun and Justine, My understanding is that in the KIP, --metadata does not do the same thing as --release-version. The --metadata flag sets the metadata version only, and does not change any other feature. So neither flag replaces the other.
In general, I'm not sure that this functionality belongs in the features command. It will have a lot of confusing interactions with the other flags. If we do want it in the features command it should be its own subcommand, not upgrade or downgrade. Because setting all the features could involve upgrading some, but downgrading others. So really what we're doing is neither an upgrade nor a downgrade, but a sync to a release version. This also raises some questions about the RPC. If we want to use the existing features RPC, we cannot perform a sync that does both upgrades and downgrades in a single RPC. So we'd either need to be non-atomic, or just refuse to do it in that case. Or add a new server-side RPC for this. Perhaps it would be more useful to just have a command that tells people what the levels of each feature would be for a given release version? Then the user could choose which ones to upgrade / downgrade (if any) and do them carefully one at a time. The shotgun approach, where you upgrade everything at once, actually isn't what I think most users want to do in production. best, Colin On Fri, Mar 1, 2024, at 13:25, Jun Rao wrote: > Hi, Justine, > > Thanks for the reply. > > 11. Yes, we can still have the option to set individual features. I was > suggesting that if one uses the tool without either --release-version > or --features, it will just use the latest version of every feature that it > knows. This is what's proposed in the original KIP-584. > > 12. If we can't deprecate --metadata METADATA, it would be useful to > describe the reason. > > Jun > > 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é >> > >>
