-
Notifications
You must be signed in to change notification settings - Fork 9.2k
YARN-4212. FairScheduler: Parent queues is not allowed to be 'Fair' policy if its children have the "drf" policy. #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,17 +35,18 @@ | |
| import org.apache.hadoop.classification.InterfaceStability.Unstable; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.yarn.conf.YarnConfiguration; | ||
| import org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.policies.FifoPolicy; | ||
| import org.xml.sax.SAXException; | ||
|
|
||
| import com.google.common.base.CharMatcher; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
| import java.util.Iterator; | ||
| import java.util.Set; | ||
| import org.apache.hadoop.yarn.api.records.Resource; | ||
|
|
||
| /** | ||
| * Maintains a list of queues as well as scheduling parameters for each queue, | ||
| * such as guaranteed share allocations, from the fair scheduler config file. | ||
| * | ||
| */ | ||
| @Private | ||
| @Unstable | ||
|
|
@@ -72,6 +73,9 @@ public FSParentQueue getRootQueue() { | |
|
|
||
| public void initialize(Configuration conf) throws IOException, | ||
| SAXException, AllocationConfigurationException, ParserConfigurationException { | ||
| // Policies of root and default queue are set to | ||
| // SchedulingPolicy.DEFAULT_POLICY since the allocation file hasn't been | ||
| // loaded yet. | ||
| rootQueue = new FSParentQueue("root", scheduler, null); | ||
| queues.put(rootQueue.getName(), rootQueue); | ||
|
|
||
|
|
@@ -80,7 +84,7 @@ public void initialize(Configuration conf) throws IOException, | |
| // Recursively reinitialize to propagate queue properties | ||
| rootQueue.reinit(true); | ||
| } | ||
|
|
||
| /** | ||
| * Get a leaf queue by name, creating it if the create param is true and is necessary. | ||
| * If the queue is not or can not be a leaf queue, i.e. it already exists as a | ||
|
|
@@ -272,12 +276,25 @@ private FSQueue createNewQueues(FSQueueType queueType, | |
| FSParentQueue newParent = null; | ||
| String queueName = i.next(); | ||
|
|
||
| // Check if child policy is allowed | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this check be in setPolicy or the FSQueue constructor instead? For instance, FSLeafQueue#setPolicy already checks if the level is appropriate. This brings up another point - do we need this check of parent-child policies AND the depth? Should we get rid of depth either in this JIRA or a follow-up?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, my original thought is to do that in another JIRA. The depth and parent-child policy are not the same. It mighty a good idea to combine them since the logic of depth checking only prevent fifo policy to be non-leaf queue. The current implementation seems a bit heavy. I can do it in this JIRA. |
||
| SchedulingPolicy childPolicy = scheduler.getAllocationConfiguration(). | ||
| getSchedulingPolicy(queueName); | ||
| if (!parent.getPolicy().isChildPolicyAllowed(childPolicy)) { | ||
| LOG.error("Can't create queue '" + queueName + "'."); | ||
| return null; | ||
| } | ||
|
|
||
| // Only create a leaf queue at the very end | ||
| if (!i.hasNext() && (queueType != FSQueueType.PARENT)) { | ||
| FSLeafQueue leafQueue = new FSLeafQueue(queueName, scheduler, parent); | ||
| leafQueues.add(leafQueue); | ||
| queue = leafQueue; | ||
| } else { | ||
| if (childPolicy instanceof FifoPolicy) { | ||
| LOG.error("Can't create queue '" + queueName + "', since " | ||
| + FifoPolicy.NAME + " is only for leaf queues."); | ||
| return null; | ||
| } | ||
| newParent = new FSParentQueue(queueName, scheduler, parent); | ||
| queue = newParent; | ||
| } | ||
|
|
@@ -479,6 +496,13 @@ private String ensureRootPrefix(String name) { | |
| public void updateAllocationConfiguration(AllocationConfiguration queueConf) { | ||
| // Create leaf queues and the parent queues in a leaf's ancestry if they do not exist | ||
| synchronized (queues) { | ||
| // Verify and set scheduling policies for existing queues before creating | ||
| // any queue, since we need parent policies to determine if we can create | ||
| // its children. | ||
| if (!rootQueue.verifyAndSetPolicyFromConf(queueConf)) { | ||
| LOG.error("Setting scheduling policies for existing queues failed!"); | ||
| } | ||
|
|
||
| for (String name : queueConf.getConfiguredQueues().get( | ||
| FSQueueType.LEAF)) { | ||
| if (removeEmptyIncompatibleQueues(name, FSQueueType.LEAF)) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worthwhile to point out the intended caller for this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.