hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yufei Gu (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-8967) Change FairScheduler to use PlacementRule interface
Date Wed, 20 Mar 2019 06:41:00 GMT

    [ https://issues.apache.org/jira/browse/YARN-8967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16796846#comment-16796846
] 

Yufei Gu commented on YARN-8967:
--------------------------------

Hi [~wilfreds], the patch v9 looks really good. 
{quote}
Based on all this I do think I need to file a follow up jira to fix the Hive SHIM that uses
the policy at the moment and move that to the new code in a backward compatible way.
{quote}
I am with you.

Some nits:
1. Sorry to miss this in last review, there is no need to add a debug log since we throw an
exception here.
{code}
LOG.debug("Initialising rule set failed", ioe);
throw new AllocationConfigurationException(
    "Rule initialisation failed with exception", ioe);
{code}
3. Too many nested if/for statements in the method fromXml(). It would be nice to exact some
logic in the loop to a separated method or we can use the {{if (! node instanceof Element)
continue;}} to avoid one layer.
4. I made up a new test case, the “nestedUserQueue” has 2 parents, only the second one
takes effect. I believe we should at least LOG a warn for the first parent “primaryGroup”
and we don’t need to create and initialize it since it will be overwritten by the second
parent. 
{code}
StringBuffer sb = new StringBuffer();
sb.append("<queuePlacementPolicy>");
sb.append("  <rule name='specified' />");
sb.append("  <rule name='nestedUserQueue'>");
sb.append("       <rule name='primaryGroup'/>");
sb.append("       <rule name='default'/>");
sb.append("  </rule>");
sb.append("</queuePlacementPolicy>");

createPolicy(sb.toString());
{code}
5. Not a fan of the getters in the nested class RuleMap. It could be as simple as possible
as a wrapper class for multiple values, just like the case class in Scala or data class in
Kotlin. This is just my preference. I’m OK with current implement though.



> Change FairScheduler to use PlacementRule interface
> ---------------------------------------------------
>
>                 Key: YARN-8967
>                 URL: https://issues.apache.org/jira/browse/YARN-8967
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: capacityscheduler, fairscheduler
>            Reporter: Wilfred Spiegelenburg
>            Assignee: Wilfred Spiegelenburg
>            Priority: Major
>         Attachments: YARN-8967.001.patch, YARN-8967.002.patch, YARN-8967.003.patch, YARN-8967.004.patch,
YARN-8967.005.patch, YARN-8967.006.patch, YARN-8967.007.patch, YARN-8967.008.patch, YARN-8967.009.patch
>
>
> The PlacementRule interface was introduced to be used by all schedulers as per YARN-3635.
The CapacityScheduler is using it but the FairScheduler is not and is using its own rule definition.
> YARN-8948 cleans up the implementation and removes the CS references which should allow
this change to go through.
> This would be the first step in using one placement rule engine for both schedulers.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
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