Hi Jun, I apologize for typos. I thought I got rid of all the non protocol versions. It is protocol version as per my previous email. I will fix the KIP.
The group coordinator version is used to upgrade to the new group coordinator protocol. (KIP-848) I don't have all the context there. I would prefer to not add the metadata flag to the storage tool as it is not necessary. The reason it is not removed is purely for backwards compatibility. Colin had strong feelings about not removing any flags. Justine On Mon, Mar 25, 2024 at 5:01 PM Jun Rao <j...@confluent.io.invalid> wrote: > Hi, Justine, > > Thanks for the updated KIP. A few more comments. > > 10. Could we describe what RPCs group.coordinator.version controls? > > 12. --metadata METADATA is not removed from kafka-features. Do we have a > justification for this? If so, should we add that to kafka-storage to be > consistent? > > 14. The KIP has both transaction.protocol.version vs transaction.version. > What's the correct feature name? > > Jun > > On Mon, Mar 25, 2024 at 4:54 PM Justine Olshan > <jols...@confluent.io.invalid> > wrote: > > > 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é > > >> > > > > > >
