I've updated the KIP to include this CLI. For now, I've moved the new api to the server as a rejected alternative.
It seems like we can keep the mapping in the tool. Given that we also need to do the same validation for using multiple --feature flags (the request will look the same to the server), we can have the --release-version flag. I think that closes the main discussions. Please let me know if there is any further discussion I missed. Justine On Mon, Mar 25, 2024 at 4:44 PM Justine Olshan <jols...@confluent.io> wrote: > Hi Jose, > > Sorry for the typos. I think you figured out what I meant. > > I can make a new API. There is a risk of creating a ton of very similar > APIs though. Even the ApiVersions api is confusing with its supported and > finalized features fields. I wonder if there is a middle ground here. > I can have the storage tool and features tool rely on the feature and not > query the server. Colin seemed to be against that. > > Anyway your idea of putting the info on the server side is probably for > the best. > > --release-version can work with the downgrade tool too. I just didn't > think I needed to directly spell that out. I can though. > > I wish we weren't splitting this conversation among the two threads. :( I > tried to get this out so it could cover all KIPs. Having this on separate > threads makes getting this consensus even harder than it already is. > From what I can tell your KIP's text matches this. Is the expectation that > the added flags will be done as part of this KIP or your KIP? I don't > really have a strong opinion about --release-version so maybe it should > have been part of your KIP all along. > > > To me version 0 doesn't necessarily mean that the feature is not > enabled. For example, for kraft.version, version 0 means the protocol > prior to KIP-853. Version 0 is the currently implemented version of > the KRaft protocol. > > This is what Colin told me in our previous discussions. I don't really > feel too strongly about the semantics here. > > So it seems like the only real undecided item here is whether we should > have this new api query the server or rely on the information being built > in the tool. > I will update the KIP to include the CLI command to get the information. > > Justine > > On Mon, Mar 25, 2024 at 4:19 PM José Armando García Sancio > <jsan...@confluent.io.invalid> wrote: > >> Hi Justine, >> >> Thanks for the update. See my comments below. >> >> On Mon, Mar 25, 2024 at 2:51 PM Justine Olshan >> <jols...@confluent.io.invalid> wrote: >> > I've updated the KIP with the changes I mentioned earlier. I have not >> yet >> > removed the --feature-version flag from the upgrade tool. >> >> What's the "--feature-version" flag? This is the first time I see it >> mentioned and I don't see it in the KIP. Did you mean >> "--release-version"? >> >> > Please take a look at the API to get the versions for a given >> > metadata version. Right now I'm using ApiVersions request and >> specifying a >> > metadata version. The supported versions are then supplied in the >> > ApiVersions response. >> > There were tradeoffs with this approach. It is a pretty minimal change, >> but >> > reusing the API means that it could be confusing (ie, the ApiKeys will >> be >> > supplied in the response but not needed.) I considered making a whole >> new >> > API, but that didn't seem necessary for this use. >> >> I agree that this is extremely confusing and we shouldn't overload the >> ApiVersions RPC to return this information. The KIP doesn't mention >> how it is going to use this API. Do you need to update the Admin >> client to include this information? >> >> Having said this, as you mentioned in the KIP the kafka-storage tool >> needs this information and that tool cannot assume that there is a >> running server it can query (send an RPC). Can the kafka-features use >> the same mechanism used by kafka-storage without calling into a >> broker? >> >> re: "It will work just like the storage tool and upgrade all the >> features to a version" >> >> Does this mean that --release-version cannot be used with >> "kafka-features downgrade"? >> >> re: Consistency with KIP-853 >> >> Jun and I have been having a similar conversation in the discussion >> thread for KIP-853. From what I can tell both changes are compatible. >> Do you mind taking a look at these two sections and confirming that >> they don't contradict your KIP? >> 1. >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-storage >> 2. >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-853%3A+KRaft+Controller+Membership+Changes#KIP853:KRaftControllerMembershipChanges-kafka-features >> >> re: nit "For MVs that existed before these features, we map the new >> features to version 0 (no feature enabled)." >> >> To me version 0 doesn't necessarily mean that the feature is not >> enabled. For example, for kraft.version, version 0 means the protocol >> prior to KIP-853. Version 0 is the currently implemented version of >> the KRaft protocol. >> >> Thanks, >> -- >> -José >> >
