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é >
