[
https://issues.apache.org/jira/browse/YARN-8967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16791411#comment-16791411
]
Yufei Gu commented on YARN-8967:
--------------------------------
Hi [~wilfreds], thanks for the patch. Some comments:
1. Nice cleanup in class QueueManager.
2. Should we deprecate two constructors of class AllocationFileLoaderService
rather than remove them since it is a public class?
3. "{{public List<PlacementRule> getRules()}}" here, Modifier “public” could be
“no modifier” since all test cases invoking method getRules() are in the same
package.
4. I would suggest to put the exception into the LOG.warn and remove LOG.debug
in method "placeApplication()".
5. The method "addApplication()" is a little bit messy due to it holds both
logic for adding a new application and recovered application. I feel like it
would be cleaner if we separate the method addApplication to two methods, one
for add new application anther for recover applications. Just some thoughts.
What do you think?
6. Since we’ve got this, we doesn’t need to check whether queueName is null in
method addApplication().
{code:java}
if (queueName != null) {
addApplication(appAddedEvent.getApplicationId(),
queueName, appAddedEvent.getUser(),
appAddedEvent.getIsAppRecovering(),
appAddedEvent.getPlacementContext());
}
{code}
7. Do we still need this check “if (queueName.startsWith(".") ||
queueName.endsWith(“.”))”? We’ve normalized queue names in placement rule for a
new application and the queue name should be valid for a recovered app.
Class {{QueuePlacementPolicy}} related comments:
1. The QueuePlacementPolicy objects in class AllocationConfiguration are never
used by production code if we {{updateRules()}} in the constructor. I would
suggest either moving {{updateRules()}} out of the QueuePlacementPolicy
constructor or removing all QueuePlacementPolicy objects and making
QueuePlacementPolicy a utility class. I prefer the first one since it reduces
coupling. In that case, the AllocationConfiguration object still keeps all
configurations items including placement rules, which is a consistent behavior.
2. You probably need an another comment style to make this link work {{{@link
#getTerminal}}}
3. Incomplete comment {{// The list must be}} in class QueuePlacementPolicy
4. Typos in comment “Builds an QueuePlacementPolicy from an xml element.” an
-> a
5. “testNoCreate()” contains some duplicated test cases. I’m OK if you delete
it or not since it isn’t introduced by your patch.
6. I would suggest to refactor the method “fromXml()” a little bit by
introducing a new method like “getParentRule()”
7. We could create a nested class like the following in class
QueuePlacementPolicy to avoid multiple “get(0)” and “get(1)” in the code.
{code:java}
public static class Policy {
public Object clazzz;
public String terminal;
}
{code}
8. Find the following code in class SpecifiedPlacementRule, no need to both log
error and throw an exception. Would you mind fix it in this patch although it
isn’t introduced by this patch?
{code:java}
LOG.error("Specified queue name not valid: '{}'", queueName);
throw new YarnException("Application submitted by user " + user +
"with illegal queue name '" + queueName + "'.");
{code}
> 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
>
>
> 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]
