Hi, Justine, Thanks for the reply.
10. If we are exposing group.coordinator.version in this KIP, I think we need to document what RPCs/records it controls or if it has dependencies on MV. 15. "For transaction version specifically, I have no metadata version dependencies besides requiring 3.3 to write the feature records and use the feature tools. I would suspect all new features would have this requirement." Could we document the TV dependency on MV and that the tools/RPC will fail if the provided TV and MV version are not compatible? Hi, Colin, The functionality of --metadata METADATA flag in kafka-features can be achieved by the new --feature flag. Is there a need to still keep the --metadata flag? Thanks, Jun On Mon, Mar 25, 2024 at 5:09 PM Justine Olshan <jols...@confluent.io.invalid> wrote: > 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é > > > >> > > > > > > > > > >
