[GitHub] [ozone] fapifta commented on pull request #3104: HDDS-6213. Pluggable OzoneManager request handling hooks

[GitBox <gi...@apache.org>]

fapifta commented on pull request #3104:
URL: https://github.com/apache/ozone/pull/3104#issuecomment-1047315023


   Thank you @kerneltime for your notes, I have added answers for you.
   
   In the meantime, I was thinking about how to answer to @errose28 beyond the simple yes, to give some more context I have realized a few questions, for which I am trying to come up with good answers and reasoning, but still I feel we should go over them one by one, and discuss it once more. I am wondering what would be a good platform for that discussion.
   
   The questions:
   1. The condition NEW_CLIENT_REQUESTS, is something which is a bit futuristic, as how on earth an old server will new about a new client, and I was wondering if ever this will be relevant...
   2. I am still wondering if an unconditional validation will ever be a way we want to go under, as that has consequences on how we organize request handling code...
   3.  Ordering of the validators is again a good question, will there be ever an ordering required (if we use unconditionally hooked in methods, then certainly it will be required...)
   4. The current structure is also a good question, now we can define a validator in terms of the validation condition, the request type, and by the hook... Request type is tied to the protocol version, and it is really not easy now to add the same validation to two requests.
   
   To give some short answers:
   1. this we can use in rare cases, for example when an incompatible change went through, then this could be an easy way to provide a fix for those whom are having trouble because of that change within an already upgraded cluster and an old one in case those need to communicate from the newer code side. The cost of this is negligible when we look at maintenance or request processing performance.
   2. unconditionals can be used to move out authorization and auditing for example, but if we go down this route then naming of the classes and methods needs to change, as these are not strictly validations of request features...
   3. ordering is easy to add, and inevitable I believe if we implement those unconditionals mentioned in 2.
   4. is a good enforcer of a - I think - better design, as it forces an implementor to implement logic for one request only, and helps us avoid speculative generalism, with that switch-cases and ifs inside a validator method (how much of this is whisful thinking?), also gives a good rationale to put validators into the request processing classes. (Note: read requests' processing have to be moved out to their own classes for this though)
   
   
   The idea @kerneltime came up with to perhaps move some generic request processing part into validator methods, made me also think further...
   If we go with unconditionals and use them for such things like authorization of the request, then we probably should rename a few things:
   RequestFeatureValidator -> RequestPreProcessor and RequestPostProcessor (with that we can remove the RequestProcessingPhase enum)
   ValidatorRegistry -> RequestHookRegistry (or something like that)
   RequestValidations -> RequestHooks (or something like that)
   
   The idea to use this with other protocols than just the ozone client protocol, also brings us to some further changes that are necessary to extend the whole API:
   - move the whole thing to hdds-common
   - we might move the javac annotation processor to hdds/annotations
   - we need to generalize the request type in the annotations (also it would still be nice to validate that somehow, and that is hard to solve due to unknown dependencies, unless we have a generic annotation processor and specific annotation processors defined alongside the protocol implementation we want to hook into)
   - we still need one wrapper around the registry per protocol, as integration with the protocol might varies for different protocols, and request parameters might be different, this has to be examined
   - we have to decide where this would be useful, and we need to introduce client version into the protocols where it is missing.
   - probably more, I still just experimenting with this in my head... also I have no clue if there would be a necessary incompatible change down this route...
   
   Q1 also leads to one more thing, for which I did not do any research, so I just put it here as something we might examine later on: newer client requests condition can be something used on the client side, if there is such an easy way to hook things into the client side of the request, as in that case the client can use this condition to alter requests when talking to older servers... again... this came up just now in my mind, and I am absolutely unsure if we can easily hook on client side, and whether it makes any sense to do this there.
   (Note to self: the idea feels like if I found a golden hammer and I started to see nails everywhere, so please argue with the usefuleness of this...)
   
   Last but not least, there was a main concern from @kerneltime, how to ensure these methods will not linger somewhere in the code, and how we can be sure which methods were running during pre/post process phase for a given query. Also how we can make this easy for devs to realize that there are these validations.
   One answer is code organization (validators along with request handlers), and possibly an enforcement of that, an other is logging, these concerns are still not addressed in the code.
   


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org