Giovanni, thanks for the contribution. Please find below a partial review (patch is large going through it slowly, and posting comments
so you can parallelize addressing them).
- DefaultRequestInterceptorREST.init does not propagate correctly to downstream interceptors... you might want to have a "localInit" method that is implemented to locally initialize the components, while the AbstractRESTRequestInterceptor takes care of propagating to downstream, by invoking the outward init
MUST HAVE CLEANUPS
- In the next version of patch, please remove the changes you already pushed in
- DefaultRequestInterceptorREST there is lots of repeting code doing the "check status" stuff, which could be solved with a single method that receives in input a string-constant and the .class of the expected outcome.
- RESTRequestInterceptor there are a few (uncommented) methods, which I am not sure belong here. Shall we move them to RMWebServiceProtocol? Help me understand why e.g., getContainer should be here.
- yarn-default.xml the comment is not very clear about how the list of classes make up the pipeline.
- AbstractRESTRequestInterceptor javadoc errors "can can"
- (RMWSConsts.STATES is a confusing name, something like CONTAINER_STATES?
- RESTRequestInterceptor the javadoc of setNextInterceptor is confusing. It says the last interceptor does not receive this call, while it will likely receive it with a null value, correct?
GOOD TO HAVE
- RMWebAppUtil.isStaticUser is invoked a lot in RMWebServices, and every time it performs a read from conf. Can we cache the value of staticUser, so this method gets much lighter to invoke?
- DefaultRequestInterceptorREST Can we try to use generics to remove lots of the almost-code-repetition we currently have?
- can we scope-down to tests the import of nodemanager project?
- why does RESTRequestInterceptor.init has a "user" param? Javadoc is incomplete, and it is not used by DefaultRequestInterceptorREST so hard to understand.
[...] more to come (I will re-post the entire list adding to it, so numbering remains consistent... we can
strike-out elements as you do them