[
https://issues.apache.org/jira/browse/YARN-5634?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15652899#comment-15652899
]
Subru Krishnan commented on YARN-5634:
--------------------------------------
Thanks [~curino] for the patch. I looked at it, please find my comments below.
General note:
* It looks like you have the intellij formatting set rather than the Hadoop
standard. For e.g. : Unnecessary change to imports in
{{FederationStateStoreFacade}}. I feel this is the root cause for the
checkstyle warnings too.
{{YarnConfiguration}}
* For every new config, you need to add corresponding default in
_yarn-default.xml_ or exclusions in {{TestYarnConfigurationFields}}. I prefer
the latter for these configs.
* I don't think we need DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS (can we
avoid FEDERATION_POLICY_MANAGER_PARAMS itself) in the interest of trying to
limit the _configuration_ explosion.
{{RouterPolicyFacade}}
* {code} String defaultPolicyParamString =
conf.get(YarnConfiguration.DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS,
YarnConfiguration.DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS); {code} should be
{code} String defaultPolicyParamString =
conf.get(YarnConfiguration.FEDERATION_POLICY_MANAGER_PARAMS,
YarnConfiguration.DEFAULT_FEDERATION_POLICY_MANAGER_PARAMS); {code}
* Please use *Charset.defaultCharset()* instead of hard-coding (also existing
usages like in {{WeightedPolicyInfo}}.
* I feel it's better if we add the the _fallbackPolicyManager_ to the cache
during initialization so that we can avoid costly
*fallbackPolicyManager::getRouterPolicy* in every invocation of
*getHomeSubcluster*.
* We should have a warning log with context that we were not able to retrieve
the policy configuration for the input queue.
* The _if_ clause can be rephrased as {code} cachedConfs.containsKey(queue)
&& !cachedConfs.get(queue).equals(configuration) {code}.
{{FederationStateStoreFacade}}
* We should specify for what _queue_, we got null from StateStore and
possibly log it.
{{TestFederationPolicyFacade}}
* Overall the coverage is nice! The only feedback is it took me quite some
time to figure out how the configuration was getting updated in
*testConfigurationUpdate*. IIUC, it's done implicitly through
{{FederationPoliciesTestUtil::initFacade}}? If so, can we do it explicitly as
that will help considerably in readability and moreover none of the other tests
use it and might even be a undesirable side-effect.
* Nit: can we rename *testgetHomeSubcluster* to *testGetHomeSubcluster* and
move it up to the first test.
> Simplify initialization/use of RouterPolicy via a RouterPolicyFacade
> ---------------------------------------------------------------------
>
> Key: YARN-5634
> URL: https://issues.apache.org/jira/browse/YARN-5634
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Affects Versions: YARN-2915
> Reporter: Carlo Curino
> Assignee: Carlo Curino
> Labels: oct16-medium
> Attachments: YARN-5634-YARN-2915.01.patch,
> YARN-5634-YARN-2915.02.patch
>
>
> The current set of policies require some machinery to (re)initialize based on
> changes in the SubClusterPolicyConfiguration. This JIRA tracks the effort to
> hide much of that behind a simple RouterPolicyFacade, making lifecycle and
> usage of the policies easier to consumers.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
