Hi, Justine, Thanks for the KIP. A few comments below.
10. Currently, MV controls records, inter-broker RPCs and code logic. 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. 11. "If not all features are covered with this flag, the command will just use the latest production version of the feature like it does for metadata version." Should we apply that logic to --release-version too? 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? 12. Should we remove --metadata METADATA from kafka-features? It does the same thing as --release-version. 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. Jun On Wed, Feb 28, 2024 at 10:49 AM Artem Livshits <alivsh...@confluent.io.invalid> wrote: > Hi Justine, > > Thank you for the KIP. I think the KIP is pretty clear and makes sense to > me. Maybe it would be good to give a little more detail on the > implementation of feature mapping and how the tool would validate the > feature combinations. For example, I'd expect that > > bin/kafka-storage.sh format --release-version 3.6-IVI --feature > transaction.version=2 > > would give an error because the new transaction protocol is not supported > in 3.6. Also, we may decide that > > bin/kafka-storage.sh format --release-version 5.0-IV0 --feature > transaction.version=0 > > would be an unsupported combination as it'll have been a while since the > new transaction protocol has been the default and it would be too risky to > enable this combination as it may not be tested any more. > > As for the new names, I'm thinking of the "transaction feature version" > more like a "transaction protocol version" -- from the user perspective we > don't really add new functionality in KIP-890, we're changing the protocol > to be more robust (and potentially faster). > > -Artem > > > > On Wed, Feb 28, 2024 at 10:08 AM Justine Olshan > <jols...@confluent.io.invalid> wrote: > > > Hey Andrew, > > > > Thanks for taking a look. > > > > I previously didn't include 1. We do plan to use these features > immediately > > for KIP-890 and KIP-848. If we think it is easier to put the discussion > in > > those KIP discussions we can, but I fear that it will easily get lost > given > > the size of the KIPs. > > > > I named the features similar to how we named metadata version. > Transaction > > version would control transaction features like enabling a new > transaction > > record format and APIs to enable KIP-890 part 2. Likewise, the group > > coordinator version would also enable the new record formats there and > the > > new group coordinator. I am open to new names or further discussion. > > > > For 2 and 3, I can provide example scripts that show the usage. I am open > > to adding --latest-stable as well. > > > > Justine > > > > On Tue, Feb 27, 2024 at 4:59 AM Andrew Schofield < > > andrew_schofield_j...@outlook.com> wrote: > > > > > Hi Justine, > > > Thanks for the KIP. This area of Kafka is complicated and making it > > easier > > > is good. > > > > > > When I use the `kafka-features.sh` tool to describe the features on my > > > cluster, I find that there’s a > > > single feature called “metadata.version”. I think this KIP does a > handful > > > of things: > > > > > > 1) It introduces the idea of two new features, TV and GCV, without > giving > > > them concrete names or > > > describing their behaviour. > > > 2) It introduces a new flag on the storage tool to enable advanced > users > > > to control individual features > > > when they format storage for a new broker. > > > 3) It introduces a new flag on the features tool to enable a set of > > latest > > > stable features for a given > > > version to be enabled all together. > > > > > > I think that (1) probably shouldn’t be in this KIP unless there are > > > concrete details. Maybe this KIP is enabling > > > the operator experience when we introduce TV and GCV in other KIPs. I > > > don’t believe the plan is to enable > > > the new group coordinator with a feature, and it seems unnecessary to > me. > > > I think it’s more compelling for TV > > > given the changes in transactions. > > > > > > For (2) and (3), it would be helpful to explicit about the syntax for > the > > > enhancements to the tool. I think > > > that for the features tool, `--release-version` is an optional > parameter > > > which requires a RELEASE_VERSION > > > argument. I wonder whether it would be helpful to have > `--latest-stable` > > > as an option too. > > > > > > Thanks, > > > Andrew > > > > > > > On 26 Feb 2024, at 21:26, Justine Olshan > <jols...@confluent.io.INVALID > > > > > > wrote: > > > > > > > > Hello folks, > > > > > > > > I'm proposing a KIP that allows for setting and upgrading new > features > > > > (other than metadata version) via the kafka storage format and > feature > > > > tools. This KIP extends on the feature versioning changes introduced > by > > > > KIP-584 by allowing for the features to be set and upgraded. > > > > > > > > Please take a look: > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1023%3A+Formatting+and+Updating+Features > > > > > > > > Thanks, > > > > > > > > Justine > > > > > > > > >
