[
https://issues.apache.org/jira/browse/YARN-5412?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16081358#comment-16081358
]
Carlo Curino commented on YARN-5412:
------------------------------------
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).
BUG?
# {{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
YARN-6792
# {{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.
NITS
# {{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?
QUESTIONS:
# 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
> Create a proxy chain for ResourceManager REST API in the Router
> ---------------------------------------------------------------
>
> Key: YARN-5412
> URL: https://issues.apache.org/jira/browse/YARN-5412
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Reporter: Subru Krishnan
> Assignee: Giovanni Matteo Fumarola
> Attachments: YARN-5412-YARN-2915.1.patch,
> YARN-5412-YARN-2915.proto.patch
>
>
> As detailed in the proposal in the umbrella JIRA, we are introducing a new
> component that routes client request to appropriate ResourceManager(s). This
> JIRA tracks the creation of a proxy for ResourceManager REST API in the
> Router. This provides a placeholder for:
> 1) throttling mis-behaving clients (YARN-1546)
> 3) mask the access to multiple RMs (YARN-3659)
> We are planning to follow the interceptor pattern like we did in YARN-2884 to
> generalize the approach and have only dynamically coupling for Federation.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
