[
https://issues.apache.org/jira/browse/YARN-6896?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16119263#comment-16119263
]
Carlo Curino edited comment on YARN-6896 at 8/9/17 12:57 AM:
-------------------------------------------------------------
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
methods?
# {{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.
was (Author: curino):
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
methods?
# {{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
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
