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