Carlo Curino commented on YARN-6896:

Thaks [~giovanni.fumarola] for the contribution. Yetus seems mostly happy, 
please fix the checkstyles that can be fixed easily (one line length and the 
unused imports). Below few comments on the implementation:

# {{FederationInterceptorREST}} would we ever care to set the REST retries 
differenly from ROUTER_CLIENTRM_SUBMIT_RETRY ? If not reusing the conf is good, 
otherwise is limiting.
# {{FederationInterceptorREST.getInterceptorForSubCluster}} has slightly weird 
get-or-set semantics. Can we break it down to a pure getter and pure setter, or 
at least rename it?
# {{FederationInterceptorREST}} line 176, you are editing the collection 
returned by the federationFacade. Is this kosher? I.e., do you know that is a 
local copy of it? Why not using the same "blacklisting" style you have in other 
# {{FederationInterceptorREST}} line 205, in the comment should we inline the 
behavior (beside stating is the same-as)? if the text is not too long it might 
be worth it.
# {{FederationInterceptorREST.submitApplication}} I would expect if we run-out 
of tries for us to remove the attempted submission from stateStore, while you 
never perform a removeApplicationHomeSubcluster, so the last attempted 
submission would be still stored in the statestore upon running out of retries. 
Is that true?
# {{FederationInterceptorREST.getApp/updateAppState/}} does this work fine in 
case the String appId in input is malformed? For submitApplication you make an 
explicit check. 
# What is the criteria for picking the subset of methods you are implementing? 
Also many of the non-implemented methods seem trivial forward-to-homesubcluster 
or broadcast type behaviors. 

# {{TestFederationInterceptorREST }} constructing a random list of sub-clusters 
seems a very common problem in Tests... can you check if we can reuse this code 
from other classes (and/or move it to a util test class).
# {{TestFederationInterceptorREST.createConfiguration}} do you need 2 mocks?
# {{TestableFederationInterceptorREST.getInterceptorForSubCluster}} this 
completely mask the underlying function, which is rather important to maintain 
the DefaultRequestInterceptorREST map. It would be better to find another 
workaround that exercise the original code, and not mock it away.

> Federation: routing REST invocations transparently to multiple RMs
> ------------------------------------------------------------------
>                 Key: YARN-6896
>                 URL: https://issues.apache.org/jira/browse/YARN-6896
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Giovanni Matteo Fumarola
>            Assignee: Giovanni Matteo Fumarola
>         Attachments: YARN-6896.proto.patch, YARN-6896.v1.patch
> This JIRA tracks the design/implementation of the layer for routing 
> RMWebServicesProtocol requests to the appropriate RM(s) in a federated YARN 
> cluster.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to