I took a quick look at the code -- looks like the previous behavior was not to set a top level error if one particular feature had an issue. We can do that.
I think it could make some sense to specify errors on features that were not valid and use the top level error to indicate that the request didn't update any features. The handling now is to complete the futures with the top level error anyway. As for the validation criteria. It seems like one bit of code that validates whether a version is allowed is in the method `reasonNotSupported` which checks the range of features available for the given feature. For metadata.version we have a method to do "additional checks" and we could have those for the various other features as well. I have an (internal) FeatureVersion interface in mind that would work well for this. For any of these validations, we return the same error `INVALID_UPDATE_VERSION`. I would think continuing to use this error follows naturally, but if we think it is necessary to specify the error code, I can do so in my KIP. Justine On Tue, Apr 9, 2024 at 1:46 PM Justine Olshan <jols...@confluent.io> wrote: > José, > > INVALID_UPDATE_VERSION was added as part of KIP-497. The KIP seems to be > lacking some details on the error. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-497%3A+Add+inter-broker+API+to+alter+ISR > > https://github.com/apache/kafka/commit/57de67db22eb373f92ec5dd449d317ed2bc8b8d1 > > The error seems to be used in the feature update path as well, though that > was also not included in KIP-584. I wonder if we were missing necessary > details for many KIPs in 2020... > > I'm not sure I fully understand the proposal. Is the question for the > exact error to use in UpdatableFeatureResult.ErrorCode and what to write > in UpdatableFeatureResult.ErrorMessage? If so, those errors and adding a > message (the dependency that was violated for example) makes sense. > I agree that it makes sense that any errors in updates should be a top > level error and not have a partial update. > > I thought these were part of KIP-584, but I will take a look and update > this KIP if they are not. > > Justine > > On Tue, Apr 9, 2024 at 1:10 PM José Armando García Sancio > <jsan...@confluent.io.invalid> wrote: > >> Hi Justine, >> >> Thanks for the KIP. I see that the KIP doesn't make any updates to the >> UpdateFeatures RPC. I was trying to understand how errors will be >> communicated to the client. >> >> Are you planning to use the INVALID_UPDATE_VERSION error and overwrite >> the ErrorMessage field for all of the validations you mentioned in the >> KIP? I see that INVALID_UPDATE_VERSION is in the code for Apache Kafka >> but I couldn't find the KIP that adds this error. It is not in KIP-584 >> or KIP-778. If you agree, do you think we should document this error >> in this KIP? >> >> It is also not clear to me when the UpdateFeaturesResponse will return >> an error per feature versus an error for the entire RPC. KIP-584 >> defines this relationship but it doesn't specify when exactly a top >> level error will be returned versus when a feature level error will be >> returned. I think that most users wouldn't want partial failures. They >> instead would like to be guaranteed that all of the feature updates >> succeeded or none did. Do you agree? Should we update the KIP to make >> this clear? >> >> Thanks! >> -- >> -José >> >
