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