[
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: [email protected]
For additional commands, e-mail: [email protected]
