YARN-4212. FairScheduler: Parent queues is not allowed to be 'Fair' policy if its children have the "drf" policy. #181

flyrain · 2017-01-19T20:20:08Z

No description provided.

kambatla · 2017-01-24T18:59:47Z

...src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueueManager.java

Should we drop this line altogether?

kambatla · 2017-01-24T20:15:21Z

...src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueueManager.java

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?

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.

kambatla · 2017-01-24T20:19:43Z

...src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueueManager.java

Initialize should likely go to setPolicy

My first thought of this one is similar to the depth checking, planned to refactor it in next JIRA. Another question in my mind is - do we need to initialize policy every time setting the policy?

kambatla · 2017-01-24T20:19:57Z

...src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueueManager.java

setPolicy should likely be called in the constructor.

kambatla · 2017-01-24T20:21:02Z

...main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java

It makes sense for this method to be boolean. I am not sure we should be throwing an exception here. The caller can decide that based on the return value.

kambatla · 2017-01-24T20:21:49Z

...a/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/policies/FairSharePolicy.java

Based on my earlier comment, this should be return ! (childPolicy instanceof DominantResourceFairnessPolicy

kambatla · 2017-01-24T20:22:23Z

...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/policies/FifoPolicy.java

return false

kambatla · 2017-01-24T20:23:38Z

...rc/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSParentQueue.java

Can this be verifyAndSetPolicyFromConf, and part of FSQueue instead of FSParentQueue.

In that case, we will not need a separate call to setPolicy.

…olicy if its children have the "drf" policy.

flyrain · 2017-01-30T09:29:37Z

Thanks Karthik for the review, a new patch posted for your comments.

kambatla

Nice work!

Since the subsequent changes are squashed into one commit, I ended up doing the review from scratch. Most comments are minor.

kambatla · 2017-02-07T23:02:42Z

...a/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/policies/FairSharePolicy.java

 import org.apache.hadoop.classification.InterfaceStability.Unstable;
 import org.apache.hadoop.yarn.api.records.Resource;
 import org.apache.hadoop.yarn.server.resourcemanager.resource.ResourceType;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.AllocationConfigurationException;


Unused import.

kambatla · 2017-02-07T23:04:02Z

...main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java

      Resource queueUsage, Resource maxAvailable);

+  /**
+   * Check whether the policy of a child queue are allowed.


kambatla · 2017-02-07T23:04:29Z

...main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java

+   *
+   * @param childPolicy the policy of child queue
+   */
+  public boolean isChildPolicyAllowed(SchedulingPolicy childPolicy) {


Like that we are adding a non-abstract method.

kambatla · 2017-02-07T23:06:17Z

...a/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/policies/FairSharePolicy.java

-    return SchedulingPolicy.DEPTH_ANY;
+  public boolean isChildPolicyAllowed(SchedulingPolicy childPolicy) {
+    if (childPolicy instanceof DominantResourceFairnessPolicy) {
+      LOG.info("Queue policies can't be " + DominantResourceFairnessPolicy.NAME


s/policies/policy

kambatla · 2017-02-07T23:07:19Z

...a/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/policies/FairSharePolicy.java

+    if (childPolicy instanceof DominantResourceFairnessPolicy) {
+      LOG.info("Queue policies can't be " + DominantResourceFairnessPolicy.NAME
+          + " if the parent policy is " + getName() + ". Please choose "
+          + "other polices for child queues instead.");


IMO, we should either (1) not say anything about other policies or (2) list the policies that are allowed.

kambatla · 2017-02-07T23:09:56Z

...ager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueue.java

  /**
   * Initialize a queue by setting its queue-specific properties and its
-   * metrics.
+   * metrics. This function don't set the policy for queues since there is


s/function/method - there is one other instance of this in the javadoc

s/don't/does not

Instead of saying there is different logic, can we call out what method does that for easier code navigability? And, it might be worth mentioning why that logic is separated, either here or at the other method.

kambatla · 2017-02-07T23:11:34Z

...ager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueue.java

+   * @return true if no policy violation and successfully set polices
+   *         for queues; false otherwise
+   */
+  public boolean verifyAndSetPolicyFromConf(AllocationConfiguration queueConf) {


It might be worthwhile to point out the intended caller for this method.

kambatla · 2017-02-07T23:15:09Z

...est/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/TestFairScheduler.java

  }

+  @Test
+  public void testSchedulingPolicyViolation() throws IOException {


TestFairScheduler is awfully long. Can we please add these methods elsewhere? TestSchedulingPolicy and TestQueueManager are potential candidates.

kambatla · 2017-02-07T23:15:58Z

.../java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/TestSchedulingPolicy.java

-   * 
-   * @throws AllocationConfigurationException
-   */
-  @Test(timeout = 1000)


Are all the cases in this test covered by other tests added here? If not, can we keep the test, maybe rename it, and capture the cases that are not covered?

Add a new test case to check if fifo policy is only for leaf queues.

…olicy if its children have the "drf" policy.

kambatla

Let us fix the checkstyle issues along with the one nit.

kambatla · 2017-02-13T01:39:40Z

...ager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueue.java

-   * different logic to do that.
-   * This function is invoked when a new queue is created or reloading the
-   * allocation configuration.
+   * metrics. This method is invoked when a new queue is created or reloading


Minor language comment. Let us use one of the following two:

when creating a new queue or reloading the allocation file.

when a new is created or the allocation file is reloaded.

…olicy if its children have the "drf" policy.

flyrain · 2017-04-04T16:45:17Z

Committed.

…arrierForVersionUpgrade The code changed significantly. I've rerun the test multiple times both with gradle and intelij. It passed every time. I suggest we enable it back. Author: Boris Shkolnik <boryas@apache.org> Reviewers: Xinyu Liu <xinyu@apache.org> Closes apache#181 from sborya/testZkBarrierForVersionUpgrade

kambatla reviewed Jan 24, 2017

View reviewed changes

YARN-4212. FairScheduler: Parent queues is not allowed to be 'Fair' p…

c79c034

…olicy if its children have the "drf" policy.

flyrain force-pushed the yarn-4212 branch from 5eac6a6 to c79c034 Compare January 30, 2017 09:22

kambatla reviewed Feb 7, 2017

View reviewed changes

YARN-4212. FairScheduler: Parent queues is not allowed to be 'Fair' p…

cc69ee0

…olicy if its children have the "drf" policy.

kambatla suggested changes Feb 13, 2017

View reviewed changes

YARN-4212. FairScheduler: Parent queues is not allowed to be 'Fair' p…

ab28244

…olicy if its children have the "drf" policy.

flyrain force-pushed the yarn-4212 branch from 49bb09b to ab28244 Compare February 13, 2017 18:42

flyrain closed this Apr 4, 2017

YARN-4212. FairScheduler: Parent queues is not allowed to be 'Fair' policy if its children have the "drf" policy. #181

YARN-4212. FairScheduler: Parent queues is not allowed to be 'Fair' policy if its children have the "drf" policy. #181

Uh oh!

Conversation

flyrain commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain commented Jan 30, 2017

Uh oh!

kambatla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kambatla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants