Thanks José -- Let me answer a few quick things
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" > I would think as the KIP stands now, we would not accept both commands Metadata flag was added as part of KIP-778. See https://cwiki.apache.org/confluence/display/KAFKA/KIP-778%3A+KRaft+to+KRaft+Upgrades and https://github.com/apache/kafka/commit/28d5a059438634db6fdecdbb816e2584715884d6 I think the usage of --feature multiple times is a little bit clunky, but we can use it for consistency with the existing tool. I was hoping it could be simple to set one feature differently and take the default for everything else. But alas. For the storage tool, if we have to set all the features, do you expect to throw an error when one is missing? This seems acceptable, but just wanted to clarify. For documentation, we can have an autogenerated doc. I haven't thought too much how this would look yet. I am ok with throwing an error if a feature isn't moving in the correct direction (upgrade or downgrade) for the command that is given. I will continue to think about everyone's comments and update the KIP over the next few days. Thanks, Justine On Thu, Feb 29, 2024 at 4:31 PM José Armando García Sancio <jsan...@confluent.io.invalid> wrote: > 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é >
