swamirishi commented on PR #6932: URL: https://github.com/apache/ozone/pull/6932#issuecomment-2226446655
> Thanks for working on this @swamirishi. While this PR is focused on the issues with the `OLDER_CLIENT_REQUEST` condition, the `CLUSTER_NEEDS_FINALIZATION` condition has the same problem. I suggest this: > > * Adding two new keys in the annotation: one for client version and one for layout version. They are exclusive. Only one is allowed to be specified per annotation. > * Removing the general `ValidationCondition` from the annotation. > * Two 3-nested maps in the registry: one for layout feature validators and one for client version validators since the version enums have different types. > > Currently there is some [iteration](https://github.com/apache/ozone/blob/b77d1bf8cca1b1e16a369c6ea6c4c114edb5d26b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestValidations.java#L108) required for every request, even if they don't have a validator. With the general concept of validation conditions removed we can do a simple map lookup which will be more efficient in the common case, especially if we order the keys accordingly. > > In this layout I think `request version -> request type -> request phase -> validator` is the most efficient layout of the map. Most clients will probably be newer versions and can fail out of the check in the first level. After that, most requests will not have compat concerns for a given version, so even more requests exit early. The phase is hardcoded depending on which method is called so there is no option to exit early here, hence it should be last. > > This is less general than the validation condition support we have now, but IMO the general implementation is causing code complexity and inefficiencies on the write path for something we do not use and may never need. This also makes it consistent with other upgrade related annotations like `DisallowedUntilLayoutVersion` which only take a version and act on it with a fixed condition. > Thanks for working on this @swamirishi. While this PR is focused on the issues with the `OLDER_CLIENT_REQUEST` condition, the `CLUSTER_NEEDS_FINALIZATION` condition has the same problem. I suggest this: > > * Adding two new keys in the annotation: one for client version and one for layout version. They are exclusive. Only one is allowed to be specified per annotation. > * Removing the general `ValidationCondition` from the annotation. > * Two 3-nested maps in the registry: one for layout feature validators and one for client version validators since the version enums have different types. > > Currently there is some [iteration](https://github.com/apache/ozone/blob/b77d1bf8cca1b1e16a369c6ea6c4c114edb5d26b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestValidations.java#L108) required for every request, even if they don't have a validator. With the general concept of validation conditions removed we can do a simple map lookup which will be more efficient in the common case, especially if we order the keys accordingly. > > In this layout I think `request version -> request type -> request phase -> validator` is the most efficient layout of the map. Most clients will probably be newer versions and can fail out of the check in the first level. After that, most requests will not have compat concerns for a given version, so even more requests exit early. The phase is hardcoded depending on which method is called so there is no option to exit early here, hence it should be last. Version shouldn't be part of the map key? We need the validator running for all clients older than a certain condition. I believe we need the same for Layout version. E.g. We know if a server hasn't currently finalized ERASURE_CODED_STORAGE_SUPPORT then we can be sure that it hasn't started preparing for HSYNC as well. Thus if a client comes in and tries to use HSYNC the validator should block that request as well. > > This is less general than the validation condition support we have now, but IMO the general implementation is causing code complexity and inefficiencies on the write path for something we do not use and may never need. This also makes it consistent with other upgrade related annotations like `DisallowedUntilLayoutVersion` which only take a version and act on it with a fixed condition. -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
