Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: scheduler
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Create the initial framework required for using OrderingPolicies and an initial FifoOrderingPolicy

      1. YARN-3318.13.patch
        46 kB
        Craig Welch
      2. YARN-3318.14.patch
        46 kB
        Craig Welch
      3. YARN-3318.17.patch
        54 kB
        Craig Welch
      4. YARN-3318.34.patch
        67 kB
        Craig Welch
      5. YARN-3318.35.patch
        68 kB
        Craig Welch
      6. YARN-3318.36.patch
        71 kB
        Craig Welch
      7. YARN-3318.39.patch
        73 kB
        Craig Welch
      8. YARN-3318.45.patch
        28 kB
        Craig Welch
      9. YARN-3318.47.patch
        31 kB
        Craig Welch
      10. YARN-3318.48.patch
        31 kB
        Craig Welch
      11. YARN-3318.52.patch
        16 kB
        Craig Welch
      12. YARN-3318.53.patch
        21 kB
        Craig Welch
      13. YARN-3318.56.patch
        26 kB
        Craig Welch
      14. YARN-3318.57.patch
        23 kB
        Craig Welch
      15. YARN-3318.58.patch
        25 kB
        Craig Welch
      16. YARN-3318.59.patch
        26 kB
        Craig Welch
      17. YARN-3318.60.patch
        26 kB
        Craig Welch
      18. YARN-3318.61.patch
        26 kB
        Craig Welch

        Issue Links

          Activity

          Hide
          cwelch Craig Welch added a comment -

          Proposed elements of the framework:

          A SchedulerProcess interface which generalizes processes to be managed by the OrderingPolicy (initially, potentially in the future by other Policies as well) Initial implementer will be the SchedulerApplicaitonAttempt.

          An OrderingPolicy interface which exposes a collection of scheduler processes which will be ordered by the policy for container assignment and preemption. The ordering policy will provide one Iterator which presents processes in the policy specific order for container assignment and another Iterator which presents them in the proper order for preemption. It will also accept SchedulerProcessEvents which may indicate a need to re-order the associated SchedulerProcess (for example, after container completion, preemption, assignment, etc)

          Show
          cwelch Craig Welch added a comment - Proposed elements of the framework: A SchedulerProcess interface which generalizes processes to be managed by the OrderingPolicy (initially, potentially in the future by other Policies as well) Initial implementer will be the SchedulerApplicaitonAttempt. An OrderingPolicy interface which exposes a collection of scheduler processes which will be ordered by the policy for container assignment and preemption. The ordering policy will provide one Iterator which presents processes in the policy specific order for container assignment and another Iterator which presents them in the proper order for preemption. It will also accept SchedulerProcessEvents which may indicate a need to re-order the associated SchedulerProcess (for example, after container completion, preemption, assignment, etc)
          Hide
          cwelch Craig Welch added a comment -

          The proposed initial implementation of the framework to support FIFO SchedulerApplicationAttempt ordering for the CapacityScheduler:

          A SchedulerComparatorPolicy which implements OrderingPolicy above. This implementation will take care of the common logic required for cases where the policy can be effectively implemented as a comparator (which is expected to be the case for several potential policies, including FIFO).

          A SchedulerComparator which is used by the SchedulerComparatorPolicy above. This is an extension of the java Comparator interface with additional logic required by the SchedulerComparatorPolicy, initially a method to accept SchedulerProcessEvents and indicate whether the require re-ordering of the associated SchedulerProcess.

          Show
          cwelch Craig Welch added a comment - The proposed initial implementation of the framework to support FIFO SchedulerApplicationAttempt ordering for the CapacityScheduler: A SchedulerComparatorPolicy which implements OrderingPolicy above. This implementation will take care of the common logic required for cases where the policy can be effectively implemented as a comparator (which is expected to be the case for several potential policies, including FIFO). A SchedulerComparator which is used by the SchedulerComparatorPolicy above. This is an extension of the java Comparator interface with additional logic required by the SchedulerComparatorPolicy, initially a method to accept SchedulerProcessEvents and indicate whether the require re-ordering of the associated SchedulerProcess.
          Hide
          cwelch Craig Welch added a comment -


          Initial, incomplete patch with the overall framework & implementation of the SchedulerComparatorPolicy and FifoComparator, major TODO includes integrating with capacity scheduler configuration. Also includes a CompoundComparator for chaining comparator based policies where desired.

          Show
          cwelch Craig Welch added a comment - Initial, incomplete patch with the overall framework & implementation of the SchedulerComparatorPolicy and FifoComparator, major TODO includes integrating with capacity scheduler configuration. Also includes a CompoundComparator for chaining comparator based policies where desired.
          Hide
          cwelch Craig Welch added a comment -

          Same as .13 except it should be possible to apply with YARN-3319 's .14 patch

          Show
          cwelch Craig Welch added a comment - Same as .13 except it should be possible to apply with YARN-3319 's .14 patch
          Hide
          cwelch Craig Welch added a comment -

          With support for configuration via the scheduler's config file

          Show
          cwelch Craig Welch added a comment - With support for configuration via the scheduler's config file
          Hide
          leftnoteasy Wangda Tan added a comment -

          Hi Craig,
          Thanks for working on the patch, I just took a look, some overall comments for interface definitions:
          1) SchedulerProcess
          1.1. It's name seems not very straightforward to me, many processes can run for an application. How about call it SchedulableEntity (to not confuse with fair scheduler's Schedulable)?

          1.2. I found SchedulerProcessEvent defined in SchedulerProcess, is there any specific reason to make SchedulerProcess to be asynchronized? As far as I can imagine, fair/capacity(fifo)/priority, all of them need synchronized call (or synchronized calls are enough). The reason is, if you don't want to write a brand-new scheduler, now all schedulers assume scheduler event need handle under scheduler lock.

          1.3. SerialEpoch and Serial not straight forward to me also, how about call them clusterTimeStamp and submissionId?

          1.4. currentConsumption is not enough to make choice, demand(pending-resource)/used and priority/weight are basic fields of a Schedulable, do you think so? I suggest you can take a look at ResourceUsage class, now it becomes field of CSQueue/LeafQueue.User/SchedulerApplicationAttempt already, it should have enough resource-related fields to make scheduling decisions. And it considered node-label as well.

          2) OrderingPolicy
          2.1 addSchedulerProcess/removeSchedulerProcess/addAllSchedulerProcesses should become interface of Orderingpolicy. Now doing getSchedulerProcess().add(..) assumes internal implementation is treeSet. It's better not depend on internal Collection<> behavior.

          2.2 handleSchedulerProcessEvent makes the OrderingPolicy mixed synchronized/asynchronized writer interface, I suggest to make it to be, containerAllocated/Preempted(SchedulerProcess, RMContainer)

          2.3 Rename it to be SchedulerProcessOrderingPolicy?

          3) SchedulerComparatorPolicy
          I understand this class is to make different comparators can share same policy implementation, and admins can combine different comparators via configuration. But it seems a little hard to use for me.

          For example, now admin need configure following options to enable fair schedling per queue:

          policy=SchedulerComparatorPolicy
          comparator=CompoundComparator, and its comparators are {fair and fifo}
          

          Instead, I think we need hide internal implementation of policy/comparator, we can predefine policy=fair means using CompoundComparator (fair+fifo) and policy = capacity means using fifo.

          We can still leave admin flexibility to configure their custom policy/comparator, but we need reduce configuration complexity for most common usecases.

          Will include review for tests once we have consent on interfaces.

          Show
          leftnoteasy Wangda Tan added a comment - Hi Craig, Thanks for working on the patch, I just took a look, some overall comments for interface definitions: 1) SchedulerProcess 1.1. It's name seems not very straightforward to me, many processes can run for an application. How about call it SchedulableEntity (to not confuse with fair scheduler's Schedulable)? 1.2. I found SchedulerProcessEvent defined in SchedulerProcess, is there any specific reason to make SchedulerProcess to be asynchronized? As far as I can imagine, fair/capacity(fifo)/priority, all of them need synchronized call (or synchronized calls are enough). The reason is, if you don't want to write a brand-new scheduler, now all schedulers assume scheduler event need handle under scheduler lock. 1.3. SerialEpoch and Serial not straight forward to me also, how about call them clusterTimeStamp and submissionId? 1.4. currentConsumption is not enough to make choice, demand(pending-resource)/used and priority/weight are basic fields of a Schedulable, do you think so? I suggest you can take a look at ResourceUsage class, now it becomes field of CSQueue/LeafQueue.User/SchedulerApplicationAttempt already, it should have enough resource-related fields to make scheduling decisions. And it considered node-label as well. 2) OrderingPolicy 2.1 addSchedulerProcess/removeSchedulerProcess/addAllSchedulerProcesses should become interface of Orderingpolicy. Now doing getSchedulerProcess().add(..) assumes internal implementation is treeSet. It's better not depend on internal Collection<> behavior. 2.2 handleSchedulerProcessEvent makes the OrderingPolicy mixed synchronized/asynchronized writer interface, I suggest to make it to be, containerAllocated/Preempted(SchedulerProcess, RMContainer) 2.3 Rename it to be SchedulerProcessOrderingPolicy ? 3) SchedulerComparatorPolicy I understand this class is to make different comparators can share same policy implementation, and admins can combine different comparators via configuration. But it seems a little hard to use for me. For example, now admin need configure following options to enable fair schedling per queue: policy=SchedulerComparatorPolicy comparator=CompoundComparator, and its comparators are {fair and fifo} Instead, I think we need hide internal implementation of policy/comparator, we can predefine policy=fair means using CompoundComparator (fair+fifo) and policy = capacity means using fifo. We can still leave admin flexibility to configure their custom policy/comparator, but we need reduce configuration complexity for most common usecases. Will include review for tests once we have consent on interfaces.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12708335/YARN-3318.34.patch
          against trunk revision 85dc3c1.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 8 new or modified test files.

          -1 javac. The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1146 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

          org.apache.hadoop.yarn.client.cli.TestYarnCLI
          org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesCapacitySched

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7163//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7163//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7163//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7163//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12708335/YARN-3318.34.patch against trunk revision 85dc3c1. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified test files. -1 javac . The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1146 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: org.apache.hadoop.yarn.client.cli.TestYarnCLI org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesCapacitySched Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7163//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7163//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7163//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7163//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          Hi Wangda,
          I have changed the patch a bit in the background without updating it on the jira. The changes are not major but I think they render some of the comments obsolete. I've uploaded the up-to-date patch just now, before doing so I took a pass through your comments- below I'll respond to each in turn-

          1) SchedulerProcess

          1.1. ...name seems not very straightforward to me...

          Well, I'm certainly open to other name options, but I do prefer SchedulerProcess to SchedulableEntity - it's common to refer the items which a scheduler will schedule as "Processes", which is what these are in this case, which is why I chose this name. "Entity" is really very generic and empty of meaning. I do wish to avoid confusion with Scheduleable (and wasn't enamored of that name either...), I expect that as integration progresses there will be a period where Schedulable will be an extension of SchedulerProcess with the (remaining) FairScheduler specific bits (which will, I think, ultimately be incorporated in some way into SchedulerProcess, but that's down the line/should be addressed in further iteration). In any case, not in favor of adding "Entity", I think when you consider the terminology as explained above SchedulerProcess works, try it on and see, and feel free to give other options...

          1.2. ...SchedulerProcessEvent...asynchronized

          Not all event handling must be asynchronous. I believe the details regarding this were spelled out reasonably well in the interface definition - if you take a peek at how these events are handled in the capacity scheduler configuration you will see that they are synchronously/safely within protection against mutation of the schedulerprocess collection. My goal was precisely to avoid needing to have implementer add a new method implementation every time a new event comes into play which may not be of interest to it, this makes maintenance of implementations easier - they can manage those which they understand and have appropriate default logic otherwise. I think this is a classic case for an enumerated set of events to be handled by the interface & so I think it should be modeled as it is as opposed to adding a new method for each new event type to the interface itself...

          1.3 ...SerialEpoch

          Yeah, I don't like the names much either, I've gone through several versions and come to the conclusion that it's not the choice of names that's the problem. This is an attempt to "hide" the application id's while also exposing them, which is compound in nature, and is made stranger by the fact that this is totally irrelevant for other potential future implementors (such as queues). I want to factor it differnently not just change the names, these are the courses I'm considering:
          1. Have SchedulerProcess implement "Comparable" and provide a "natural ordering", which is fifo for apps. This seems to "privilege" fifo but, as a matter of fact, it's the fallback for fair so I'm not sure that's really an inappropriate thing to do - it seems like it is the "natural ordering" for apps. Other things can give their own natural ordering (queues - the hierarchy...), so it should extend reasonably well without the current awkwardness. This would remove all of "getSerialEpoch", "getSerial", and "getId" in favor of just implementing "compareTo" from comparable. The downsides I see are the "privilage" and that if an implementor of SchedulerProcess implemented "comparable" in an unworkable fashion it would be an issue, not the case for what we are presently looking at supporting afaik
          2. Have an explicit "compareCreationOrder(SchedulerProcess other)" method which returns 0 + - like compareTo. This is much like 1, but removes the "privilege" and the possible Comparable collision... this also does away getSerialEpoch, getSerial, and getId in favor of the comparison method.
          What do you think of these options? Preference?
          BTW, FS has an actual "startTime" for fsappattempts, but looking through it I don't like that approach - it doesn't appear to do the right thing in some cases (like rm failover or recovery), it still can be ambiguous for some cases (simultanious start w/in tsmillis granularity) where there's a fallback to appid, so it doesn't look to really add anything as you still have to be able to fall back to the app id for those cases so you can't get away from the issue, and it adds a bit of complexity to boot.

          1.4 ...currentConsumption is not enough to make choice, demand(pending-resource)/used and priority/weight are basic fields of a Schedulable, do you think so...

          Of those, only demand is required for the initial step of supporting application level fairness when sizeBasedWeight is active, the others are only relevant for other uses (such as queues) which we're not taking on in the first go. I had already added demand to the interface, it is presently being calculated using pending requests. I looked at switching to using the pending value of the attemptResourceUsage in the SchedulerApplicationAttempt but I don't see that it is being updated anywhere, am I missing it? It seems that it should have been made to do so when it was added to SchedulerApplicationAttempt but that it was possibly missed? In any case, if it is/were it made to be I could use it. Also, looking over ResourceUsage, I think there needs to be a "getCopyPending" or the like, for usecases where you get the value and then start to use it, as the code is today it can be mutated as soon as the value is returned, so while in some sense it's threadsafe, not really in a usable way for many callers who would want a snapshot copy of the final value which won't change (as in this case...)

          2) Ordering Policy

          2.1 addSchedulerProcess/removeSchedulerProcess/addAllSchedulerProcesses ...Collection... treeSet

          I think the current factoring is correct. First, you'll notice that the return type of getSchedulerProcess() is a Collection, in no way is this tied to treeSet, implementers are welcome to use any collection type they like (or implement the minimal collection interface themselves if really necessary...). It is appropriate to have a Collection view on the processes, it is the proper abstraction for it and it's used in various places throughout the scheduler - effectively the ask is to add the collection interface methods to this interface, which doesn't make sense, it's better to use the existing/common interface as I am doing at present

          2.2 ...

          no it doesn't, see 1.2 above

          2.3 rename...SchedulerProcessOrderingPolicy

          I suppose we could, although that assumes we keep SchedulerProcess I thought that an unnecessarily long name & that there would not be a lot of ordering policies to give rise to ambiguity, but assuming we stick with SchedulerProcess, or whatever we do name it, we could expand this out - not sure its really necessary, but not really opposed...

          3) ...configuration...

          so, the simplified configuration you are suggesting where it's just necessary to setup one config entry with "fair" or "fifo" is already the way it will work. The default for the policy, if unspecified, is the SchedulerComparatorPolicy, so for the cases we are building here, the policy doesn't need to be set in configuration, only the comparator (if other than fifo). I expect that most functionality will be able to be implemented via the SchedulerComparatorPolicy (all that we are implementing at present can be...), and so the "extra" configuration will not be needed. The higher level abstraction is present because it provides a clear point where other approaches could be taken, but those would not also be specifying the comparator side, and so would also not need "double configuration". I am presently spelling out the "FairComparator" class to ease the patch management, when the initial patch goes in there will be a "fair" option to match the "fifo", where that is all that would need to be specified. "fair" can actually be used on it's own without compounding with fifo, the fall back is on the lexical appid, which should be good for cases where "fairness" is really what is wanted, we could add a "fairFifo" shortcut config to give a convenient way to compound them but I don't think it's really needed. When priority becomes available I do intend to support shortcut "priorityFair" "priorityFifo" options to allow for one-stop compound configuration as you suggest for those cases, it's not there yet because it can't be until priority is settled so that it can be incorporated. (to be clear, these "shortcut" options (priorityFair, priorityFifo, others as needed) would be in the same role as "fifo" or "compound" today, and would configure the whole setup w/in the SchedulerComparatorPolicy)

          Show
          cwelch Craig Welch added a comment - Hi Wangda, I have changed the patch a bit in the background without updating it on the jira. The changes are not major but I think they render some of the comments obsolete. I've uploaded the up-to-date patch just now, before doing so I took a pass through your comments- below I'll respond to each in turn- 1) SchedulerProcess 1.1. ...name seems not very straightforward to me... Well, I'm certainly open to other name options, but I do prefer SchedulerProcess to SchedulableEntity - it's common to refer the items which a scheduler will schedule as "Processes", which is what these are in this case, which is why I chose this name. "Entity" is really very generic and empty of meaning. I do wish to avoid confusion with Scheduleable (and wasn't enamored of that name either...), I expect that as integration progresses there will be a period where Schedulable will be an extension of SchedulerProcess with the (remaining) FairScheduler specific bits (which will, I think, ultimately be incorporated in some way into SchedulerProcess, but that's down the line/should be addressed in further iteration). In any case, not in favor of adding "Entity", I think when you consider the terminology as explained above SchedulerProcess works, try it on and see, and feel free to give other options... 1.2. ...SchedulerProcessEvent...asynchronized Not all event handling must be asynchronous. I believe the details regarding this were spelled out reasonably well in the interface definition - if you take a peek at how these events are handled in the capacity scheduler configuration you will see that they are synchronously/safely within protection against mutation of the schedulerprocess collection. My goal was precisely to avoid needing to have implementer add a new method implementation every time a new event comes into play which may not be of interest to it, this makes maintenance of implementations easier - they can manage those which they understand and have appropriate default logic otherwise. I think this is a classic case for an enumerated set of events to be handled by the interface & so I think it should be modeled as it is as opposed to adding a new method for each new event type to the interface itself... 1.3 ...SerialEpoch Yeah, I don't like the names much either, I've gone through several versions and come to the conclusion that it's not the choice of names that's the problem. This is an attempt to "hide" the application id's while also exposing them, which is compound in nature, and is made stranger by the fact that this is totally irrelevant for other potential future implementors (such as queues). I want to factor it differnently not just change the names, these are the courses I'm considering: 1. Have SchedulerProcess implement "Comparable" and provide a "natural ordering", which is fifo for apps. This seems to "privilege" fifo but, as a matter of fact, it's the fallback for fair so I'm not sure that's really an inappropriate thing to do - it seems like it is the "natural ordering" for apps. Other things can give their own natural ordering (queues - the hierarchy...), so it should extend reasonably well without the current awkwardness. This would remove all of "getSerialEpoch", "getSerial", and "getId" in favor of just implementing "compareTo" from comparable. The downsides I see are the "privilage" and that if an implementor of SchedulerProcess implemented "comparable" in an unworkable fashion it would be an issue, not the case for what we are presently looking at supporting afaik 2. Have an explicit "compareCreationOrder(SchedulerProcess other)" method which returns 0 + - like compareTo. This is much like 1, but removes the "privilege" and the possible Comparable collision... this also does away getSerialEpoch, getSerial, and getId in favor of the comparison method. What do you think of these options? Preference? BTW, FS has an actual "startTime" for fsappattempts, but looking through it I don't like that approach - it doesn't appear to do the right thing in some cases (like rm failover or recovery), it still can be ambiguous for some cases (simultanious start w/in tsmillis granularity) where there's a fallback to appid, so it doesn't look to really add anything as you still have to be able to fall back to the app id for those cases so you can't get away from the issue, and it adds a bit of complexity to boot. 1.4 ...currentConsumption is not enough to make choice, demand(pending-resource)/used and priority/weight are basic fields of a Schedulable, do you think so... Of those, only demand is required for the initial step of supporting application level fairness when sizeBasedWeight is active, the others are only relevant for other uses (such as queues) which we're not taking on in the first go. I had already added demand to the interface, it is presently being calculated using pending requests. I looked at switching to using the pending value of the attemptResourceUsage in the SchedulerApplicationAttempt but I don't see that it is being updated anywhere, am I missing it? It seems that it should have been made to do so when it was added to SchedulerApplicationAttempt but that it was possibly missed? In any case, if it is/were it made to be I could use it. Also, looking over ResourceUsage, I think there needs to be a "getCopyPending" or the like, for usecases where you get the value and then start to use it, as the code is today it can be mutated as soon as the value is returned, so while in some sense it's threadsafe, not really in a usable way for many callers who would want a snapshot copy of the final value which won't change (as in this case...) 2) Ordering Policy 2.1 addSchedulerProcess/removeSchedulerProcess/addAllSchedulerProcesses ...Collection... treeSet I think the current factoring is correct. First, you'll notice that the return type of getSchedulerProcess() is a Collection, in no way is this tied to treeSet, implementers are welcome to use any collection type they like (or implement the minimal collection interface themselves if really necessary...). It is appropriate to have a Collection view on the processes, it is the proper abstraction for it and it's used in various places throughout the scheduler - effectively the ask is to add the collection interface methods to this interface, which doesn't make sense, it's better to use the existing/common interface as I am doing at present 2.2 ... no it doesn't, see 1.2 above 2.3 rename...SchedulerProcessOrderingPolicy I suppose we could, although that assumes we keep SchedulerProcess I thought that an unnecessarily long name & that there would not be a lot of ordering policies to give rise to ambiguity, but assuming we stick with SchedulerProcess, or whatever we do name it, we could expand this out - not sure its really necessary, but not really opposed... 3) ...configuration... so, the simplified configuration you are suggesting where it's just necessary to setup one config entry with "fair" or "fifo" is already the way it will work. The default for the policy, if unspecified, is the SchedulerComparatorPolicy, so for the cases we are building here, the policy doesn't need to be set in configuration, only the comparator (if other than fifo). I expect that most functionality will be able to be implemented via the SchedulerComparatorPolicy (all that we are implementing at present can be...), and so the "extra" configuration will not be needed. The higher level abstraction is present because it provides a clear point where other approaches could be taken, but those would not also be specifying the comparator side, and so would also not need "double configuration". I am presently spelling out the "FairComparator" class to ease the patch management, when the initial patch goes in there will be a "fair" option to match the "fifo", where that is all that would need to be specified. "fair" can actually be used on it's own without compounding with fifo, the fall back is on the lexical appid, which should be good for cases where "fairness" is really what is wanted, we could add a "fairFifo" shortcut config to give a convenient way to compound them but I don't think it's really needed. When priority becomes available I do intend to support shortcut "priorityFair" "priorityFifo" options to allow for one-stop compound configuration as you suggest for those cases, it's not there yet because it can't be until priority is settled so that it can be incorporated. (to be clear, these "shortcut" options (priorityFair, priorityFifo, others as needed) would be in the same role as "fifo" or "compound" today, and would configure the whole setup w/in the SchedulerComparatorPolicy)
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12708385/YARN-3318.35.patch
          against trunk revision b5a22e9.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 8 new or modified test files.

          -1 javac. The applied patch generated 1151 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

          org.apache.hadoop.yarn.client.cli.TestYarnCLI
          org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesCapacitySched

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7168//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7168//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7168//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7168//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12708385/YARN-3318.35.patch against trunk revision b5a22e9. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified test files. -1 javac . The applied patch generated 1151 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: org.apache.hadoop.yarn.client.cli.TestYarnCLI org.apache.hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesCapacitySched Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7168//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7168//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7168//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7168//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          Fixes for release audit warnings

          Show
          cwelch Craig Welch added a comment - Fixes for release audit warnings
          Hide
          leftnoteasy Wangda Tan added a comment -

          Well, I'm certainly open to other name options, but I do prefer SchedulerProcess to SchedulableEntity ...

          I don't like generic "Entity" as well, I cannot get a better name now, so just keep it as-is now and see if anybody has better suggestions, renaming should be simple

          Not all event handling must be asynchronous... My goal was precisely to avoid needing to have implementer add a new method implementation every time a new event comes into play which may not be of interest to it

          This is one thing I don't agree. CS implemnts EventHandler is that CS will be called by other modules in asynchronized way, it handle such events one by one, but dispatcher in RM will queuing events for CS. I think SchedulerProcess will likely to be used in a sycnhronized way, making EventHandler can make less changes for interfaces but also reduces readability. I think this is why all scheduler internal classes choose to use setters instead of event handler

          SerialEpoch..

          I read FairShareComparator and also FifoComparator again, how about keep an ID() only, it's a string and should contains everything we need to compare "nature" ordering of two SchedulerProcess, instead of policies in FairScheduler confusing different fields to compare nature order. We will keep a note that, we will use lexicographical order of id() to compare. submitTime should be an important property that need to be kept in SchedulerProcess even if we don't use it for now.

          I looked at switching to using the pending value of the attemptResourceUsage in the SchedulerApplicationAttempt but I don't see that it is being updated anywhere, am I missing it?

          Oh I should mention that, such fields are set in YARN-3361, beyond requirements of fair policy, these pending values are used when doing normal scheduling as well as preemption.

          I think there needs to be a "getCopyPending" or the like, for usecases where you get the value and then start to use it.

          Good suggestion, I think we should do that to avoid resource being modified while using by another reader. I think we can make internal resources in ResourceUsage to be "copy-on-write", to make sure resources returned by getters is just a snapshot.

          I think the current factoring is correct. First, you'll notice that the return type of getSchedulerProcess() is a Collection

          The thing I'm worrying about is, this assumes caller can modify the collection. It is very possible returned collection is just a copy or immutable collection to avoid race condition according to different SchedulerProcess impl. So I think it's better to make callers can only use limited interfaces to get/set internal structure.
          And also, there could have some operations need to do when add/remove object in the internal data structure, caller shouldn't silently modify it.

          ...Simplify configuration...

          I suggest you can take a look at YARN-2986, which is very related to this discussion.
          We don't need to manually set comparator for most cases, instead, it comes with policy. I don't want to expose "comparator" to end user since it's too specific to internal logics. Instead, user need just specify queue's policy="fair" and set some "policy-options" like application-priority-enabled=true, etc.
          "policy=fair" assumes use comparator=fair+fifo, when application-priority-enabled, it assumes "priority comparator" added to "CompoundComparator" – they're all internal logics.
          The configuration in my mind is (it's hierarchy config, also we can make it to be plain config)

          <queue>
            <policy name="fair">
               <application-priority-enabled>true</application-priority-enabled>
               <size-based-weight-enabled>true</size-based-weight-enabled>
            </policy>
          
          Show
          leftnoteasy Wangda Tan added a comment - Well, I'm certainly open to other name options, but I do prefer SchedulerProcess to SchedulableEntity ... I don't like generic "Entity" as well, I cannot get a better name now, so just keep it as-is now and see if anybody has better suggestions, renaming should be simple Not all event handling must be asynchronous... My goal was precisely to avoid needing to have implementer add a new method implementation every time a new event comes into play which may not be of interest to it This is one thing I don't agree. CS implemnts EventHandler is that CS will be called by other modules in asynchronized way, it handle such events one by one, but dispatcher in RM will queuing events for CS. I think SchedulerProcess will likely to be used in a sycnhronized way, making EventHandler can make less changes for interfaces but also reduces readability. I think this is why all scheduler internal classes choose to use setters instead of event handler SerialEpoch.. I read FairShareComparator and also FifoComparator again, how about keep an ID() only, it's a string and should contains everything we need to compare "nature" ordering of two SchedulerProcess, instead of policies in FairScheduler confusing different fields to compare nature order. We will keep a note that, we will use lexicographical order of id() to compare. submitTime should be an important property that need to be kept in SchedulerProcess even if we don't use it for now. I looked at switching to using the pending value of the attemptResourceUsage in the SchedulerApplicationAttempt but I don't see that it is being updated anywhere, am I missing it? Oh I should mention that, such fields are set in YARN-3361 , beyond requirements of fair policy, these pending values are used when doing normal scheduling as well as preemption. I think there needs to be a "getCopyPending" or the like, for usecases where you get the value and then start to use it. Good suggestion, I think we should do that to avoid resource being modified while using by another reader. I think we can make internal resources in ResourceUsage to be "copy-on-write", to make sure resources returned by getters is just a snapshot. I think the current factoring is correct. First, you'll notice that the return type of getSchedulerProcess() is a Collection The thing I'm worrying about is, this assumes caller can modify the collection. It is very possible returned collection is just a copy or immutable collection to avoid race condition according to different SchedulerProcess impl. So I think it's better to make callers can only use limited interfaces to get/set internal structure. And also, there could have some operations need to do when add/remove object in the internal data structure, caller shouldn't silently modify it. ...Simplify configuration... I suggest you can take a look at YARN-2986 , which is very related to this discussion. We don't need to manually set comparator for most cases, instead, it comes with policy. I don't want to expose "comparator" to end user since it's too specific to internal logics. Instead, user need just specify queue's policy="fair" and set some "policy-options" like application-priority-enabled=true, etc. "policy=fair" assumes use comparator=fair+fifo, when application-priority-enabled, it assumes "priority comparator" added to "CompoundComparator" – they're all internal logics. The configuration in my mind is (it's hierarchy config, also we can make it to be plain config) <queue> <policy name= "fair" > <application-priority-enabled> true </application-priority-enabled> <size-based-weight-enabled> true </size-based-weight-enabled> </policy>
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12708491/YARN-3318.36.patch
          against trunk revision 79f7f2a.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified test files.

          -1 javac. The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

          org.apache.hadoop.yarn.server.resourcemanager.TestRM

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7175//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7175//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7175//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12708491/YARN-3318.36.patch against trunk revision 79f7f2a. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified test files. -1 javac . The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: org.apache.hadoop.yarn.server.resourcemanager.TestRM Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7175//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7175//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7175//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          The remaining javac error doesn't appear to be related to my changes, which is confusing. On the next patch will have a change to try and address it anyway. TestRM passes on my box, I assume it's a transient issue.

          Show
          cwelch Craig Welch added a comment - The remaining javac error doesn't appear to be related to my changes, which is confusing. On the next patch will have a change to try and address it anyway. TestRM passes on my box, I assume it's a transient issue.
          Hide
          cwelch Craig Welch added a comment -

          BTW, can't just do lexical sort on the string version of application id - one problem with using the lexical compare on appid, the format for the "id" component is a min of 4 digits, which means that going from 9999 to 10000 will result in incorrect lexical sort wrt to actual order

          Show
          cwelch Craig Welch added a comment - BTW, can't just do lexical sort on the string version of application id - one problem with using the lexical compare on appid, the format for the "id" component is a min of 4 digits, which means that going from 9999 to 10000 will result in incorrect lexical sort wrt to actual order
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Top level comments

          • It is not entirely clear how the ordering and limits work together - as one policy with multiple facets or multiple policy types. So we need to think more about what we expose right now
            • let's split the patch that exposes this to the client side / web UI and in the API records into its own JIRA. We should do it a little later after the framework stabilizes.
            • Capacity Scheduler Configuration also need more thought in terms of naming and layout. I think it's premature to support this as a publicly supportable configuration. But at the same time, to enable experimentation, we should simply not put in the public side of the code.
          • For future's sake and for discussion with FairScheduler folks, I think it is useful to split off CS changes into their own JIRA. We can strictly focus on the policy framework here.

          Quick comments on the policy stuff itself

          • I also like SchedulableEntity better. If not that, simply Schedulable works too
          • You add/remove applications to/from LeafQueue's policy but addition/removal of containers is an event. I think we should be consistent with these two. May be invent some sort of a notion of parent/child SchedulableEntities and make this generic?
          • The notion of a comparator doesn't make sense to an admin. It is simply a policy. Depending on how ordering and limits come together, they may become properties of a policy?
          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Top level comments It is not entirely clear how the ordering and limits work together - as one policy with multiple facets or multiple policy types. So we need to think more about what we expose right now let's split the patch that exposes this to the client side / web UI and in the API records into its own JIRA. We should do it a little later after the framework stabilizes. Capacity Scheduler Configuration also need more thought in terms of naming and layout. I think it's premature to support this as a publicly supportable configuration. But at the same time, to enable experimentation, we should simply not put in the public side of the code. For future's sake and for discussion with FairScheduler folks, I think it is useful to split off CS changes into their own JIRA. We can strictly focus on the policy framework here. Quick comments on the policy stuff itself I also like SchedulableEntity better. If not that, simply Schedulable works too You add/remove applications to/from LeafQueue's policy but addition/removal of containers is an event. I think we should be consistent with these two. May be invent some sort of a notion of parent/child SchedulableEntities and make this generic? The notion of a comparator doesn't make sense to an admin. It is simply a policy. Depending on how ordering and limits come together, they may become properties of a policy?
          Hide
          cwelch Craig Welch added a comment -

          Wangda Tan

          SchedulerProcessEvents replaced with containerAllocated and containerReleased
          Serial and SerialEpoch replaced with compareInputOrderTo(), which is the option 2 for addressing it which we settled on offline
          Added addSchedulerProcess/removeSchedulerProcess/addAllSchedulerProcesses
          Changed configuration so that
          yarn.scheduler.capacity.root.default.ordering-policy=fair
          will setup the fair configuration, "fifo" will setup fifo, "fair+fifo" will setup compound fair + fifo, etc. It is possible to setup a custom ordering policy class using a different configuration, but the base one will handle the "friendly" setup.

          Vinod Kumar Vavilapalli

          It is not entirely clear how the ordering and limits work together - as one policy with multiple facets or multiple policy types

          This should be modeled as different types of policies, so that they can each focus on their particular purpose and avoid a repetition of the intermingling which has made it difficult to mix, match, and share capabilities. Having multiple policy types is essential to make it easy to combine them as needed.

          let's split the patch that exposes this to the client side / web UI and in the API records into its own JIRA...premature to support this as a publicly supportable configuration...

          The goal is to make this available quickly but iteratively, keeping the changes small but making them available for use and feedback. Clearly we can mark things "unstable", communicate that they are not fully mature/subject to change/should be used gently, but we will need to make it possible to activate the feature and use it in order to accomplish the use and feedback. We should grow it organically, gradually, iteratively, think of it is a facet of the policy framework hooked up and available but with more to follow

          ...SchedulableEntity better... well, I'd actually talked Wangda Tan into SchedulerProcess So, we can chew on this a bit more & see where we go

          You add/remove applications to/from LeafQueue's policy but addition/removal of containers is an event...

          This has been factored differently along Wangda Tan's suggestion, it should now be consistent

          The notion of a comparator doesn't make sense to an admin. It is simply a policy...

          Have modeled "policy configuration" differently, so "comparator" is out of sight (see above).

          Depending on how ordering and limits come together, they may become properties of a policy

          I expect them to be distinct, this is specifically an "ordering-policy", limits will be other types of "limit-policy"(ies)

          patch with these changes to follow in a few...

          Show
          cwelch Craig Welch added a comment - Wangda Tan SchedulerProcessEvents replaced with containerAllocated and containerReleased Serial and SerialEpoch replaced with compareInputOrderTo(), which is the option 2 for addressing it which we settled on offline Added addSchedulerProcess/removeSchedulerProcess/addAllSchedulerProcesses Changed configuration so that yarn.scheduler.capacity.root.default.ordering-policy=fair will setup the fair configuration, "fifo" will setup fifo, "fair+fifo" will setup compound fair + fifo, etc. It is possible to setup a custom ordering policy class using a different configuration, but the base one will handle the "friendly" setup. Vinod Kumar Vavilapalli It is not entirely clear how the ordering and limits work together - as one policy with multiple facets or multiple policy types This should be modeled as different types of policies, so that they can each focus on their particular purpose and avoid a repetition of the intermingling which has made it difficult to mix, match, and share capabilities. Having multiple policy types is essential to make it easy to combine them as needed. let's split the patch that exposes this to the client side / web UI and in the API records into its own JIRA...premature to support this as a publicly supportable configuration... The goal is to make this available quickly but iteratively, keeping the changes small but making them available for use and feedback. Clearly we can mark things "unstable", communicate that they are not fully mature/subject to change/should be used gently, but we will need to make it possible to activate the feature and use it in order to accomplish the use and feedback. We should grow it organically, gradually, iteratively, think of it is a facet of the policy framework hooked up and available but with more to follow ...SchedulableEntity better... well, I'd actually talked Wangda Tan into SchedulerProcess So, we can chew on this a bit more & see where we go You add/remove applications to/from LeafQueue's policy but addition/removal of containers is an event... This has been factored differently along Wangda Tan 's suggestion, it should now be consistent The notion of a comparator doesn't make sense to an admin. It is simply a policy... Have modeled "policy configuration" differently, so "comparator" is out of sight (see above). Depending on how ordering and limits come together, they may become properties of a policy I expect them to be distinct, this is specifically an "ordering-policy", limits will be other types of "limit-policy"(ies) patch with these changes to follow in a few...
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12708923/YARN-3318.39.patch
          against trunk revision 867d5d2.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified test files.

          -1 javac. The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

          org.apache.hadoop.yarn.client.api.impl.TestAMRMClient

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7198//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7198//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7198//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12708923/YARN-3318.39.patch against trunk revision 867d5d2. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified test files. -1 javac . The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: org.apache.hadoop.yarn.client.api.impl.TestAMRMClient Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7198//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7198//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7198//console This message is automatically generated.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          I think it is useful to split off CS changes into their own JIRA. We can strictly focus on the policy framework here.

          You missed this, let's please do this.

          well, I'd actually talked Wangda Tan into SchedulerProcess So, we can chew on this a bit more & see where we go

          SchedulerProcess is definitely misleading. It seems to point to a process that is doing scheduling. What you need is a Schedulable / SchedulableEntity / Consumer etc. You could also say SchedulableProcess, but Process is way too overloaded.

          The goal is to make this available quickly but iteratively, keeping the changes small but making them available for use and feedback. (..) We should grow it organically, gradually, iteratively, think of it is a facet of the policy framework hooked up and available but with more to follow

          I agree to this, but we are not in a position to support the APIs, CLI, config names in a supportable manner yet. They may or may not change depending on how parent queue policies, limit policies evolve. For that reason alone, I am saying that (1) Don't make the configurations public yet, or put a warning saying that they are unstable and (2) don't expose them in CLI , REST APIs yet. It's okay to put in the web UI, web UI scraping is not a contract.

          You add/remove applications to/from LeafQueue's policy but addition/removal of containers is an event...

          This has been factored differently along Wangda Tan's suggestion, it should now be consistent

          It's a bit better now. Although we are hard-coding Containers. Can revisit this later.

          Other comments

          • SchedulerApplicationAttempt.getDemand() should be private.
          • SchedulerProcess
            • updateCaches() -> updateState() / updateSchedulingState() as that is what it is doing?
            • getCachedConsumption() / getCachedDemand(): simply getCurrent*() ?
          • SchedulerComparator
            • We aren't comparing Schedulers. Given the current name, it should have been SchedulerProcessComparator, but SchedulerProcess itself should be renamed as mentioned before.
            • What is the need for reorderOnContainerAllocate () / reorderOnContainerRelease()?
          • Move all the comparator related classed into their own package.
          • SchedulerComparatorPolicy
            • This is really a ComparatorBasedOrderingPolicy. Do we really see non-comparator based ordering-policy. We are unnecessarily adding two abstractions - adding policies and comparators.
            • Use className.getName() instead of hardcoded strings like "org.apache.hadoop.yarn.server.resourcemanager.scheduler.policy.FifoComparator"
          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - I think it is useful to split off CS changes into their own JIRA. We can strictly focus on the policy framework here. You missed this, let's please do this. well, I'd actually talked Wangda Tan into SchedulerProcess So, we can chew on this a bit more & see where we go SchedulerProcess is definitely misleading. It seems to point to a process that is doing scheduling. What you need is a Schedulable / SchedulableEntity / Consumer etc. You could also say SchedulableProcess, but Process is way too overloaded. The goal is to make this available quickly but iteratively, keeping the changes small but making them available for use and feedback. (..) We should grow it organically, gradually, iteratively, think of it is a facet of the policy framework hooked up and available but with more to follow I agree to this, but we are not in a position to support the APIs, CLI, config names in a supportable manner yet. They may or may not change depending on how parent queue policies, limit policies evolve. For that reason alone, I am saying that (1) Don't make the configurations public yet, or put a warning saying that they are unstable and (2) don't expose them in CLI , REST APIs yet. It's okay to put in the web UI, web UI scraping is not a contract. You add/remove applications to/from LeafQueue's policy but addition/removal of containers is an event... This has been factored differently along Wangda Tan's suggestion, it should now be consistent It's a bit better now. Although we are hard-coding Containers. Can revisit this later. Other comments SchedulerApplicationAttempt.getDemand() should be private. SchedulerProcess updateCaches() -> updateState() / updateSchedulingState() as that is what it is doing? getCachedConsumption() / getCachedDemand(): simply getCurrent*() ? SchedulerComparator We aren't comparing Schedulers. Given the current name, it should have been SchedulerProcessComparator, but SchedulerProcess itself should be renamed as mentioned before. What is the need for reorderOnContainerAllocate () / reorderOnContainerRelease()? Move all the comparator related classed into their own package. SchedulerComparatorPolicy This is really a ComparatorBasedOrderingPolicy. Do we really see non-comparator based ordering-policy. We are unnecessarily adding two abstractions - adding policies and comparators. Use className.getName() instead of hardcoded strings like "org.apache.hadoop.yarn.server.resourcemanager.scheduler.policy.FifoComparator"
          Hide
          leftnoteasy Wangda Tan added a comment -

          Craig Welch,
          I took a look at your latest patch as well as Vinod Kumar Vavilapalli's suggestions, comments:

          1. I prefer what Vinod suggested, split "SchedulerProcess" to be "QueueSchedulable" and "AppSchedulable" to avoid notes in FairScheduler interface "Schedulable" like:

          /** Start time for jobs in FIFO queues; meaningless for QueueSchedulables.*/
          

          They can both inherit Schedulable. With this patch, we can limit to AppSchedulable and Schedulable definition.
          Also, regarding to schedulable comparator, not all "Schedulable" fit for all comparator, it's meaningless to do "FIFO" scheduling in parent queue level.
          I think:

          Schedulable contains ResourceUsage (class), and name
          In addition, AppSchedulable contains compareSubmissionOrderTo(AppSchedulable) and Priority
          

          2. About inherit relationships between interfaces/classes, now it's not very clear to me, I spent some time got what they're doing. My suggestion is:

          FairOrderingPolicy/FifoOrderingPolicy ------------> OrderingPolicy
                                                (implements)
          FairOrderingPolicy and FifoOrderingPolicy could inherit from AbstractOrderingPolicy with commmon implementations
          
          FairOrderingPolicy/FifoOrderingPolicy ------------> FairSchedulableComparator/FifoSchedulableComparator
                                                  (uses)
          It's no need to invent "SchedulerComparator" interface, use existing Java Comparator interface should be simple and enough.
          

          3. Regarding relationship between OrderingPolicy and comparator:
          I understand the method of SchedulerComparator is to reduce unnecessary re-sort Schedulables being added/modified in OrderingPolicy, but actually we can
          1) Do this in OrderingPolicy itself. For example, with my above suggestion, FifoOrderingPolicy will simply ignore container changed notifications.
          2) Comparator doesn't know about global info, only OrderingPolicy knows how combination of Comparator actors, I don't want containerAllocate/Release coupled in Comparator interface.
          And we don't need a separated "CompoundComparator", this can be put in AbstractOrderingPolicy.

          4. Regarding configuration (CapacitySchedulerConfiguration):
          I think we don't need ORDERING_POLICY_CLASS, two operations for very similar purpose can confuse user. I suggest only leave ordering-policy, and it name can be:
          "fifo", "fair" regardless of its internal "comparator" implementaiton. And in the future we can add "priority-fifo", "priority-fair". (note the "-" in name doesn't means "AND" only, it could be collaborate of the two instead of simply combination).
          If user specify a name not in white-list-shortname given by us, we will try to load class with the name.

          5. Regarding longer term plan, LimitPolicy:
          This part seems not well discussed, to limit scope of this JIRA, so I think its implementation and definition should happen in separated ticket.
          For longer plan, considering YARN-2986 as well, we may configure queue like following:

          <queue name="a">
              <queues>
                  <queue name = "a1">
                      <policy-properties>
                          <ordering-policy>fair</ordering-policy>
                          <limit-policy>
                              <user-limit-policy>
                                <enabled>true</enabled>
                                <user-limit-percentage>50</user-limit-percentage>
                              </user-limit-policy>
                              <queue-capacity-policy>
                                 <capacity>..</capacity>
                                 <max-capacity>..</max-capacity>
                              </queue-capacity-policy>
                          </limit-policy>
                      </policy-properties>
                  </queue>
              <queues>
          </queue>
          

          Changes of this patch in CapacitySchedulerConfiguration seems reasonable, as Craig mentioned, simply mark it to be unstable or experimental should be enough. Longer term is to define and stablize YARN-2986 to make a real uniformed scheduler.

          6. Regarding scope of this JIRA
          I think it should be fine to make policy interfaces define as well as CapacityScheduler changes together with this patch (only for FifoOrderingPolicy), it's good to see how interfaces and policies work in CS, is it easy or not, etc. =
          And following I suggest to move to a separated ticket:
          1) UI (Web and CLI)
          2) REST
          3) PB related changes
          Along with patch getting changes, you don't have to maintain above changes together with the patch.

          Please feel free to let me know your thoughts.

          Show
          leftnoteasy Wangda Tan added a comment - Craig Welch , I took a look at your latest patch as well as Vinod Kumar Vavilapalli 's suggestions, comments: 1. I prefer what Vinod suggested, split "SchedulerProcess" to be "QueueSchedulable" and "AppSchedulable" to avoid notes in FairScheduler interface "Schedulable" like: /** Start time for jobs in FIFO queues; meaningless for QueueSchedulables.*/ They can both inherit Schedulable . With this patch, we can limit to AppSchedulable and Schedulable definition. Also, regarding to schedulable comparator, not all "Schedulable" fit for all comparator, it's meaningless to do "FIFO" scheduling in parent queue level. I think: Schedulable contains ResourceUsage (class), and name In addition, AppSchedulable contains compareSubmissionOrderTo(AppSchedulable) and Priority 2. About inherit relationships between interfaces/classes, now it's not very clear to me, I spent some time got what they're doing. My suggestion is: FairOrderingPolicy/FifoOrderingPolicy ------------> OrderingPolicy ( implements ) FairOrderingPolicy and FifoOrderingPolicy could inherit from AbstractOrderingPolicy with commmon implementations FairOrderingPolicy/FifoOrderingPolicy ------------> FairSchedulableComparator/FifoSchedulableComparator (uses) It's no need to invent "SchedulerComparator" interface , use existing Java Comparator interface should be simple and enough. 3. Regarding relationship between OrderingPolicy and comparator: I understand the method of SchedulerComparator is to reduce unnecessary re-sort Schedulables being added/modified in OrderingPolicy, but actually we can 1) Do this in OrderingPolicy itself. For example, with my above suggestion, FifoOrderingPolicy will simply ignore container changed notifications. 2) Comparator doesn't know about global info, only OrderingPolicy knows how combination of Comparator actors, I don't want containerAllocate/Release coupled in Comparator interface. And we don't need a separated "CompoundComparator", this can be put in AbstractOrderingPolicy. 4. Regarding configuration (CapacitySchedulerConfiguration): I think we don't need ORDERING_POLICY_CLASS, two operations for very similar purpose can confuse user. I suggest only leave ordering-policy, and it name can be: "fifo", "fair" regardless of its internal "comparator" implementaiton. And in the future we can add "priority-fifo", "priority-fair". (note the "-" in name doesn't means "AND" only, it could be collaborate of the two instead of simply combination). If user specify a name not in white-list-shortname given by us, we will try to load class with the name. 5. Regarding longer term plan, LimitPolicy: This part seems not well discussed, to limit scope of this JIRA, so I think its implementation and definition should happen in separated ticket. For longer plan, considering YARN-2986 as well, we may configure queue like following: <queue name= "a" > <queues> <queue name = "a1" > <policy-properties> <ordering-policy>fair</ordering-policy> <limit-policy> <user-limit-policy> <enabled> true </enabled> <user-limit-percentage>50</user-limit-percentage> </user-limit-policy> <queue-capacity-policy> <capacity>..</capacity> <max-capacity>..</max-capacity> </queue-capacity-policy> </limit-policy> </policy-properties> </queue> <queues> </queue> Changes of this patch in CapacitySchedulerConfiguration seems reasonable, as Craig mentioned, simply mark it to be unstable or experimental should be enough. Longer term is to define and stablize YARN-2986 to make a real uniformed scheduler. 6. Regarding scope of this JIRA I think it should be fine to make policy interfaces define as well as CapacityScheduler changes together with this patch (only for FifoOrderingPolicy), it's good to see how interfaces and policies work in CS, is it easy or not, etc. = And following I suggest to move to a separated ticket: 1) UI (Web and CLI) 2) REST 3) PB related changes Along with patch getting changes, you don't have to maintain above changes together with the patch. Please feel free to let me know your thoughts.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          I think it should be fine to make policy interfaces define as well as CapacityScheduler changes together with this patch (only for FifoOrderingPolicy), it's good to see how interfaces and policies work in CS, is it easy or not, etc. =

          We can still do this with patches on two JIRAs - one for the framework, one for CS, one for FS etc. The Fifo one can be here for demonstration, no problem with that. Why is it so hard to focus one thing in one JIRA?

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - I think it should be fine to make policy interfaces define as well as CapacityScheduler changes together with this patch (only for FifoOrderingPolicy), it's good to see how interfaces and policies work in CS, is it easy or not, etc. = We can still do this with patches on two JIRAs - one for the framework, one for CS, one for FS etc. The Fifo one can be here for demonstration, no problem with that. Why is it so hard to focus one thing in one JIRA?
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Filed YARN-3441 and YARN-3442 for parent queues and for limits.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Filed YARN-3441 and YARN-3442 for parent queues and for limits.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12709343/YARN-3318.45.patch
          against trunk revision 023133c.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javac. The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7216//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7216//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7216//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7216//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12709343/YARN-3318.45.patch against trunk revision 023133c. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7216//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7216//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7216//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7216//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          Vinod Kumar Vavilapalli

          ...We can strictly focus on the policy framework here...

          Sure, limited patch to "framework"

          ...You could also say SchedulableProcess...

          SchedulableProcess it is, done

          I agree to this, but we are not in a position to support the APIs, CLI, config names in a supportable manner yet. They may or may not change depending on how parent queue policies, limit policies evolve. For that reason alone, I am saying that (1) Don't make the configurations public yet, or put a warning saying that they are unstable and (2) don't expose them in CLI , REST APIs yet. It's okay to put in the web UI, web UI scraping is not a contract.

          You can't see it, because it's part of "Capacity Scheduler Integration", but removed CLI and proto related change. There was no rest api change, the web UI change is still present. Will warn unstable when added to config files in the scheduler integration patch

          SchedulerApplicationAttempt.getDemand() should be private

          Done

          updateCaches() -> updateState() / updateSchedulingState() as that is what it is doing? getCachedConsumption() / getCachedDemand(): simply getCurrent*() ? What is the need for reorderOnContainerAllocate () / reorderOnContainerRelease()?

          Is now getSchedulingConsumption(); getSchedulingDemand(); updateSchedulingState();

          This is needed because mutable values which are used for ordering cannot be allowed to change for an item in the tree, else it will not be found in some cases during the delete before reinsert process which occurs when a schedulable's mutable values used in comparison change (for fairness, changes to consumption and potentially demand) Not all OrderingPolicies require reordering on these events, for efficiency they get to decide if they do or not, hence the "reorderOn". The "reorderOn" are now reorderForContainerAllocation reorderForContainerRelease

          Move all the comparator related classed into their own package

          No longer needed as comparators are now just a property of policies, see below for details

          This is really a ComparatorBasedOrderingPolicy. Do we really see non-comparator based ordering-policy. We are unnecessarily adding two abstractions - adding policies and comparators

          Originally, there was a perceived need to be able to support a more flexible interface than the comparator one, but also a desire to build up a simpler, composible abstraction to be used with an instance of the former which had "most of the hard stuff done". Given that all of the policies we've contemplated building fit the latter abstraction and the level of flexibility does not appear to actually be that different, I think it's fair to say that we only need what was previously the "SchedulerComparator" abstraction as a plugin-point. Given that, a slightly refactored version of the "SchedulerComparator" abstraction is now the only plugin point and is now what goes by the name of "OrderingPolicy". What was previously the "OrderingPolicy" is now a single concrete class implementing the surrounding logic, meant to be usable from any scheduler, named "SchedulingOrder". So, one abstraction, a comparator-based ordering-policy. If we really do find we need a flexibility we don't have some day, the SchedulingOrder class could be abstracted to provide that higher level abstraction - but as we see no need for it now, and it appears probably never will, there's no reason to do so at present

          ...Use className.getName()...

          Done

          Wangda Tan

          ...I prefer what Vinod suggested, split "SchedulerProcess" to be "QueueSchedulable" and "AppSchedulable" ...

          I don't see that he has suggested that. In any case, with the removal of "Serial" and the move to compareInputOrderTo() I don't at present see a need to have separate subtypes for app and queue to avoid "dangling properties". And, I think if we do it right we won't end up introducing them. By splitting in the suggested way we commit ourselves to either multiple comparators (to use the differing functionality) or awkward testing of subtype/etc logic in one comparator - so it basically moves the complexity/awkwardness, it doesn't eliminate it. I've refactored such that the Policy now provides a Comparator as opposed to extending it, so there is now room for it to provide multiple comparators and handle subtypes if need be, but I think we should wait until we see that we must do that before doing so, as I don't believe we will end up needing to (but if we do, existing code should need little change, and implementing what you suggest should be essentially additive...)

          ...About inherit relationships between interfaces/classes...

          Policies will be composed to achieve combined capabilities yet the collection of SchedulableProcesses & as such must reside in one instance, so attempting to place that logic in the multiple-instances (where multiple policies are constructed, each containing a SchedulerProcess collection via inheritance) is a non starter. Before the latest refactor this was handled by having a single SchedulerComparatorOrderingPolicy with multiple Comparators, post the latest refactor the SchedulingOrder is a single concrete type containing the SchedulableProcess collection, and using one or more OrderingPolicies which possess the policy specific logic (including, but not limited to, comparator logic). As it happens, they do not appear to need to share logic, so there is no super class, just a shared interface implementation - were that to change we could certainly adjust the implementation, but there's no need to now, and it's an implementation not an api issue anyway.

          ...Regarding relationship between OrderingPolicy and comparator...

          There's no longer this duality, there are now just OrderingPolicies

          ...Regarding configuration...

          There's now just one type of configuration, for policy, and will handle "friendly policy labels". Only the fifo is available now, and generalized composition logic, it's in SchedulingOrder

          Show
          cwelch Craig Welch added a comment - Vinod Kumar Vavilapalli ...We can strictly focus on the policy framework here... Sure, limited patch to "framework" ...You could also say SchedulableProcess... SchedulableProcess it is, done I agree to this, but we are not in a position to support the APIs, CLI, config names in a supportable manner yet. They may or may not change depending on how parent queue policies, limit policies evolve. For that reason alone, I am saying that (1) Don't make the configurations public yet, or put a warning saying that they are unstable and (2) don't expose them in CLI , REST APIs yet. It's okay to put in the web UI, web UI scraping is not a contract. You can't see it, because it's part of "Capacity Scheduler Integration", but removed CLI and proto related change. There was no rest api change, the web UI change is still present. Will warn unstable when added to config files in the scheduler integration patch SchedulerApplicationAttempt.getDemand() should be private Done updateCaches() -> updateState() / updateSchedulingState() as that is what it is doing? getCachedConsumption() / getCachedDemand(): simply getCurrent*() ? What is the need for reorderOnContainerAllocate () / reorderOnContainerRelease()? Is now getSchedulingConsumption(); getSchedulingDemand(); updateSchedulingState(); This is needed because mutable values which are used for ordering cannot be allowed to change for an item in the tree, else it will not be found in some cases during the delete before reinsert process which occurs when a schedulable's mutable values used in comparison change (for fairness, changes to consumption and potentially demand) Not all OrderingPolicies require reordering on these events, for efficiency they get to decide if they do or not, hence the "reorderOn". The "reorderOn" are now reorderForContainerAllocation reorderForContainerRelease Move all the comparator related classed into their own package No longer needed as comparators are now just a property of policies, see below for details This is really a ComparatorBasedOrderingPolicy. Do we really see non-comparator based ordering-policy. We are unnecessarily adding two abstractions - adding policies and comparators Originally, there was a perceived need to be able to support a more flexible interface than the comparator one, but also a desire to build up a simpler, composible abstraction to be used with an instance of the former which had "most of the hard stuff done". Given that all of the policies we've contemplated building fit the latter abstraction and the level of flexibility does not appear to actually be that different, I think it's fair to say that we only need what was previously the "SchedulerComparator" abstraction as a plugin-point. Given that, a slightly refactored version of the "SchedulerComparator" abstraction is now the only plugin point and is now what goes by the name of "OrderingPolicy". What was previously the "OrderingPolicy" is now a single concrete class implementing the surrounding logic, meant to be usable from any scheduler, named "SchedulingOrder". So, one abstraction, a comparator-based ordering-policy. If we really do find we need a flexibility we don't have some day, the SchedulingOrder class could be abstracted to provide that higher level abstraction - but as we see no need for it now, and it appears probably never will, there's no reason to do so at present ...Use className.getName()... Done Wangda Tan ...I prefer what Vinod suggested, split "SchedulerProcess" to be "QueueSchedulable" and "AppSchedulable" ... I don't see that he has suggested that. In any case, with the removal of " Serial " and the move to compareInputOrderTo() I don't at present see a need to have separate subtypes for app and queue to avoid "dangling properties". And, I think if we do it right we won't end up introducing them. By splitting in the suggested way we commit ourselves to either multiple comparators (to use the differing functionality) or awkward testing of subtype/etc logic in one comparator - so it basically moves the complexity/awkwardness, it doesn't eliminate it. I've refactored such that the Policy now provides a Comparator as opposed to extending it, so there is now room for it to provide multiple comparators and handle subtypes if need be, but I think we should wait until we see that we must do that before doing so, as I don't believe we will end up needing to (but if we do, existing code should need little change, and implementing what you suggest should be essentially additive...) ...About inherit relationships between interfaces/classes... Policies will be composed to achieve combined capabilities yet the collection of SchedulableProcesses & as such must reside in one instance, so attempting to place that logic in the multiple-instances (where multiple policies are constructed, each containing a SchedulerProcess collection via inheritance) is a non starter. Before the latest refactor this was handled by having a single SchedulerComparatorOrderingPolicy with multiple Comparators, post the latest refactor the SchedulingOrder is a single concrete type containing the SchedulableProcess collection, and using one or more OrderingPolicies which possess the policy specific logic (including, but not limited to, comparator logic). As it happens, they do not appear to need to share logic, so there is no super class, just a shared interface implementation - were that to change we could certainly adjust the implementation, but there's no need to now, and it's an implementation not an api issue anyway. ...Regarding relationship between OrderingPolicy and comparator... There's no longer this duality, there are now just OrderingPolicies ...Regarding configuration... There's now just one type of configuration, for policy, and will handle "friendly policy labels". Only the fifo is available now, and generalized composition logic, it's in SchedulingOrder
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12709373/YARN-3318.47.patch
          against trunk revision ef591b1.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javac. The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

          org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.TestFairScheduler

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7217//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7217//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7217//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7217//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12709373/YARN-3318.47.patch against trunk revision ef591b1. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.TestFairScheduler Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7217//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7217//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7217//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7217//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          javac error looks bogus, existing error has simply moved
          findbugs looks bogus, class it's complaining about is static. uploading new version so see if it notices now
          TestFairScheduler passes on my box with the patch, and can't see any way it would be effected. Tests will rerun with new patch, so we'll see.

          Show
          cwelch Craig Welch added a comment - javac error looks bogus, existing error has simply moved findbugs looks bogus, class it's complaining about is static. uploading new version so see if it notices now TestFairScheduler passes on my box with the patch, and can't see any way it would be effected. Tests will rerun with new patch, so we'll see.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12709391/YARN-3318.48.patch
          against trunk revision ef591b1.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javac. The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

          org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7218//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7218//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7218//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7218//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12709391/YARN-3318.48.patch against trunk revision ef591b1. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The applied patch generated 1149 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7218//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7218//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7218//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7218//console This message is automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Hi Craig,
          Thanks for updating, some comments:

          1) Regarding OrderingPolicy and SchedulingOrder responsibilities:
          If I understand correctly, SchedulingOrder plays a bridge role between scheduler and policy, but do we really need this additional layer? Such layer is helpful when we need support different OrderingPolicy interface, since we're defining new interfaces, I don't think that will be a target.
          Looking at methods of OrderingPolicy, most of them are just pass through parameters to OrderingPolicy, and rest of them are instantiation OrderingPolicies.

          So I suggest to remove the SchedulingOrder class to reduce complexity, and create a OrderingPolicyFactory class to encapsulate all instantiation methods.

          2) OrderingPolicy should be a per-queue instance or global library?
          I think it's better to make Policy per queue since it needs some internal fields which are different between each queues. SchedulingOrder class is to store per-queue states, I think we can merge them to OrderingPolicy.

          3) Suggestion about OrderingPolicy interface design (if you agree with 1/2):

          • reorderForContainerAllocation is not needed, instead, it's better to call it inform/notifyContainerAllocated(RMContainer r). You don't need to keep SchedulableProcess, it's unclear that if the "Schedulable" is from app/user/leaf-queue. OrderingPolicy will figure it out.
          • As same as reorderForContainerRelease
          • SchedulingComparator is actually implementation detail, the usaful thing is getAssignmentIterator/getPreemptionIterator.
          • add/remove/addAllSchedulableProcess should be a part of OrderingPolicy

          4) CompoundOrderingPolicy is implemenation detail for FairOrderingPolicy, don't need put in the patch.

          5) SchedulableProcess:

          • "Process" is easily to be confused with "process in the system", I prefer SchedulableEntity even if "Entity" and "Process" are all overloaded words, at least "Entity" is not so confusing to me
          • About spliting SchedulableProcess to App and Queue, I think we need consider queue's requirements since we're defining new interfaces. FairScheduler has property to say a "depth" of a policy to avoid mis-applying queue/app's policy, which I don't like. Separating App/Queue is not equals to "instanceof" need add everywhere. Just check once when adding them is enough.
          • As I mentioned before, use ResourceUsage is much better, it can avoid defining too much methods like getDemand, etc. and respect node-labels. SchedulerApplicationAttempt and all CSQueues has ResourceUsage. We should make node-label as first-class citizen. Your concern about ResourceUsage is tracked by YARN-3440.
          Show
          leftnoteasy Wangda Tan added a comment - Hi Craig, Thanks for updating, some comments: 1) Regarding OrderingPolicy and SchedulingOrder responsibilities: If I understand correctly, SchedulingOrder plays a bridge role between scheduler and policy, but do we really need this additional layer? Such layer is helpful when we need support different OrderingPolicy interface, since we're defining new interfaces, I don't think that will be a target. Looking at methods of OrderingPolicy, most of them are just pass through parameters to OrderingPolicy, and rest of them are instantiation OrderingPolicies. So I suggest to remove the SchedulingOrder class to reduce complexity, and create a OrderingPolicyFactory class to encapsulate all instantiation methods. 2) OrderingPolicy should be a per-queue instance or global library? I think it's better to make Policy per queue since it needs some internal fields which are different between each queues. SchedulingOrder class is to store per-queue states, I think we can merge them to OrderingPolicy. 3) Suggestion about OrderingPolicy interface design (if you agree with 1/2): reorderForContainerAllocation is not needed, instead, it's better to call it inform/notifyContainerAllocated(RMContainer r). You don't need to keep SchedulableProcess, it's unclear that if the "Schedulable" is from app/user/leaf-queue. OrderingPolicy will figure it out. As same as reorderForContainerRelease SchedulingComparator is actually implementation detail, the usaful thing is getAssignmentIterator/getPreemptionIterator. add/remove/addAllSchedulableProcess should be a part of OrderingPolicy 4) CompoundOrderingPolicy is implemenation detail for FairOrderingPolicy, don't need put in the patch. 5) SchedulableProcess: "Process" is easily to be confused with "process in the system", I prefer SchedulableEntity even if "Entity" and "Process" are all overloaded words, at least "Entity" is not so confusing to me About spliting SchedulableProcess to App and Queue, I think we need consider queue's requirements since we're defining new interfaces. FairScheduler has property to say a "depth" of a policy to avoid mis-applying queue/app's policy, which I don't like. Separating App/Queue is not equals to "instanceof" need add everywhere. Just check once when adding them is enough. As I mentioned before, use ResourceUsage is much better, it can avoid defining too much methods like getDemand, etc. and respect node-labels. SchedulerApplicationAttempt and all CSQueues has ResourceUsage . We should make node-label as first-class citizen. Your concern about ResourceUsage is tracked by YARN-3440 .
          Hide
          cwelch Craig Welch added a comment -

          1) Regarding OrderingPolicy and SchedulingOrder responsibilities:

          Scheduling order has multiple purposes, including:

          1. Housing supporting code for using policies common across schedulers, e.g. a common implementation of behavior
          2. Allowing for the composition of multiple policies together to accomplish desired queue behavior - it is awkward to factor the functionality in SchedulingOrder down into the policies, as multiple policies are in play for one instance of the logic in SchedulingOrder

          Although I mentioned that it could be made abstract some day if needed, that's not it's current purpose, the above are.

          ...Looking at methods of OrderingPolicy, most of them are just pass through parameters to OrderingPolicy, and rest of them are instantiation OrderingPolicies...

          Well, no, it has quite a lot of implementation logic around managing the SchedulerProcess collection and the interactions between it and multiple policies, it is certainly not limited to Factory operations

          OrderingPolicy should be a per-queue instance or global library

          OrderingPolicies are per-queue and stateful in terms of configuration specific to that queue configuration. For the reasons mentioned above regarding the composition of policies, they do not (and should not) maintain queue state (scheduler processes, etc).

          Suggestion about OrderingPolicy interface design (if you agree with 1/2):

          I don't agree, so skipping the section. The essential thing that I think is being missed here is that there is an intentional desire to compose ordering policies for a queue to achieve behavior - so priority + fifo, or fair + fifo, etc, and for that reason it is not appropriate to place the management of the collection of processes shared amongst policies into the policy implementation - it belongs outside, as it is today, in SchedulingOrder. Mixing these together defeats composition and also mixes concerns, making the code more (not less) complex and certainly less clean in terms of separation of concern and overall design and flow.

          ...CompoundOrderingPolicy is implemenation detail for FairOrderingPolicy, don't need put in the patch...

          Not is isn't, it's a feature of the generalized framework to support multiple policies being composed for a queue, it's not specific to fairness at all (fairness may be the first user, but so might priority - in any case, any set of policies may use it, it's not specific to any one of them, and therefore is framework...)

          ...About spliting SchedulableProcess to App and Queue...

          I stand by my earlier explanation (and don't see anything here which alters it...), I anticipate that with the current factoring of SchedulerProcess we won't have to subtype it to support Queues. That said, the right time to do that is when we are adding such support, "anticipatory complexity" is the worst kind. It is factored such that adding the subtyping should be additive if it needs to happen, so there is no need to anticipate it now (the room is there to add it, which is all we need. We should wait to add it until we know we need it).

          ...As I mentioned before, use ResourceUsage is much better...

          As I mentioned before, it doesn't presently supply the needed functionality, when it does we can convert to it.

          Show
          cwelch Craig Welch added a comment - 1) Regarding OrderingPolicy and SchedulingOrder responsibilities: Scheduling order has multiple purposes, including: 1. Housing supporting code for using policies common across schedulers, e.g. a common implementation of behavior 2. Allowing for the composition of multiple policies together to accomplish desired queue behavior - it is awkward to factor the functionality in SchedulingOrder down into the policies, as multiple policies are in play for one instance of the logic in SchedulingOrder Although I mentioned that it could be made abstract some day if needed, that's not it's current purpose, the above are. ...Looking at methods of OrderingPolicy, most of them are just pass through parameters to OrderingPolicy, and rest of them are instantiation OrderingPolicies... Well, no, it has quite a lot of implementation logic around managing the SchedulerProcess collection and the interactions between it and multiple policies, it is certainly not limited to Factory operations OrderingPolicy should be a per-queue instance or global library OrderingPolicies are per-queue and stateful in terms of configuration specific to that queue configuration. For the reasons mentioned above regarding the composition of policies, they do not (and should not) maintain queue state (scheduler processes, etc). Suggestion about OrderingPolicy interface design (if you agree with 1/2): I don't agree, so skipping the section. The essential thing that I think is being missed here is that there is an intentional desire to compose ordering policies for a queue to achieve behavior - so priority + fifo, or fair + fifo, etc, and for that reason it is not appropriate to place the management of the collection of processes shared amongst policies into the policy implementation - it belongs outside, as it is today, in SchedulingOrder. Mixing these together defeats composition and also mixes concerns, making the code more (not less) complex and certainly less clean in terms of separation of concern and overall design and flow. ...CompoundOrderingPolicy is implemenation detail for FairOrderingPolicy, don't need put in the patch... Not is isn't, it's a feature of the generalized framework to support multiple policies being composed for a queue, it's not specific to fairness at all (fairness may be the first user, but so might priority - in any case, any set of policies may use it, it's not specific to any one of them, and therefore is framework...) ...About spliting SchedulableProcess to App and Queue... I stand by my earlier explanation (and don't see anything here which alters it...), I anticipate that with the current factoring of SchedulerProcess we won't have to subtype it to support Queues. That said, the right time to do that is when we are adding such support, "anticipatory complexity" is the worst kind. It is factored such that adding the subtyping should be additive if it needs to happen, so there is no need to anticipate it now (the room is there to add it, which is all we need. We should wait to add it until we know we need it). ...As I mentioned before, use ResourceUsage is much better... As I mentioned before, it doesn't presently supply the needed functionality, when it does we can convert to it.
          Hide
          cwelch Craig Welch added a comment -

          ...Do we really see non-comparator based ordering-policy. We are unnecessarily adding two abstractions - adding policies and comparators...

          In the context of the code so far, the "comparator based" approach is specific to compounding comparators to achieve functionality (priority + fifo, fair + fifo, etc). This was the initial motivation for the two level api & configuration, the broader surface of the policy which would allow for different collection types, sorting on demand, etc, (the original policy) and the narrower one within that (comparator) for the cases where comparator logic was sufficient, e.g. sharing a collection (for composition) and a collection type (a tree, for efficient resorting of individual elements when required) was possible.

          The two level api & configuration was not well received. Offline Wangda has indicated that he thinks there are policies coming up which will need the wider, initial api, with control over the collection, sorting, etc. Supporting policy composition for those cases would be very awkward & is not really worth pursuing.

          The various competing tradeoffs, the aversion to a multilevel api, the need for the higher level api, and the ability to compose policies creates something of a tension, I don't think it's realistic to try and accomplish it all together, the result will be Frankensteinian at best. Something has to go. Originally, I chose the multilevel api to resolve the dilemma, I like that choice, it seems unpopular with the crowd. Given that, the other "optional" dynamic is the ability to compose policies (there's no requirement for either of these as far as I can tell, it is a "bonus feature"). While I like the composition approach, it can't be maintained as such with the broader api and without the multilevel config/api. As one of these has to go and it appears it can't be the broader api or the multilevel api I suppose it will have to be composition. Internally there can be some composition of course, but it won't be transparent/exposed/configurable as it was initially.

          I'll put out a patch with that in a bit.

          Show
          cwelch Craig Welch added a comment - ...Do we really see non-comparator based ordering-policy. We are unnecessarily adding two abstractions - adding policies and comparators... In the context of the code so far, the "comparator based" approach is specific to compounding comparators to achieve functionality (priority + fifo, fair + fifo, etc). This was the initial motivation for the two level api & configuration, the broader surface of the policy which would allow for different collection types, sorting on demand, etc, (the original policy) and the narrower one within that (comparator) for the cases where comparator logic was sufficient, e.g. sharing a collection (for composition) and a collection type (a tree, for efficient resorting of individual elements when required) was possible. The two level api & configuration was not well received. Offline Wangda has indicated that he thinks there are policies coming up which will need the wider, initial api, with control over the collection, sorting, etc. Supporting policy composition for those cases would be very awkward & is not really worth pursuing. The various competing tradeoffs, the aversion to a multilevel api, the need for the higher level api, and the ability to compose policies creates something of a tension, I don't think it's realistic to try and accomplish it all together, the result will be Frankensteinian at best. Something has to go. Originally, I chose the multilevel api to resolve the dilemma, I like that choice, it seems unpopular with the crowd. Given that, the other "optional" dynamic is the ability to compose policies (there's no requirement for either of these as far as I can tell, it is a "bonus feature"). While I like the composition approach, it can't be maintained as such with the broader api and without the multilevel config/api. As one of these has to go and it appears it can't be the broader api or the multilevel api I suppose it will have to be composition. Internally there can be some composition of course, but it won't be transparent/exposed/configurable as it was initially. I'll put out a patch with that in a bit.
          Hide
          cwelch Craig Welch added a comment -

          Update, removing composition in favor of broader interface

          Show
          cwelch Craig Welch added a comment - Update, removing composition in favor of broader interface
          Hide
          leftnoteasy Wangda Tan added a comment -

          Hi Craig,
          Thanks for updating, the latest patch generally LGTM, several minor comments:

          • Can you put some comments explain why CompoundComparator is needed here, since nobody is using this class
          • It's better to merge getSchedulingConsumption/getSchedulingDemand to getResourceUsage, and returns ResourceUsage class, which contains by-label statistics. Or explicitly call it getCachedResourceUsage?
          • If you agree above, rename updateSchedulingState to updateResourceUsage/updateCachedResourceUsage?
          • Add a getName to SchedulableEntity, this will be usaful when doing logging, etc.
          • compareInputOrderTo and compareSubmissionOrderTo, which one is better in your mind?
          Show
          leftnoteasy Wangda Tan added a comment - Hi Craig, Thanks for updating, the latest patch generally LGTM, several minor comments: Can you put some comments explain why CompoundComparator is needed here, since nobody is using this class It's better to merge getSchedulingConsumption/getSchedulingDemand to getResourceUsage, and returns ResourceUsage class, which contains by-label statistics. Or explicitly call it getCachedResourceUsage? If you agree above, rename updateSchedulingState to updateResourceUsage/updateCachedResourceUsage? Add a getName to SchedulableEntity, this will be usaful when doing logging, etc. compareInputOrderTo and compareSubmissionOrderTo, which one is better in your mind?
          Hide
          leftnoteasy Wangda Tan added a comment -

          In addition, if you plan to use ResourceUsage, ResourceUsage need have implementation to support snapshotting, set callers will only take effect after ResourceUsage.updateCache called, which need to be addressed in a separated JIRA.

          Show
          leftnoteasy Wangda Tan added a comment - In addition, if you plan to use ResourceUsage, ResourceUsage need have implementation to support snapshotting, set callers will only take effect after ResourceUsage.updateCache called, which need to be addressed in a separated JIRA.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12724329/YARN-3318.52.patch
          against trunk revision 623fd46.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestSetrepIncreasing
          org.apache.hadoop.hdfs.TestLease
          org.apache.hadoop.hdfs.TestRenameWhileOpen

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7281//testReport/
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7281//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724329/YARN-3318.52.patch against trunk revision 623fd46. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestSetrepIncreasing org.apache.hadoop.hdfs.TestLease org.apache.hadoop.hdfs.TestRenameWhileOpen Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7281//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7281//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12724398/YARN-3318.53.patch
          against trunk revision af9d4fe.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javac. The applied patch generated 1151 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7290//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7290//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7290//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7290//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724398/YARN-3318.53.patch against trunk revision af9d4fe. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The applied patch generated 1151 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7290//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7290//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7290//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7290//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          added comments to compoundcomparator, re-introduced getId (in lieu of getName), switched to ResourceUsage - to avoid unnecessary dependency on YARN-3361 SchedularApplicationAttempt manages pending in a way it won't have to long term, this doesn't effect the api and allows these to be committed in any order. Sticking with "Scheduling" instead of "Cached" as suggested earlier by Vinod Kumar Vavilapalli to keep it's purpose clear (Cached is too general) and because it can't be used as a generalized "cache" of the values, the lifecycle is tied to use by OrderingPolicies.

          Show
          cwelch Craig Welch added a comment - added comments to compoundcomparator, re-introduced getId (in lieu of getName), switched to ResourceUsage - to avoid unnecessary dependency on YARN-3361 SchedularApplicationAttempt manages pending in a way it won't have to long term, this doesn't effect the api and allows these to be committed in any order. Sticking with "Scheduling" instead of "Cached" as suggested earlier by Vinod Kumar Vavilapalli to keep it's purpose clear (Cached is too general) and because it can't be used as a generalized "cache" of the values, the lifecycle is tied to use by OrderingPolicies.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12724882/YARN-3318.56.patch
          against trunk revision f8f5887.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javac. The applied patch generated 1151 javac compiler warnings (more than the trunk's current 1148 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7313//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7313//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7313//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7313//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12724882/YARN-3318.56.patch against trunk revision f8f5887. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The applied patch generated 1151 javac compiler warnings (more than the trunk's current 1148 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7313//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7313//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7313//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7313//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          Looking again at using ResourceUsage instead of the initial use of application demand and consumption, while it may be preferable for future cases like queues with node label aware policies, there are deficiencies which need to be addressed to use it for the initial case, and it makes it more complex to do so. In fact, for the initial case, this approach is inferior.

          ResourceUsage is still a bit rough and incomplete, get does not properly handle the ANY/ALL case, which is what we need for application fairness - otherwise, applications whose resource requests are labeled something other than NO_LABEL will be erroneously preferred for scheduling in the fair case. The prior approach was working with full consumption and demand and did not have this issue and did not require additional change to support fairness properly.

          Even supporting ANY/ALL in ResourceUsage is a little tricky, as I see no reason why someone could not set values on ResourceUsage using the ANY label definition, and then there is a question as to what is the proper behavior for an ANY get request - should it sum all the values for all labels (which is, in some sense, correct), or just return the previously set ANY value? Should we disallow setting ANY? (that seems a bit arbitrary...) My suggestion is that we introduce explicit getAll(Used, Pending, etc), (not an ALL CommonNodeLabelsManager constant, I think this just moves/replicates the existing problem). There would be no corresponding setAll. getAll(XYZ) would iterate all labels in ResourceUsage for the passed ResourceType and return a total.

          For OrderingPolicy, the values should be cached on ResourceUsage instead of in SchedulableEntity for cases where that is needed - cloning an entire ResourceUsage will be expensive, inefficient, and unnecessary. We could add a separate cache collection in ResourceUsage, but I think it would actually be better to add values to the ResourceType enum, SCHEDULING_USED, SCHEDULING_PENDING

          When updating the cached value for Used, OrderingPolicy would then call getAllUsed() on ResourceUsage and set the resulting value with set (ANY node label expression, SCHEDULING_USED ResourceType), and for demand, getAllPending() and then set ANY node label expression, SCHEDULING_PENDING

          When getting the cached value, OrderingPolicy would call getUsed(ANY nlexpression, SCHEDULING_USED ResourceType) and for pending, getPending(ANY, SCHEDULING_PENDING)

          I'm inclined to roll forward with using ResourceUsage despite this additional scope to ease future usecases, but we need to be very careful about continuing to pull in additional change and complexity which is not required right now, and should avoid doing so again this iteration. It's good to aim for a stable api, but it's also good to complete the initial functionality, and to realize it's not possible to anticipate all future needs / highly likely there will be some change to api's like this as the system evolves.

          Show
          cwelch Craig Welch added a comment - Looking again at using ResourceUsage instead of the initial use of application demand and consumption, while it may be preferable for future cases like queues with node label aware policies, there are deficiencies which need to be addressed to use it for the initial case, and it makes it more complex to do so. In fact, for the initial case, this approach is inferior. ResourceUsage is still a bit rough and incomplete, get does not properly handle the ANY/ALL case, which is what we need for application fairness - otherwise, applications whose resource requests are labeled something other than NO_LABEL will be erroneously preferred for scheduling in the fair case. The prior approach was working with full consumption and demand and did not have this issue and did not require additional change to support fairness properly. Even supporting ANY/ALL in ResourceUsage is a little tricky, as I see no reason why someone could not set values on ResourceUsage using the ANY label definition, and then there is a question as to what is the proper behavior for an ANY get request - should it sum all the values for all labels (which is, in some sense, correct), or just return the previously set ANY value? Should we disallow setting ANY? (that seems a bit arbitrary...) My suggestion is that we introduce explicit getAll(Used, Pending, etc), (not an ALL CommonNodeLabelsManager constant, I think this just moves/replicates the existing problem). There would be no corresponding setAll. getAll(XYZ) would iterate all labels in ResourceUsage for the passed ResourceType and return a total. For OrderingPolicy, the values should be cached on ResourceUsage instead of in SchedulableEntity for cases where that is needed - cloning an entire ResourceUsage will be expensive, inefficient, and unnecessary. We could add a separate cache collection in ResourceUsage, but I think it would actually be better to add values to the ResourceType enum, SCHEDULING_USED, SCHEDULING_PENDING When updating the cached value for Used, OrderingPolicy would then call getAllUsed() on ResourceUsage and set the resulting value with set (ANY node label expression, SCHEDULING_USED ResourceType), and for demand, getAllPending() and then set ANY node label expression, SCHEDULING_PENDING When getting the cached value, OrderingPolicy would call getUsed(ANY nlexpression, SCHEDULING_USED ResourceType) and for pending, getPending(ANY, SCHEDULING_PENDING) I'm inclined to roll forward with using ResourceUsage despite this additional scope to ease future usecases, but we need to be very careful about continuing to pull in additional change and complexity which is not required right now, and should avoid doing so again this iteration. It's good to aim for a stable api, but it's also good to complete the initial functionality, and to realize it's not possible to anticipate all future needs / highly likely there will be some change to api's like this as the system evolves.
          Hide
          cwelch Craig Welch added a comment -

          Better ResourceUsage usage

          Show
          cwelch Craig Welch added a comment - Better ResourceUsage usage
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12725075/YARN-3318.57.patch
          against trunk revision a1afbc4.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javac. The patch appears to cause the build to fail.

          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7320//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725075/YARN-3318.57.patch against trunk revision a1afbc4. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The patch appears to cause the build to fail. Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7320//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          Missed attaching ResourceUsage

          Show
          cwelch Craig Welch added a comment - Missed attaching ResourceUsage
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12725084/YARN-3318.58.patch
          against trunk revision a1afbc4.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javac. The applied patch generated 1150 javac compiler warnings (more than the trunk's current 1147 warnings).

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7321//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7321//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7321//artifact/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7321//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725084/YARN-3318.58.patch against trunk revision a1afbc4. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. -1 javac . The applied patch generated 1150 javac compiler warnings (more than the trunk's current 1147 warnings). +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7321//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7321//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Javac warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7321//artifact/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7321//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          checkpatch fixes

          Show
          cwelch Craig Welch added a comment - checkpatch fixes
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12725121/YARN-3318.59.patch
          against trunk revision 838b06a.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:

          org.apache.hadoop.yarn.server.resourcemanager.TestClientRMService

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7322//testReport/
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7322//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725121/YARN-3318.59.patch against trunk revision 838b06a. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: org.apache.hadoop.yarn.server.resourcemanager.TestClientRMService Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7322//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7322//console This message is automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Beyond SchedulerApplicationAttempt which is pending YARN-3361, Few comments on latest patch:

          1) CACHED_USED/CACHED_PENDING don't used by anybody, are they pending YARN-3361 as well?
          2) AbstractComparatorOrderingPolicy doesn't handle locks, I suggest to add synchronized lock to all methods if you think it will only be used in single-thread scenario
          3) FifoComparator, it will be used by FairOrderingPolicy as well? If so, better to make it to a separated class
          4) How about call getInfo to getStatusMessage, since the "info" is too generic. And add a comment to indicate it will be used for logger printing.
          5) getComparator of AbstractComparatorOrderingPolicy is @VisibleForTest?

          Show
          leftnoteasy Wangda Tan added a comment - Beyond SchedulerApplicationAttempt which is pending YARN-3361 , Few comments on latest patch: 1) CACHED_USED/CACHED_PENDING don't used by anybody, are they pending YARN-3361 as well? 2) AbstractComparatorOrderingPolicy doesn't handle locks, I suggest to add synchronized lock to all methods if you think it will only be used in single-thread scenario 3) FifoComparator, it will be used by FairOrderingPolicy as well? If so, better to make it to a separated class 4) How about call getInfo to getStatusMessage , since the "info" is too generic. And add a comment to indicate it will be used for logger printing. 5) getComparator of AbstractComparatorOrderingPolicy is @VisibleForTest?
          Hide
          cwelch Craig Welch added a comment -

          Beyond SchedulerApplicationAttempt which is pending YARN-3361, Few comments on latest patch:

          I think you misunderstood, the patch doesn't depend on 3361, but after 3361 is in some things should be removed from this patch. In any case, I decided that it really belonged in the integration patch, YARN-3463, so I've dropped it from here and it will be committed there

          1) CACHED_USED/CACHED_PENDING don't used by anybody, are they pending YARN-3361 as well?

          No, that was a miss during the ResourceUsage usage changes! Something which could affect functionality! Amazing, fixed.

          2) AbstractComparatorOrderingPolicy doesn't handle locks, I suggest to add synchronized lock to all methods if you think it will only be used in single-thread scenario

          Since the api returns iterators which must be externally synchronized, OrderingPolicy makes it clear in documentation that the burden for synchronization rests with the user (the schedulers). That's the threading model, so synchronizing here would be pointless

          3) FifoComparator, it will be used by FairOrderingPolicy as well? If so, better to make it to a separated class

          sure, done

          4) How about call getInfo to getStatusMessage, since the "info" is too generic. And add a comment to indicate it will be used for logger printing.

          sure, done

          5) getComparator of AbstractComparatorOrderingPolicy is @VisibleForTest?

          sure, done

          Show
          cwelch Craig Welch added a comment - Beyond SchedulerApplicationAttempt which is pending YARN-3361 , Few comments on latest patch: I think you misunderstood, the patch doesn't depend on 3361, but after 3361 is in some things should be removed from this patch. In any case, I decided that it really belonged in the integration patch, YARN-3463 , so I've dropped it from here and it will be committed there 1) CACHED_USED/CACHED_PENDING don't used by anybody, are they pending YARN-3361 as well? No, that was a miss during the ResourceUsage usage changes! Something which could affect functionality! Amazing, fixed. 2) AbstractComparatorOrderingPolicy doesn't handle locks, I suggest to add synchronized lock to all methods if you think it will only be used in single-thread scenario Since the api returns iterators which must be externally synchronized, OrderingPolicy makes it clear in documentation that the burden for synchronization rests with the user (the schedulers). That's the threading model, so synchronizing here would be pointless 3) FifoComparator, it will be used by FairOrderingPolicy as well? If so, better to make it to a separated class sure, done 4) How about call getInfo to getStatusMessage, since the "info" is too generic. And add a comment to indicate it will be used for logger printing. sure, done 5) getComparator of AbstractComparatorOrderingPolicy is @VisibleForTest? sure, done
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12725311/YARN-3318.60.patch
          against trunk revision 0fefda6.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7331//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7331//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7331//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725311/YARN-3318.60.patch against trunk revision 0fefda6. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7331//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/7331//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7331//console This message is automatically generated.
          Hide
          cwelch Craig Welch added a comment -

          fix findbugs recurrance due to class name change

          Show
          cwelch Craig Welch added a comment - fix findbugs recurrance due to class name change
          Hide
          leftnoteasy Wangda Tan added a comment -

          Patch LGTM, +1. Will commit it by end of today if no opposite opinions.

          Show
          leftnoteasy Wangda Tan added a comment - Patch LGTM, +1. Will commit it by end of today if no opposite opinions.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12725371/YARN-3318.61.patch
          against trunk revision 05007b4.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7332//testReport/
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7332//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725371/YARN-3318.61.patch against trunk revision 05007b4. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/7332//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/7332//console This message is automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Committed to trunk/branch-2, thanks Craig Welch and review from Vinod Kumar Vavilapalli.

          Show
          leftnoteasy Wangda Tan added a comment - Committed to trunk/branch-2, thanks Craig Welch and review from Vinod Kumar Vavilapalli .
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #7588 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7588/)
          YARN-3318. Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7588 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7588/ ) YARN-3318 . Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #2097 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2097/)
          YARN-3318. Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2097 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2097/ ) YARN-3318 . Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #156 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/156/)
          YARN-3318. Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          • hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #156 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/156/ ) YARN-3318 . Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #165 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/165/)
          YARN-3318. Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #165 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/165/ ) YARN-3318 . Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #899 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/899/)
          YARN-3318. Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e)

          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #899 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/899/ ) YARN-3318 . Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e) hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #166 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/166/)
          YARN-3318. Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          • hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #166 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/166/ ) YARN-3318 . Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2115 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2115/)
          YARN-3318. Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java
          • hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2115 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2115/ ) YARN-3318 . Create Initial OrderingPolicy Framework and FifoOrderingPolicy. (Craig Welch via wangda) (wangda: rev 5004e753322084e42dfda4be1d2db66677f86a1e) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/TestFifoOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ResourceUsage.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/AbstractComparatorOrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/SchedulableEntity.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoComparator.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/MockSchedulableEntity.java hadoop-yarn-project/hadoop-yarn/dev-support/findbugs-exclude.xml hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/OrderingPolicy.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/policy/FifoOrderingPolicy.java
          Hide
          jhung Jonathan Hung added a comment -

          Hi Tan, Wangda, any chance we can get this committed to branch-2.7? This contains some of the code needed to support application priorities in the fifo scheduler (we are planning to port some of the patches from YARN-1963 to branch-2.7). Thanks!

          Show
          jhung Jonathan Hung added a comment - Hi Tan, Wangda , any chance we can get this committed to branch-2.7? This contains some of the code needed to support application priorities in the fifo scheduler (we are planning to port some of the patches from YARN-1963 to branch-2.7). Thanks!
          Hide
          leftnoteasy Wangda Tan added a comment -

          Hi Jonathan Hung, typically we don't put new features to branch-2.7, this is clearly a new feature instead of enhancement to me. And since 2.8.0 release is happening, it won't be far away any more!

          Show
          leftnoteasy Wangda Tan added a comment - Hi Jonathan Hung , typically we don't put new features to branch-2.7, this is clearly a new feature instead of enhancement to me. And since 2.8.0 release is happening, it won't be far away any more!

            People

            • Assignee:
              cwelch Craig Welch
              Reporter:
              cwelch Craig Welch
            • Votes:
              0 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development