fapifta commented on PR #6919: URL: https://github.com/apache/ozone/pull/6919#issuecomment-2224248023
Two additional thoughts for a proper fix: - we need to change the check in the KeyValueHandler and introduce a RequestFeatureValidator annotated method for that with the proper version check, the method should do the BlockUtils.verifyReplicaIdx() call, and error out from there in case the ClientVersion is at least EC_REPLICA_INDEX_REQUIRED_IN_BLOCK_REQUEST instead of having the check within the request processing logic - If we choose to implement the second suggestion of mine, and include the version in the mapping inside ValidatorRegistry, that means we need to add a 4th parameter to the annotation with the client's version, and also we need to think about the associated annotation preprocessor and figure out if we need verification for the version defined in the annotation. On the other hand, thinking further about this option, we need to consider how it will affect further developments of the framework, if we add the version to the annotation , as we also had plans to introduce this framework to internal protocol calls where the version may not be a ClientVersion, but it can also be a DatanodeVersion, a StorageContainerManagerVersion (which we did not define yet) or an OzoneManagerVersion. So this makes me think that it would be better to have the client's version be checked within the annotated validator methods and not within the framework itself, but still I am open to explore this idea further. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
