hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Subru Krishnan (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-5634) Simplify initialization/use of RouterPolicy via a RouterPolicyFacade
Date Thu, 10 Nov 2016 03:31:58 GMT

    [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message