[
https://issues.apache.org/jira/browse/YARN-3659?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16060350#comment-16060350
]
Subru Krishnan edited comment on YARN-3659 at 6/23/17 6:05 PM:
---------------------------------------------------------------
Thanks [~giovanni.fumarola] for working on this. I looked at your patch, please
find my comments below:
* A general note on Javadocs - missing code/link annotations throughout.
* Can you add some documentation for ROUTER_CLIENTRM_SUBMIT_RETRY. You will
also need to exclude it in {{TestYarnConfigurationFields}}.
* Is it possible to move the check of subcluster list being empty to
{{AbstractRouterPolicy}} to prevent duplication?
* A note on *isRunning* field in {{MockResourceManagerFacade}} will be useful.
* Rename NO_SUBCLUSTER_MESSAGE to NO_ACTIVE_SUBCLUSTER_AVAILABLE and update the
value accordingly.
* Why is the visibility of {{RouterUtil}} public? I feel you can also rename it
to RouterServerUtil.
* Resolution of UGI can be moved to {{AbstractClientRequestInterceptor}} as
again all child classes are duplicating the same code.
* Would it be better to use *LRUCacheHashMap* for _clientRMProxies_?
* Rename *getApplicationClientProtocolFromSubClusterId* -->
*getClientRMProxyForSubCluster*.
* Distinguish the log message for different exceptions for clarity.
* Use the {{UniformRandomRouterPolicy}} to determine a random subcluster as
that's the purpose of the policy.
* {code} Client: behavior as YARN. should be Client: identical behavior as
{@code ClientRMService}. {code}
* Documentation for exception handling in *submitApplication* is unclear,
please rephrase it.
* Rename _aclTemp_ to _clientRMProxy_.
* {code}"Unable to create a new application to SubCluster " should be " No
response when attempting to submit the application " + applicationId + "to
SubCluster " + subClusterId.getId() {code}.
* The log statement for app submission can be moved to post successful
submission as it's redundant now.
* We should forward any exceptions we get on *forceKillApplication* and
*getApplicationReport* to client.
* Have a note upfront saying we have only implemented the core functionalities
and rest will be done in a follow up JIRA (create and link).
* Use {{FederationStateStoreTestUtil}} for helper methods like
*registerSubCluster/deRegisterSubCluster/registerSubClusters*.
* In {{TestFederationClientInterceptorRetry}}, assert that
*getApplicationHomeSubCluster* is the good subCluster.
was (Author: subru):
Thanks [~giovanni.fumarola] for working on this. I looked at your patch, please
find my comments below:
* A general note on Javadocs - missing code/link annotations throughout.
* Can you add some documentation for ROUTER_CLIENTRM_SUBMIT_RETRY. You will
also need to exclude it in {{TestYarnConfigurationFields}}.
* Is it possible to move the check of subcluster list being empty to
{{AbstractRouterPolicy}} to prevent duplication?
* A note on *isRunning* field in {{MockResourceManagerFacade}} will be useful.
* Rename NO_SUBCLUSTER_MESSAGE to NO_ACTIVE_SUBCLUSTER_AVAILABLE and update the
value accordingly.
* Why is the visibility of {{RouterUtil}} public? I feel you can also rename it
to RouterServerUtil.
* Resolution of UGI can be moved to {{AbstractClientRequestInterceptor}} as
again all child classes are duplicating the same code.
* Would it be better to use *LRUCacheHashMap* for _clientRMProxies_?
* Rename *getApplicationClientProtocolFromSubClusterId* -->
*getClientRMProxyForSubCluster*.
* Distinguish the log message for different exceptions for clarity.
* Use the {{UniformRandomRouterPolicy}} to determine a random subcluster as
that's the purpose of the policy.
* {code} Client: behavior as YARN. should be Client: identical behavior as
{@code ClientRMService}. {code}
* Documentation for exception handling in *submitApplication* is unclear,
please rephrase it.
* Rename _aclTemp_ to _clientRMProxy_.
* {code}"Unable to create a new application to SubCluster " should be " No
response when attempting to submit the application " + applicationId + "to
SubCluster " + subClusterId.getId() {code}.
* The log statement for app submission can be moved to post successful
submission as it's redundant now.
* We should forward any exceptions we get on *forceKillApplication* and
*getApplicationReport* to client.
* Have a note upfront saying we have only implemented the core functionalities
and rest will be done in a follow up JIRA (create and link).
> Federation Router (hiding multiple RMs for ApplicationClientProtocol)
> ---------------------------------------------------------------------
>
> Key: YARN-3659
> URL: https://issues.apache.org/jira/browse/YARN-3659
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: client, resourcemanager
> Reporter: Giovanni Matteo Fumarola
> Assignee: Giovanni Matteo Fumarola
> Attachments: YARN-3659.pdf, YARN-3659-YARN-2915.1.patch,
> YARN-3659-YARN-2915.draft.patch
>
>
> This JIRA tracks the design/implementation of the layer for routing
> ApplicaitonClientProtocol 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]
