Thanks for the reply Justine. See my comments below: On Thu, Feb 29, 2024 at 3:39 PM Justine Olshan <jols...@confluent.io.invalid> wrote: > 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.
--feature in kafka-features allows for multiple features to be specified. Please see the implementation of the tool and UpdateFeatures RPC. For example, you can execute this command kafka-features.sh upgrade --feature metadata.version=15 --feature kraft.version=1. You should be able to support the same UI in kafka-storage.sh. It is what KIP-853 suggests in the interest of making it consistent across CLI. > 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. I was thinking of making them mutually exclusive in both cases. Much easier to explain and document. If the user wants to use --feature in kafka-storage, they need to specify all of the supported features. For the "kafka-feature upgrade" case, they can enumerate all of the --feature versions that they want to upgrade in one command. > 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. If you are not going to deprecate or alias --metadata what happens if the user uses these flags in one command: "kafka-features upgrade --metadata 3.8 --feature kraft.version=1" One of the problems I am having is that I can't seem to find the KIP that introduces --metadata so I can only speculate as to the intent of this flag from the CLI help and implementation. Do you know which KIP added that flag? > 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? "kafka-features.sh upgrade" supports multiple --feature flags. Please see the tools implementation and the UpdateFeatures RPC. You can specify multiple features in the storage tool with: "kafka-storage format --feature kraft.version=1 --feature metadata.version=15". The command line parser used by both tools support flags that append values to a list. This is how the kafka-features CLI works already. > 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 Are you going to generate the documentation of these dependencies automatically from the released code like we do for Kafka configuration properties? Or do Kafka developers need to remember to update the documentation with these dependencies? > 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. "kafka-features upgrade" should only upgrade and "kafka-features downgrade" command should only downgrade. It should be an error if any of the UpdateFeatures requests violates this. What we need to do is define if an error in one feature results in an error for the entire request. The UpdateFeatures RPC seems to allow individual errors but that gets confusing and difficult to enforce once you introduce dependencies between feature versions. Thanks! -- -José
