adoroszlai commented on PR #6919: URL: https://github.com/apache/ozone/pull/6919#issuecomment-2220979370
Thanks @errose28 for the explanations. With the refactor (which I agree should be done in follow-up), condition will be moved from the validator method to the annotation. Until then, I would suggest removing the version condition from usages of the annotation as part of this fix. Consider introducing a new version `V` along with some validation method. `CURRENT_VERSION` right after such change is `V`. The method performs some logic for client versions `<V`. Due to the annotation's condition `OLDER_CLIENT_REQUEST`, the method is not called for any code path using version `V`. Thus the logic in the method is validated only by explicit compatibility tests. The current problem (forgetting to add any version check inside the method) is caught only when version `V+1` is introduced. The annotation is great for handling conditions unrelated to the validator's logic, like "not yet finalized" state. As soon as the framework can handle client versions (without the 2-enum problem you mentioned), validators will no longer have to check client version and thus the "unrelated condition" statement will still be true. -- 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]
