Hi Colin, Thanks for bringing up these points. I believe Jose suggested failing the command if some features upgrade on the downgrade call and vice versa. This could address some of the concerns you bring up here. I agree we should not have an rpc to do both.
As for setting metadata only, we do have the option of using the --feature flag to set only metadata version, but that requires writing it by short rather than by metadata string. Not sure if that's what folks had in mind. Including a command to list the features for a given version is pretty useful. I can add that to the kip unless folks have concerns. Justine On Thu, Mar 7, 2024 at 1:28 PM Colin McCabe <cmcc...@apache.org> wrote: > 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é > >> > > >> >
