Uploaded image for project: 'Hadoop YARN'
  1. Hadoop YARN
  2. YARN-3926 Extend the YARN resource model for easier resource-type management and profiles
  3. YARN-6610

DominantResourceCalculator#getResourceAsValue dominant param is updated to handle multiple resources

    Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: YARN-3926
    • Fix Version/s: 3.1.0
    • Component/s: resourcemanager
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The dominant param assumes there are only two resources, i.e. true means to compare the dominant, and false means to compare the subordinate. Now that there are n resources, this parameter no longer makes sense.

      1. YARN-6610.YARN-3926.perf-wangda-002.patch
        30 kB
        Wangda Tan
      2. YARN-6610.YARN-3926.perf-wangda-001.patch
        31 kB
        Wangda Tan
      3. YARN-6610.YARN-3926.perf-rebased.patch
        27 kB
        Wangda Tan
      4. YARN-6610.YARN-3926.perf.patch
        27 kB
        Daniel Templeton
      5. YARN-6610.YARN-3926.007.patch
        30 kB
        Sunil G
      6. YARN-6610.YARN-3926.006-rebased.patch
        25 kB
        Wangda Tan
      7. YARN-6610.YARN-3926.006.patch
        25 kB
        Daniel Templeton
      8. YARN-6610.YARN-3926.005.patch
        11 kB
        Daniel Templeton
      9. YARN-6610.YARN-3926.004.patch
        9 kB
        Daniel Templeton
      10. YARN-6610.YARN-3926.003.patch
        10 kB
        Daniel Templeton
      11. YARN-6610.YARN-3926.002.patch
        10 kB
        Daniel Templeton
      12. YARN-6610.001.patch
        6 kB
        Daniel Templeton

        Issue Links

          Activity

          Hide
          templedf Daniel Templeton added a comment -

          Here's a first pass at making the compare make sense again. Thoughts? The patch still needs tests.

          Show
          templedf Daniel Templeton added a comment - Here's a first pass at making the compare make sense again. Thoughts? The patch still needs tests.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 patch 0m 7s YARN-6610 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



          Subsystem Report/Notes
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868398/YARN-6610.001.patch
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/15942/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 7s YARN-6610 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12868398/YARN-6610.001.patch Console output https://builds.apache.org/job/PreCommit-YARN-Build/15942/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          sunilg Sunil G added a comment -

          Converted as a sub-task under YANR-3926, Please revert if any issues.

          Show
          sunilg Sunil G added a comment - Converted as a sub-task under YANR-3926, Please revert if any issues.
          Hide
          sunilg Sunil G added a comment -

          Daniel Templeton, thanks for the patch.

          I have some more thoughts to add here. Earlier, we were considering only Memory and VCores for dominance comparison. Given dominant parameter, we were trying to see which resource has a larger share and return the comparison value as per that.

          In resource profile case, where more resource are considered, i do not think all resources has to be considered for larger share comparison. There could be "scarce" resources such as GPU. In a case where there are very less GPU machines in cluster, at some point of time, GPU can also become higher to VCores or Memory. But I do not think, such a scenario could be handled only by resource calculator easily. Constraints, and rich placement strategy set will also play a part to consume "scarce" resources.

          In general, "scarce" object could be labelled/pre-configured but also could be identified by YARN. cc/Varun Vasudev and Wangda Tan

          Show
          sunilg Sunil G added a comment - Daniel Templeton , thanks for the patch. I have some more thoughts to add here. Earlier, we were considering only Memory and VCores for dominance comparison. Given dominant parameter, we were trying to see which resource has a larger share and return the comparison value as per that. In resource profile case, where more resource are considered, i do not think all resources has to be considered for larger share comparison. There could be "scarce" resources such as GPU. In a case where there are very less GPU machines in cluster, at some point of time, GPU can also become higher to VCores or Memory. But I do not think, such a scenario could be handled only by resource calculator easily. Constraints, and rich placement strategy set will also play a part to consume "scarce" resources. In general, "scarce" object could be labelled/pre-configured but also could be identified by YARN. cc/ Varun Vasudev and Wangda Tan
          Hide
          templedf Daniel Templeton added a comment -

          I had similar thoughts. Maybe instead of adding scarce resources, we could cover most use cases by doing what I did in this patch plus adding a calculator that looks at only CPU and memory, like the pre-resource-types DominantResourceCalculator. (I guess the other way to look at it would be to have DominantResourceCalculator ignore everything except CPU and memory and add a new calculator to do what I did in the patch.)

          Show
          templedf Daniel Templeton added a comment - I had similar thoughts. Maybe instead of adding scarce resources, we could cover most use cases by doing what I did in this patch plus adding a calculator that looks at only CPU and memory, like the pre-resource-types DominantResourceCalculator . (I guess the other way to look at it would be to have DominantResourceCalculator ignore everything except CPU and memory and add a new calculator to do what I did in the patch.)
          Hide
          sunilg Sunil G added a comment -

          Thanks Daniel Templeton
          I think the approach taken in this patch in general sounds fine. However we iterate over resourceNames, which is full list of resources. I guess we can define Memory and CPU as resources to calculate for. I am not so sure whether its too early to expose as a config for user to express.

          Show
          sunilg Sunil G added a comment - Thanks Daniel Templeton I think the approach taken in this patch in general sounds fine. However we iterate over resourceNames , which is full list of resources. I guess we can define Memory and CPU as resources to calculate for. I am not so sure whether its too early to expose as a config for user to express.
          Hide
          templedf Daniel Templeton added a comment -

          After a long discussion with Karthik Kambatla, I think I'm now convinced that there's no correctness issue with using DRF in a multi-resource configuration. There is, of course, the performance issue created by increasing the number of dimensions in the resource comparisons, but that's an issue to be resolved for resource types in general. I think the posted patch should be fine.

          Show
          templedf Daniel Templeton added a comment - After a long discussion with Karthik Kambatla , I think I'm now convinced that there's no correctness issue with using DRF in a multi-resource configuration. There is, of course, the performance issue created by increasing the number of dimensions in the resource comparisons, but that's an issue to be resolved for resource types in general. I think the posted patch should be fine.
          Hide
          yufeigu Yufei Gu added a comment - - edited

          Hi Daniel Templeton, the patch doesn't apply to trunk. Can you rebase it? Seems like it can apply for branch YARN-3926. Just need to change the patch file name.

          Show
          yufeigu Yufei Gu added a comment - - edited Hi Daniel Templeton , the patch doesn't apply to trunk. Can you rebase it? Seems like it can apply for branch YARN-3926 . Just need to change the patch file name.
          Hide
          templedf Daniel Templeton added a comment -

          Sunil G, I don't think we can ignore everything except CPU and memory. Consider that scarce resources are typically going to be expensive. We want to make sure they have priority when scheduling, so we want them to be treated according to their dominant resource, not just CPU and memory.

          Show
          templedf Daniel Templeton added a comment - Sunil G , I don't think we can ignore everything except CPU and memory. Consider that scarce resources are typically going to be expensive. We want to make sure they have priority when scheduling, so we want them to be treated according to their dominant resource, not just CPU and memory.
          Hide
          yufeigu Yufei Gu added a comment -

          Thanks Daniel Templeton for working on this. Some thoughts:

          1. I like what you did in method getResourceAsDominantValue().
          2. The parameter "boolean singleType" is not honored in method compare(Resource clusterResource, Resource lhs, Resource rhs, boolean singleType).
          3. It may easily to go to this branch with multiple resources, say, only few nodes have one type of resource and they went down, which causes resource comparison less meaningful since compare(Resource lhs, Resource rhs) could easily return 0. I assume we could use the similar algorithm in compare(Resource clusterResource, Resource lhs, Resource rhs, boolean singleType).
                if (isInvalidDivisor(clusterResource)) {
                  return this.compare(lhs, rhs);
                }
            
          4. Do you mind add unit tests for compare(Resource clusterResource, Resource lhs, Resource rhs, boolean singleType)?
          5. extra space in line "resource. The share ..."
          Show
          yufeigu Yufei Gu added a comment - Thanks Daniel Templeton for working on this. Some thoughts: I like what you did in method getResourceAsDominantValue(). The parameter "boolean singleType" is not honored in method compare(Resource clusterResource, Resource lhs, Resource rhs, boolean singleType) . It may easily to go to this branch with multiple resources, say, only few nodes have one type of resource and they went down, which causes resource comparison less meaningful since compare(Resource lhs, Resource rhs) could easily return 0. I assume we could use the similar algorithm in compare(Resource clusterResource, Resource lhs, Resource rhs, boolean singleType) . if (isInvalidDivisor(clusterResource)) { return this .compare(lhs, rhs); } Do you mind add unit tests for compare(Resource clusterResource, Resource lhs, Resource rhs, boolean singleType) ? extra space in line "resource. The share ..."
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Daniel Templeton for working on the patch and comments from Sunil G/Yufei Gu.

          Tried to review the patch, it is already outdated, I will do a detailed review once the patch is updated. For one thing definitely need to be updated: Existing impl creates and uses TreeSet for every compare operation, which could be very slow according to testing of YARN-6788 (we expect another patch to completely remove map operations in the code path of Resource). I suggest to relook at the patch once we fill major performance gaps of YARN-3926 branch.

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Daniel Templeton for working on the patch and comments from Sunil G / Yufei Gu . Tried to review the patch, it is already outdated, I will do a detailed review once the patch is updated. For one thing definitely need to be updated: Existing impl creates and uses TreeSet for every compare operation, which could be very slow according to testing of YARN-6788 (we expect another patch to completely remove map operations in the code path of Resource). I suggest to relook at the patch once we fill major performance gaps of YARN-3926 branch.
          Hide
          templedf Daniel Templeton added a comment -

          Yeah, the TreeSet was just there until YARN-6788 gets in. Now that we're close, I'll put this one on hold.

          Show
          templedf Daniel Templeton added a comment - Yeah, the TreeSet was just there until YARN-6788 gets in. Now that we're close, I'll put this one on hold.
          Hide
          templedf Daniel Templeton added a comment -

          Now that YARN-6788 is in, here's a fresh patch that is significantly better optimized.

          Show
          templedf Daniel Templeton added a comment - Now that YARN-6788 is in, here's a fresh patch that is significantly better optimized.
          Hide
          templedf Daniel Templeton added a comment -

          Realized that I had the sorts backwards. New patch attached.

          Show
          templedf Daniel Templeton added a comment - Realized that I had the sorts backwards. New patch attached.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s 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.
                YARN-3926 Compile Tests
          +1 mvninstall 13m 48s YARN-3926 passed
          +1 compile 0m 29s YARN-3926 passed
          +1 checkstyle 0m 21s YARN-3926 passed
          +1 mvnsite 0m 32s YARN-3926 passed
          +1 findbugs 1m 1s YARN-3926 passed
          +1 javadoc 0m 29s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 28s the patch passed
          +1 compile 0m 28s the patch passed
          +1 javac 0m 28s the patch passed
          +1 checkstyle 0m 17s the patch passed
          +1 mvnsite 0m 32s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 4s the patch passed
          +1 javadoc 0m 25s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)
                Other Tests
          +1 unit 2m 21s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          24m 8s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881048/YARN-6610.YARN-3926.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux e4258d200d34 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / 1b586d7
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16807/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16807/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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.       YARN-3926 Compile Tests +1 mvninstall 13m 48s YARN-3926 passed +1 compile 0m 29s YARN-3926 passed +1 checkstyle 0m 21s YARN-3926 passed +1 mvnsite 0m 32s YARN-3926 passed +1 findbugs 1m 1s YARN-3926 passed +1 javadoc 0m 29s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 28s the patch passed +1 compile 0m 28s the patch passed +1 javac 0m 28s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 32s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 4s the patch passed +1 javadoc 0m 25s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)       Other Tests +1 unit 2m 21s hadoop-yarn-common in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 24m 8s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881048/YARN-6610.YARN-3926.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux e4258d200d34 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / 1b586d7 Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16807/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16807/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          Refactored a little to make it cleaner.

          Show
          templedf Daniel Templeton added a comment - Refactored a little to make it cleaner.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s 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.
                YARN-3926 Compile Tests
          +1 mvninstall 12m 54s YARN-3926 passed
          +1 compile 0m 28s YARN-3926 passed
          +1 checkstyle 0m 20s YARN-3926 passed
          +1 mvnsite 0m 30s YARN-3926 passed
          +1 findbugs 0m 55s YARN-3926 passed
          +1 javadoc 0m 27s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 26s the patch passed
          +1 compile 0m 24s the patch passed
          +1 javac 0m 24s the patch passed
          +1 checkstyle 0m 17s the patch passed
          +1 mvnsite 0m 27s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 59s the patch passed
          +1 javadoc 0m 24s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)
                Other Tests
          +1 unit 2m 31s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          23m 0s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881282/YARN-6610.YARN-3926.004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux ea7a45e04bb9 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / 1b586d7
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16828/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16828/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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.       YARN-3926 Compile Tests +1 mvninstall 12m 54s YARN-3926 passed +1 compile 0m 28s YARN-3926 passed +1 checkstyle 0m 20s YARN-3926 passed +1 mvnsite 0m 30s YARN-3926 passed +1 findbugs 0m 55s YARN-3926 passed +1 javadoc 0m 27s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 26s the patch passed +1 compile 0m 24s the patch passed +1 javac 0m 24s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 27s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 59s the patch passed +1 javadoc 0m 24s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)       Other Tests +1 unit 2m 31s hadoop-yarn-common in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 23m 0s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881282/YARN-6610.YARN-3926.004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ea7a45e04bb9 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / 1b586d7 Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16828/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16828/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          sunilg Sunil G added a comment -

          Thanks Daniel Templeton for the patch. Few comments,

          1. # In DRC#compare, below could be avoided
            137	      Arrays.sort(lhsShares);
            138	      Arrays.sort(rhsShares);
            

            We are now treating resources per is index and the index is fixed.

          2. In DRC#compare, there are chances of IndexOutOfBoundException etc. We need to ensure that such cases are handled correctly in compare and result in desired behavior. Do you feel RM should fail-fast in such conditions?
          3. In case when singleType is false, we are now looping two times over resource array. first time to calculate and then to compare. May be we could merge the same ?
          Show
          sunilg Sunil G added a comment - Thanks Daniel Templeton for the patch. Few comments, # In DRC#compare , below could be avoided 137 Arrays.sort(lhsShares); 138 Arrays.sort(rhsShares); We are now treating resources per is index and the index is fixed. In DRC#compare , there are chances of IndexOutOfBoundException etc. We need to ensure that such cases are handled correctly in compare and result in desired behavior. Do you feel RM should fail-fast in such conditions? In case when singleType is false, we are now looping two times over resource array. first time to calculate and then to compare. May be we could merge the same ?
          Hide
          yufeigu Yufei Gu added a comment -

          Thanks Daniel Templeton's patch and Sunil G's review. The patch looks much better. Some comments:

          1. Is it guaranteed that lhs and rhs get the same length? If not, we need to handle the ArrayIndexOutOfBoundsException nicely. It seems OK to ignore the exception.
          2. Still think unit tests for DominantResourceCalculator#compare() are worth to do. I've searched around callers and callers of callers. Didn't find good enough tests for this. Please let me know if they are there.
          3. Not sure how we can avoid this and merge two loops in Sunil G's comment #3. We could alway do further optimization, but it is premature without performance tests and detailed performance analysis.
            137	      Arrays.sort(lhsShares);
            138	      Arrays.sort(rhsShares);
            
          Show
          yufeigu Yufei Gu added a comment - Thanks Daniel Templeton 's patch and Sunil G 's review. The patch looks much better. Some comments: Is it guaranteed that lhs and rhs get the same length? If not, we need to handle the ArrayIndexOutOfBoundsException nicely. It seems OK to ignore the exception. Still think unit tests for DominantResourceCalculator#compare() are worth to do. I've searched around callers and callers of callers. Didn't find good enough tests for this. Please let me know if they are there. Not sure how we can avoid this and merge two loops in Sunil G 's comment #3. We could alway do further optimization, but it is premature without performance tests and detailed performance analysis. 137 Arrays.sort(lhsShares); 138 Arrays.sort(rhsShares);
          Hide
          templedf Daniel Templeton added a comment -

          Thanks for the review. I don't see how to avoid sorting the shares in DRC#compare(). We have to compare the two resources' shares in order of dominance, which requires that we order them by dominance. Instead of sorting we could hunt for the dominant share by going through the arrays, which would be faster than sorting only if the dominant shares aren't equal. Worst case is that all shares are equal, which makes that approach slower than sorting. It seems to me that it's too early to optimize this part.

          #2 is an interesting point. It shouldn't ever happen that resources disagree on how many resource types there are. If it happens, it's a critical issue that should take the RM down. We could put the whole thing in a try/catch to have the RM go down with a friendlier message than an ArrayIndexOutOfBoundsException, but there is a small cost to adding the try block, mostly that we limit some kinds of compiler optimizations. I think it's probably worth it, though.

          For #3, we can't do the comparison until we've sorted the shares arrays. Even if we were to try being clever about merging the sort into the calculation, we'd still need another pass to do the comparison.

          Show
          templedf Daniel Templeton added a comment - Thanks for the review. I don't see how to avoid sorting the shares in DRC#compare() . We have to compare the two resources' shares in order of dominance, which requires that we order them by dominance. Instead of sorting we could hunt for the dominant share by going through the arrays, which would be faster than sorting only if the dominant shares aren't equal. Worst case is that all shares are equal, which makes that approach slower than sorting. It seems to me that it's too early to optimize this part. #2 is an interesting point. It shouldn't ever happen that resources disagree on how many resource types there are. If it happens, it's a critical issue that should take the RM down. We could put the whole thing in a try/catch to have the RM go down with a friendlier message than an ArrayIndexOutOfBoundsException , but there is a small cost to adding the try block, mostly that we limit some kinds of compiler optimizations. I think it's probably worth it, though. For #3, we can't do the comparison until we've sorted the shares arrays. Even if we were to try being clever about merging the sort into the calculation, we'd still need another pass to do the comparison.
          Hide
          templedf Daniel Templeton added a comment -

          I added a handler for the exception.

          Show
          templedf Daniel Templeton added a comment - I added a handler for the exception.
          Hide
          templedf Daniel Templeton added a comment -

          Yufei Gu, yeah, you're right about unit tests. There is currently the TestResourceCalculator class, but it only tests compare() when the cluster resource is 0, and it does nothing to test more than 2 resources.

          Show
          templedf Daniel Templeton added a comment - Yufei Gu , yeah, you're right about unit tests. There is currently the TestResourceCalculator class, but it only tests compare() when the cluster resource is 0, and it does nothing to test more than 2 resources.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s 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.
                YARN-3926 Compile Tests
          +1 mvninstall 15m 19s YARN-3926 passed
          +1 compile 0m 33s YARN-3926 passed
          +1 checkstyle 0m 23s YARN-3926 passed
          +1 mvnsite 0m 36s YARN-3926 passed
          +1 findbugs 1m 8s YARN-3926 passed
          +1 javadoc 0m 36s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 38s the patch passed
          +1 compile 0m 36s the patch passed
          +1 javac 0m 36s the patch passed
          +1 checkstyle 0m 19s the patch passed
          +1 mvnsite 0m 37s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 14s the patch passed
          +1 javadoc 0m 28s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)
                Other Tests
          +1 unit 2m 34s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          27m 3s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881821/YARN-6610.YARN-3926.005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux fb6886c189b3 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / 8f80907
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16893/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16893/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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.       YARN-3926 Compile Tests +1 mvninstall 15m 19s YARN-3926 passed +1 compile 0m 33s YARN-3926 passed +1 checkstyle 0m 23s YARN-3926 passed +1 mvnsite 0m 36s YARN-3926 passed +1 findbugs 1m 8s YARN-3926 passed +1 javadoc 0m 36s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 38s the patch passed +1 compile 0m 36s the patch passed +1 javac 0m 36s the patch passed +1 checkstyle 0m 19s the patch passed +1 mvnsite 0m 37s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 14s the patch passed +1 javadoc 0m 28s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)       Other Tests +1 unit 2m 34s hadoop-yarn-common in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 27m 3s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881821/YARN-6610.YARN-3926.005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux fb6886c189b3 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / 8f80907 Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16893/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16893/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          sunilg Sunil G added a comment -

          Yes Daniel Templeton
          I am still not very sure. I suggested those optimizations if possible because I saw a small performance dip with this patch post YARN-6892. I am working on some concrete numbers to share. I ll share soon.

          Show
          sunilg Sunil G added a comment - Yes Daniel Templeton I am still not very sure. I suggested those optimizations if possible because I saw a small performance dip with this patch post YARN-6892 . I am working on some concrete numbers to share. I ll share soon.
          Hide
          templedf Daniel Templeton added a comment -

          I would expect a small performance dip, because the code is now doing the right thing rather than the inexpensive thing. Without this patch the code is fast but wrong. I'll keep thinking about a way to do this faster, but I'm not sure there's much more that can be squeezed out. Maybe a cheaper sort like radix would buy us a little. The real answer is to go through capacity scheduler and evaluate if all of the uses of ResourceCalculator (mostly via Resources) are correct. I just did that for fair scheduler, and there were many places where it was being misused.

          Show
          templedf Daniel Templeton added a comment - I would expect a small performance dip, because the code is now doing the right thing rather than the inexpensive thing. Without this patch the code is fast but wrong. I'll keep thinking about a way to do this faster, but I'm not sure there's much more that can be squeezed out. Maybe a cheaper sort like radix would buy us a little. The real answer is to go through capacity scheduler and evaluate if all of the uses of ResourceCalculator (mostly via Resources ) are correct. I just did that for fair scheduler, and there were many places where it was being misused.
          Hide
          templedf Daniel Templeton added a comment -

          Added unit tests and fixed an error.

          Show
          templedf Daniel Templeton added a comment - Added unit tests and fixed an error.
          Hide
          templedf Daniel Templeton added a comment -

          My previous comment about performance was assuming that you're testing with more than 2 resources. If you're testing with only 2 resources, then I'd be surprised to see much of a difference. If that's the case, I can take a closer look.

          Show
          templedf Daniel Templeton added a comment - My previous comment about performance was assuming that you're testing with more than 2 resources. If you're testing with only 2 resources, then I'd be surprised to see much of a difference. If that's the case, I can take a closer look.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                YARN-3926 Compile Tests
          +1 mvninstall 14m 58s YARN-3926 passed
          +1 compile 0m 30s YARN-3926 passed
          +1 checkstyle 0m 21s YARN-3926 passed
          +1 mvnsite 0m 33s YARN-3926 passed
          +1 findbugs 1m 0s YARN-3926 passed
          +1 javadoc 0m 30s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 29s the patch passed
          +1 compile 0m 28s the patch passed
          +1 javac 0m 28s the patch passed
          +1 checkstyle 0m 17s the patch passed
          +1 mvnsite 0m 31s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 12s the patch passed
          +1 javadoc 0m 28s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)
                Other Tests
          +1 unit 2m 29s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          25m 48s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882033/YARN-6610.YARN-3926.006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux bf843fd1c873 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / 8f80907
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16918/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16918/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       YARN-3926 Compile Tests +1 mvninstall 14m 58s YARN-3926 passed +1 compile 0m 30s YARN-3926 passed +1 checkstyle 0m 21s YARN-3926 passed +1 mvnsite 0m 33s YARN-3926 passed +1 findbugs 1m 0s YARN-3926 passed +1 javadoc 0m 30s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 29s the patch passed +1 compile 0m 28s the patch passed +1 javac 0m 28s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 31s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 12s the patch passed +1 javadoc 0m 28s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)       Other Tests +1 unit 2m 29s hadoop-yarn-common in the patch passed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 25m 48s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882033/YARN-6610.YARN-3926.006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux bf843fd1c873 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / 8f80907 Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16918/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16918/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Daniel Templeton,

          My previous comment about performance was assuming that you're testing with more than 2 resources. If you're testing with only 2 resources, then I'd be surprised to see much of a difference. If that's the case, I can take a closer look.

          Actually before YARN-6788, trunk is 2x faster than the branch (by using perf test added by YARN-6775) with 2 resources, the most expensive piece is Collection operations (such as Set look up) and unnecessary box/unboxing/instance-initialization. So I think we need to be more careful here, new feature is good we can optimize performance of the new feature slowly, however we should avoid any obvious performance regression for users which are not using the feature.

          Show
          leftnoteasy Wangda Tan added a comment - Daniel Templeton , My previous comment about performance was assuming that you're testing with more than 2 resources. If you're testing with only 2 resources, then I'd be surprised to see much of a difference. If that's the case, I can take a closer look. Actually before YARN-6788 , trunk is 2x faster than the branch (by using perf test added by YARN-6775 ) with 2 resources, the most expensive piece is Collection operations (such as Set look up) and unnecessary box/unboxing/instance-initialization. So I think we need to be more careful here, new feature is good we can optimize performance of the new feature slowly, however we should avoid any obvious performance regression for users which are not using the feature.
          Hide
          templedf Daniel Templeton added a comment -

          Looking at the diff, before this patch, the code does 2 or 4 passes through the resources. I suspect 4 is the common case. After this patch, the code does only 2 passes, but adds two sorts. If you're seeing a notable performance difference, I would suspect the sorts. If we want to optimize for the case of only CPU and memory, we can add another code path to DRC#compare() for it.

          Show
          templedf Daniel Templeton added a comment - Looking at the diff, before this patch, the code does 2 or 4 passes through the resources. I suspect 4 is the common case. After this patch, the code does only 2 passes, but adds two sorts. If you're seeing a notable performance difference, I would suspect the sorts. If we want to optimize for the case of only CPU and memory, we can add another code path to DRC#compare() for it.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Daniel Templeton, I just checked, in Java, Arrays.sort uses dual pivot quicksort, in implementation, it actually use insertion sort for tiny arrays. (See http://codeblab.com/wp-content/uploads/2009/09/DualPivotQuicksort.pdf).

          So I think the sort may not cause significant performance regression comparing to the original approach.

          If any regression happens, I would suggest to take a look at the additional array allocation as well (even if JVM manages memory allocation pretty fast). Probably we can use ThreadLocal static arrays to hold resource arrays instead of allocating arrays every time.

          Show
          leftnoteasy Wangda Tan added a comment - Daniel Templeton , I just checked, in Java, Arrays.sort uses dual pivot quicksort, in implementation, it actually use insertion sort for tiny arrays. (See http://codeblab.com/wp-content/uploads/2009/09/DualPivotQuicksort.pdf ). So I think the sort may not cause significant performance regression comparing to the original approach. If any regression happens, I would suggest to take a look at the additional array allocation as well (even if JVM manages memory allocation pretty fast). Probably we can use ThreadLocal static arrays to hold resource arrays instead of allocating arrays every time.
          Hide
          templedf Daniel Templeton added a comment -

          Here's a patch that adds a special case for # res == 2. Sunil G, any chance you can run it through SLS and see what the difference is?

          Show
          templedf Daniel Templeton added a comment - Here's a patch that adds a special case for # res == 2. Sunil G , any chance you can run it through SLS and see what the difference is?
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                YARN-3926 Compile Tests
          +1 mvninstall 13m 3s YARN-3926 passed
          +1 compile 0m 32s YARN-3926 passed
          +1 checkstyle 0m 20s YARN-3926 passed
          +1 mvnsite 0m 33s YARN-3926 passed
          +1 findbugs 1m 7s YARN-3926 passed
          +1 javadoc 0m 28s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 30s the patch passed
          +1 compile 0m 32s the patch passed
          +1 javac 0m 32s the patch passed
          +1 checkstyle 0m 18s the patch passed
          +1 mvnsite 0m 31s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 15s the patch passed
          +1 javadoc 0m 28s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)
                Other Tests
          +1 unit 2m 35s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          24m 2s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882050/YARN-6610.YARN-3926.perf.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux de412fc06846 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / 8f80907
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16921/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16921/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       YARN-3926 Compile Tests +1 mvninstall 13m 3s YARN-3926 passed +1 compile 0m 32s YARN-3926 passed +1 checkstyle 0m 20s YARN-3926 passed +1 mvnsite 0m 33s YARN-3926 passed +1 findbugs 1m 7s YARN-3926 passed +1 javadoc 0m 28s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 30s the patch passed +1 compile 0m 32s the patch passed +1 javac 0m 32s the patch passed +1 checkstyle 0m 18s the patch passed +1 mvnsite 0m 31s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 15s the patch passed +1 javadoc 0m 28s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4569 unchanged - 5 fixed = 4569 total (was 4574)       Other Tests +1 unit 2m 35s hadoop-yarn-common in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 24m 2s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882050/YARN-6610.YARN-3926.perf.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux de412fc06846 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / 8f80907 Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16921/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16921/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          templedf Daniel Templeton added a comment -

          I just did some direct profiling, and with the pref patch, the compare() method is an order of magnitude faster with only CPU and memory than before.

          Show
          templedf Daniel Templeton added a comment - I just did some direct profiling, and with the pref patch, the compare() method is an order of magnitude faster with only CPU and memory than before.
          Hide
          sunilg Sunil G added a comment -

          Thanks Daniel Templeton YARN-3926 is rebased to latest trunk, so we can run below unit test to see performance change. We ll run same and share results as well.

          Show
          sunilg Sunil G added a comment - Thanks Daniel Templeton YARN-3926 is rebased to latest trunk, so we can run below unit test to see performance change. We ll run same and share results as well.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Daniel Templeton,

          Thanks for updating patch, I did some test and got some amazing results:
          By executing test: mvn test -Dtest=TestCapacityScheduler#testUserLimitThroughput -DRunUserLimitThroughput=true

          For throughput (higher is better)
          1) YARN-3926 w/o the patch: 50505.05
          2) trunk: 57142.855
          3) 3926 + 6610 (perf): 606060.6 (!!)
          4) 3926 + 6610 (006): 46403.71

          Since YARN-3926 is rebased to trunk, I did some rebases while doing the test as well. The number looks great, I just hope we didn't accidentally introduce some issues. I will do some other verifications as well.

          Show
          leftnoteasy Wangda Tan added a comment - Daniel Templeton , Thanks for updating patch, I did some test and got some amazing results: By executing test: mvn test -Dtest=TestCapacityScheduler#testUserLimitThroughput -DRunUserLimitThroughput=true For throughput (higher is better) 1) YARN-3926 w/o the patch: 50505.05 2) trunk: 57142.855 3) 3926 + 6610 (perf): 606060.6 (!!) 4) 3926 + 6610 (006): 46403.71 Since YARN-3926 is rebased to trunk, I did some rebases while doing the test as well. The number looks great, I just hope we didn't accidentally introduce some issues. I will do some other verifications as well.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Attached rebased patch (with rebased suffix)

          Show
          leftnoteasy Wangda Tan added a comment - Attached rebased patch (with rebased suffix)
          Hide
          leftnoteasy Wangda Tan added a comment -

          Investigated more, it looks like after the perf patch, most user limit calculation becomes no-op, I'm trying to figure out what happened..

          Show
          leftnoteasy Wangda Tan added a comment - Investigated more, it looks like after the perf patch, most user limit calculation becomes no-op, I'm trying to figure out what happened..
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                YARN-3926 Compile Tests
          +1 mvninstall 15m 9s YARN-3926 passed
          +1 compile 0m 36s YARN-3926 passed
          +1 checkstyle 0m 26s YARN-3926 passed
          +1 mvnsite 0m 38s YARN-3926 passed
          +1 findbugs 1m 19s YARN-3926 passed
          +1 javadoc 0m 46s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 36s the patch passed
          +1 compile 0m 37s the patch passed
          +1 javac 0m 37s the patch passed
          +1 checkstyle 0m 23s the patch passed
          +1 mvnsite 0m 36s the patch passed
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 findbugs 1m 31s the patch passed
          +1 javadoc 0m 38s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515)
                Other Tests
          +1 unit 2m 32s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          27m 39s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882196/YARN-6610.YARN-3926.006-rebased.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 4ec4ced9f4fe 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / f25bc50
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16940/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16940/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16940/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       YARN-3926 Compile Tests +1 mvninstall 15m 9s YARN-3926 passed +1 compile 0m 36s YARN-3926 passed +1 checkstyle 0m 26s YARN-3926 passed +1 mvnsite 0m 38s YARN-3926 passed +1 findbugs 1m 19s YARN-3926 passed +1 javadoc 0m 46s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 36s the patch passed +1 compile 0m 37s the patch passed +1 javac 0m 37s the patch passed +1 checkstyle 0m 23s the patch passed +1 mvnsite 0m 36s the patch passed -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 findbugs 1m 31s the patch passed +1 javadoc 0m 38s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515)       Other Tests +1 unit 2m 32s hadoop-yarn-common in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 27m 39s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882196/YARN-6610.YARN-3926.006-rebased.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4ec4ced9f4fe 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / f25bc50 Default Java 1.8.0_144 findbugs v3.1.0-RC1 whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16940/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16940/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16940/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Investigated more, it looks like original perf patch has some typos inside calculateShares2, I tried to run all CapacityScheduler tests with the old perf patch, it causes many failures.

          With attached patch YARN-6610.YARN-3926.perf-wangda-001.patch, it has 2 failures, but looks unrelated. (Ran unit test w/o the patch, it fails as well).

          Daniel Templeton, it looks like existing tests in TestResourceCalculator are not enough to capture the issue, I tried to add some (testCompare2) but it looks not enough as well. If you have any cycles, could you help to add more unit tests?

          I also tried to run testUserLimitThroughput again, the latest patch shows very similar result.

          Since this is blocker of YARN-3926 merge, and it doesn't show performance regression, I plan to commit this patch to branch by end of today if no opposite opinions.

          Show
          leftnoteasy Wangda Tan added a comment - Investigated more, it looks like original perf patch has some typos inside calculateShares2 , I tried to run all CapacityScheduler tests with the old perf patch, it causes many failures. With attached patch YARN-6610 . YARN-3926 .perf-wangda-001.patch , it has 2 failures, but looks unrelated. (Ran unit test w/o the patch, it fails as well). Daniel Templeton , it looks like existing tests in TestResourceCalculator are not enough to capture the issue, I tried to add some (testCompare2) but it looks not enough as well. If you have any cycles, could you help to add more unit tests? I also tried to run testUserLimitThroughput again, the latest patch shows very similar result. Since this is blocker of YARN-3926 merge, and it doesn't show performance regression, I plan to commit this patch to branch by end of today if no opposite opinions.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
                YARN-3926 Compile Tests
          0 mvndep 0m 12s Maven dependency ordering for branch
          +1 mvninstall 15m 54s YARN-3926 passed
          +1 compile 10m 45s YARN-3926 passed
          +1 checkstyle 1m 4s YARN-3926 passed
          +1 mvnsite 1m 36s YARN-3926 passed
          +1 findbugs 2m 47s YARN-3926 passed
          +1 javadoc 1m 19s YARN-3926 passed
                Patch Compile Tests
          0 mvndep 0m 11s Maven dependency ordering for patch
          +1 mvninstall 1m 23s the patch passed
          +1 compile 7m 6s the patch passed
          +1 javac 7m 6s the patch passed
          -0 checkstyle 0m 58s hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 208 unchanged - 0 fixed = 209 total (was 208)
          +1 mvnsite 1m 19s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 26s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 41s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515)
          +1 javadoc 0m 27s hadoop-yarn-server-resourcemanager in the patch passed.
                Other Tests
          +1 unit 2m 30s hadoop-yarn-common in the patch passed.
          -1 unit 44m 25s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 26s The patch does not generate ASF License warnings.
          104m 16s



          Reason Tests
          FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
            org.apache.hadoop.yarn.util.resource.DominantResourceCalculator.totalInvoked isn't final but should be At DominantResourceCalculator.java:be At DominantResourceCalculator.java:[line 59]
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacityScheduler
            hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesApps
            hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882204/YARN-6610.YARN-3926.perf-wangda-001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux ad901b9f6d1b 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / f25bc50
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16941/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/16941/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.html
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16941/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16941/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16941/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.       YARN-3926 Compile Tests 0 mvndep 0m 12s Maven dependency ordering for branch +1 mvninstall 15m 54s YARN-3926 passed +1 compile 10m 45s YARN-3926 passed +1 checkstyle 1m 4s YARN-3926 passed +1 mvnsite 1m 36s YARN-3926 passed +1 findbugs 2m 47s YARN-3926 passed +1 javadoc 1m 19s YARN-3926 passed       Patch Compile Tests 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 1m 23s the patch passed +1 compile 7m 6s the patch passed +1 javac 7m 6s the patch passed -0 checkstyle 0m 58s hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 208 unchanged - 0 fixed = 209 total (was 208) +1 mvnsite 1m 19s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 26s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 41s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515) +1 javadoc 0m 27s hadoop-yarn-server-resourcemanager in the patch passed.       Other Tests +1 unit 2m 30s hadoop-yarn-common in the patch passed. -1 unit 44m 25s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 26s The patch does not generate ASF License warnings. 104m 16s Reason Tests FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common   org.apache.hadoop.yarn.util.resource.DominantResourceCalculator.totalInvoked isn't final but should be At DominantResourceCalculator.java:be At DominantResourceCalculator.java: [line 59] Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacityScheduler   hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesApps   hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882204/YARN-6610.YARN-3926.perf-wangda-001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ad901b9f6d1b 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / f25bc50 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16941/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/16941/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.html unit https://builds.apache.org/job/PreCommit-YARN-Build/16941/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16941/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/16941/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Oops, included some debug code, will update patch shortly.

          Show
          leftnoteasy Wangda Tan added a comment - Oops, included some debug code, will update patch shortly.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Updated patch (wangda-002)

          Show
          leftnoteasy Wangda Tan added a comment - Updated patch (wangda-002)
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 23s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                YARN-3926 Compile Tests
          +1 mvninstall 15m 4s YARN-3926 passed
          +1 compile 0m 34s YARN-3926 passed
          +1 checkstyle 0m 26s YARN-3926 passed
          +1 mvnsite 0m 38s YARN-3926 passed
          +1 findbugs 1m 23s YARN-3926 passed
          +1 javadoc 0m 34s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 32s the patch passed
          +1 compile 0m 29s the patch passed
          +1 javac 0m 29s the patch passed
          -0 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common: The patch generated 1 new + 5 unchanged - 0 fixed = 6 total (was 5)
          +1 mvnsite 0m 30s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 23s the patch passed
          +1 javadoc 0m 31s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515)
                Other Tests
          +1 unit 2m 43s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 14s The patch does not generate ASF License warnings.
          27m 4s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882255/YARN-6610.YARN-3926.perf-wangda-002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 6faec6dcf63f 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / f25bc50
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16949/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16949/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16949/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 23s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       YARN-3926 Compile Tests +1 mvninstall 15m 4s YARN-3926 passed +1 compile 0m 34s YARN-3926 passed +1 checkstyle 0m 26s YARN-3926 passed +1 mvnsite 0m 38s YARN-3926 passed +1 findbugs 1m 23s YARN-3926 passed +1 javadoc 0m 34s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 32s the patch passed +1 compile 0m 29s the patch passed +1 javac 0m 29s the patch passed -0 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common: The patch generated 1 new + 5 unchanged - 0 fixed = 6 total (was 5) +1 mvnsite 0m 30s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 23s the patch passed +1 javadoc 0m 31s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515)       Other Tests +1 unit 2m 43s hadoop-yarn-common in the patch passed. +1 asflicense 0m 14s The patch does not generate ASF License warnings. 27m 4s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882255/YARN-6610.YARN-3926.perf-wangda-002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 6faec6dcf63f 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / f25bc50 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16949/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16949/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16949/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          sunilg Sunil G added a comment -

          Latest changes looks fine. Fixing checkstyle comment and renaming calculateShares2 to calculateSharesForMandatoryResources

          Show
          sunilg Sunil G added a comment - Latest changes looks fine. Fixing checkstyle comment and renaming calculateShares2 to calculateSharesForMandatoryResources
          Hide
          sunilg Sunil G added a comment -

          Attaching same patch shared by Wangda Tan fixing an unused import and method name change.

          +1 on this patch.

          Show
          sunilg Sunil G added a comment - Attaching same patch shared by Wangda Tan fixing an unused import and method name change. +1 on this patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                YARN-3926 Compile Tests
          +1 mvninstall 13m 39s YARN-3926 passed
          +1 compile 0m 27s YARN-3926 passed
          +1 checkstyle 0m 24s YARN-3926 passed
          +1 mvnsite 0m 30s YARN-3926 passed
          +1 findbugs 1m 6s YARN-3926 passed
          +1 javadoc 0m 34s YARN-3926 passed
                Patch Compile Tests
          +1 mvninstall 0m 27s the patch passed
          +1 compile 0m 28s the patch passed
          +1 javac 0m 28s the patch passed
          +1 checkstyle 0m 19s the patch passed
          +1 mvnsite 0m 29s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 10s the patch passed
          +1 javadoc 0m 32s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515)
                Other Tests
          +1 unit 2m 28s hadoop-yarn-common in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          24m 21s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6610
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882319/YARN-6610.YARN-3926.007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 849c24fa9be6 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-3926 / f25bc50
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16957/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16957/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       YARN-3926 Compile Tests +1 mvninstall 13m 39s YARN-3926 passed +1 compile 0m 27s YARN-3926 passed +1 checkstyle 0m 24s YARN-3926 passed +1 mvnsite 0m 30s YARN-3926 passed +1 findbugs 1m 6s YARN-3926 passed +1 javadoc 0m 34s YARN-3926 passed       Patch Compile Tests +1 mvninstall 0m 27s the patch passed +1 compile 0m 28s the patch passed +1 javac 0m 28s the patch passed +1 checkstyle 0m 19s the patch passed +1 mvnsite 0m 29s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 10s the patch passed +1 javadoc 0m 32s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common generated 0 new + 4510 unchanged - 5 fixed = 4510 total (was 4515)       Other Tests +1 unit 2m 28s hadoop-yarn-common in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 24m 21s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6610 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882319/YARN-6610.YARN-3926.007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 849c24fa9be6 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-3926 / f25bc50 Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16957/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common Console output https://builds.apache.org/job/PreCommit-YARN-Build/16957/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          sunilg Sunil G added a comment -

          Committed to branch. Thank you very much Daniel Templeton for raising and handling same. Thanks Wangda Tan for pitching in and helping in this patch.

          Show
          sunilg Sunil G added a comment - Committed to branch. Thank you very much Daniel Templeton for raising and handling same. Thanks Wangda Tan for pitching in and helping in this patch.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Pushed changes to trunk, set fix version to 3.1.0

          Show
          leftnoteasy Wangda Tan added a comment - Pushed changes to trunk, set fix version to 3.1.0

            People

            • Assignee:
              templedf Daniel Templeton
              Reporter:
              templedf Daniel Templeton
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development