Details

    • Target Version/s:

      Description

      Enhance locks in AbstractYarnScheduler/CapacityScheduler/FairScheduler, as mentioned in YARN-3091, a possible solution is using read/write lock. Other fine-graind locks for specific purposes / bugs should be addressed in separated tickets.

      1. YARN-3139.branch-2.007.patch
        170 kB
        Wangda Tan
      2. YARN-3139.7.patch
        169 kB
        Wangda Tan
      3. YARN-3139.6.patch
        169 kB
        Wangda Tan
      4. YARN-3139.5.patch
        169 kB
        Wangda Tan
      5. YARN-3139.4.patch
        170 kB
        Wangda Tan
      6. YARN-3139.3.patch
        170 kB
        Wangda Tan
      7. YARN-3139.2.patch
        169 kB
        Wangda Tan
      8. YARN-3139.1.patch
        170 kB
        Wangda Tan
      9. YARN-3139.0.patch
        169 kB
        Wangda Tan

        Issue Links

          Activity

          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Arun Suresh and Jian He for review and commit!

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Arun Suresh and Jian He for review and commit!
          Hide
          jianhe Jian He added a comment -

          Committed to trunk and branch-2, thanks Wangda !

          Show
          jianhe Jian He added a comment - Committed to trunk and branch-2, thanks Wangda !
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +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.
          +1 mvninstall 7m 8s branch-2 passed
          +1 compile 0m 28s branch-2 passed with JDK v1.8.0_101
          +1 compile 0m 33s branch-2 passed with JDK v1.7.0_111
          +1 checkstyle 0m 25s branch-2 passed
          +1 mvnsite 0m 37s branch-2 passed
          +1 mvneclipse 0m 17s branch-2 passed
          +1 findbugs 1m 13s branch-2 passed
          +1 javadoc 0m 21s branch-2 passed with JDK v1.8.0_101
          +1 javadoc 0m 23s branch-2 passed with JDK v1.7.0_111
          +1 mvninstall 0m 31s the patch passed
          +1 compile 0m 27s the patch passed with JDK v1.8.0_101
          +1 javac 0m 27s the patch passed
          +1 compile 0m 29s the patch passed with JDK v1.7.0_111
          +1 javac 0m 29s the patch passed
          -1 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 241 unchanged - 53 fixed = 248 total (was 294)
          +1 mvnsite 0m 35s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 21s the patch passed
          +1 javadoc 0m 21s the patch passed with JDK v1.8.0_101
          +1 javadoc 0m 22s the patch passed with JDK v1.7.0_111
          +1 unit 39m 28s hadoop-yarn-server-resourcemanager in the patch passed with JDK v1.8.0_101.
          +1 unit 40m 20s hadoop-yarn-server-resourcemanager in the patch passed with JDK v1.7.0_111.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          97m 32s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:b59b8b7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831820/YARN-3139.branch-2.007.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 8196edcdafe5 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2 / 539c3e4
          Default Java 1.7.0_111
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13298/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13298/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13298/console
          Powered by Apache Yetus 0.3.0 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. +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. +1 mvninstall 7m 8s branch-2 passed +1 compile 0m 28s branch-2 passed with JDK v1.8.0_101 +1 compile 0m 33s branch-2 passed with JDK v1.7.0_111 +1 checkstyle 0m 25s branch-2 passed +1 mvnsite 0m 37s branch-2 passed +1 mvneclipse 0m 17s branch-2 passed +1 findbugs 1m 13s branch-2 passed +1 javadoc 0m 21s branch-2 passed with JDK v1.8.0_101 +1 javadoc 0m 23s branch-2 passed with JDK v1.7.0_111 +1 mvninstall 0m 31s the patch passed +1 compile 0m 27s the patch passed with JDK v1.8.0_101 +1 javac 0m 27s the patch passed +1 compile 0m 29s the patch passed with JDK v1.7.0_111 +1 javac 0m 29s the patch passed -1 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 241 unchanged - 53 fixed = 248 total (was 294) +1 mvnsite 0m 35s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 21s the patch passed +1 javadoc 0m 21s the patch passed with JDK v1.8.0_101 +1 javadoc 0m 22s the patch passed with JDK v1.7.0_111 +1 unit 39m 28s hadoop-yarn-server-resourcemanager in the patch passed with JDK v1.8.0_101. +1 unit 40m 20s hadoop-yarn-server-resourcemanager in the patch passed with JDK v1.7.0_111. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 97m 32s Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831820/YARN-3139.branch-2.007.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 8196edcdafe5 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2 / 539c3e4 Default Java 1.7.0_111 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13298/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13298/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13298/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Attached patch for branch-2

          Show
          leftnoteasy Wangda Tan added a comment - Attached patch for branch-2
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10542 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10542/)
          YARN-3139. Improve locks in (jianhe: rev 31f8da22d0b8d2dcce5fbc8e45d832f40acf056f)

          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairScheduler.java
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractYarnScheduler.java
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/common/fica/FiCaSchedulerApp.java
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/SchedulerApplicationAttempt.java
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/LeafQueue.java
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMServerUtils.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10542 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10542/ ) YARN-3139 . Improve locks in (jianhe: rev 31f8da22d0b8d2dcce5fbc8e45d832f40acf056f) (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairScheduler.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractYarnScheduler.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/common/fica/FiCaSchedulerApp.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/SchedulerApplicationAttempt.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/LeafQueue.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMServerUtils.java
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +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.
          +1 mvninstall 8m 18s trunk passed
          +1 compile 0m 40s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 0m 42s trunk passed
          +1 mvneclipse 0m 20s trunk passed
          +1 findbugs 1m 9s trunk passed
          +1 javadoc 0m 23s trunk passed
          +1 mvninstall 0m 35s the patch passed
          +1 compile 0m 36s the patch passed
          +1 javac 0m 36s the patch passed
          -1 checkstyle 0m 23s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 242 unchanged - 53 fixed = 249 total (was 295)
          +1 mvnsite 0m 42s the patch passed
          +1 mvneclipse 0m 17s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 11s the patch passed
          +1 javadoc 0m 22s the patch passed
          +1 unit 39m 37s hadoop-yarn-server-resourcemanager in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          57m 6s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831441/YARN-3139.7.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 574caae163ec 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 853d65a
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13270/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13270/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13270/console
          Powered by Apache Yetus 0.3.0 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. +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. +1 mvninstall 8m 18s trunk passed +1 compile 0m 40s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 0m 42s trunk passed +1 mvneclipse 0m 20s trunk passed +1 findbugs 1m 9s trunk passed +1 javadoc 0m 23s trunk passed +1 mvninstall 0m 35s the patch passed +1 compile 0m 36s the patch passed +1 javac 0m 36s the patch passed -1 checkstyle 0m 23s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 242 unchanged - 53 fixed = 249 total (was 295) +1 mvnsite 0m 42s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 11s the patch passed +1 javadoc 0m 22s the patch passed +1 unit 39m 37s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 57m 6s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831441/YARN-3139.7.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 574caae163ec 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 853d65a Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13270/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13270/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13270/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Rebased patch to latest trunk (ver.7)

          Show
          leftnoteasy Wangda Tan added a comment - Rebased patch to latest trunk (ver.7)
          Hide
          jianhe Jian He added a comment -

          latest patch looks good

          Show
          jianhe Jian He added a comment - latest patch looks good
          Hide
          leftnoteasy Wangda Tan added a comment -

          Daniel Templeton,

          Do you have chance to look at the latest patch?
          Thanks

          Show
          leftnoteasy Wangda Tan added a comment - Daniel Templeton , Do you have chance to look at the latest patch? Thanks
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +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.
          +1 mvninstall 7m 26s trunk passed
          +1 compile 0m 33s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 38s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 0m 57s trunk passed
          +1 javadoc 0m 21s trunk passed
          +1 mvninstall 0m 32s the patch passed
          +1 compile 0m 31s the patch passed
          +1 javac 0m 31s the patch passed
          -1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 242 unchanged - 54 fixed = 248 total (was 296)
          +1 mvnsite 0m 35s the patch passed
          +1 mvneclipse 0m 15s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 8s the patch passed
          +1 javadoc 0m 18s the patch passed
          -1 unit 34m 26s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          50m 0s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830850/YARN-3139.6.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 49cd0d18bfa6 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 47f8092
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13247/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13247/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13247/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/13247/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13247/console
          Powered by Apache Yetus 0.3.0 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. +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. +1 mvninstall 7m 26s trunk passed +1 compile 0m 33s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 57s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 32s the patch passed +1 compile 0m 31s the patch passed +1 javac 0m 31s the patch passed -1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 242 unchanged - 54 fixed = 248 total (was 296) +1 mvnsite 0m 35s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 8s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 34m 26s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 50m 0s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830850/YARN-3139.6.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 49cd0d18bfa6 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 47f8092 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13247/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/13247/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13247/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/13247/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13247/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Jian He, added the null check, somehow I forgot updating this. Fixed this in ver.6 patch, mind to review again?

          Show
          leftnoteasy Wangda Tan added a comment - Jian He , added the null check, somehow I forgot updating this. Fixed this in ver.6 patch, mind to review again?
          Hide
          jianhe Jian He added a comment - - edited

          Patch looks good to me.

          Instead of taking readLock, I think we can check if getNode(container.getNodeId()) == null, and stop if it is.

          In the latest patch, do you intend to not check "getNode(container.getNodeId()) == null" ? I think this is possible if a node is removed while the container on that is being released by AM.

          Show
          jianhe Jian He added a comment - - edited Patch looks good to me. Instead of taking readLock, I think we can check if getNode(container.getNodeId()) == null, and stop if it is. In the latest patch, do you intend to not check "getNode(container.getNodeId()) == null" ? I think this is possible if a node is removed while the container on that is being released by AM.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +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.
          +1 mvninstall 7m 21s trunk passed
          +1 compile 0m 34s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 38s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 0m 59s trunk passed
          +1 javadoc 0m 20s trunk passed
          +1 mvninstall 0m 30s the patch passed
          +1 compile 0m 32s the patch passed
          +1 javac 0m 32s the patch passed
          -1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 242 unchanged - 54 fixed = 248 total (was 296)
          +1 mvnsite 0m 39s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 6s the patch passed
          +1 javadoc 0m 18s the patch passed
          -1 unit 34m 50s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          50m 19s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830577/YARN-3139.5.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 5ba4a2553bff 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 1831be8
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13230/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13230/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13230/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/13230/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13230/console
          Powered by Apache Yetus 0.3.0 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. +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. +1 mvninstall 7m 21s trunk passed +1 compile 0m 34s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 59s trunk passed +1 javadoc 0m 20s trunk passed +1 mvninstall 0m 30s the patch passed +1 compile 0m 32s the patch passed +1 javac 0m 32s the patch passed -1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 242 unchanged - 54 fixed = 248 total (was 296) +1 mvnsite 0m 39s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 6s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 34m 50s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 50m 19s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830577/YARN-3139.5.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5ba4a2553bff 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 1831be8 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13230/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/13230/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13230/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/13230/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13230/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Daniel Templeton,

          but there are a large number of line break changes that make it hard to tell what has actually changed.

          Actually I deliberately avoid making unnecessary changes, the try .. finally block added extra indent, so I have to reformat code inside that block, it will be very hard for me to revert such change

          Show
          leftnoteasy Wangda Tan added a comment - Daniel Templeton , but there are a large number of line break changes that make it hard to tell what has actually changed. Actually I deliberately avoid making unnecessary changes, the try .. finally block added extra indent, so I have to reformat code inside that block, it will be very hard for me to revert such change
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Jian He, attached ver.5 patch.

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Jian He , attached ver.5 patch.
          Hide
          templedf Daniel Templeton added a comment -

          I've started reviewing the patch, but there are a large number of line break changes that make it hard to tell what has actually changed. Could you please post a patch with only the changes that are relevant to the JIRA?

          Show
          templedf Daniel Templeton added a comment - I've started reviewing the patch, but there are a large number of line break changes that make it hard to tell what has actually changed. Could you please post a patch with only the changes that are relevant to the JIRA?
          Hide
          jianhe Jian He added a comment -
          • I remember previous intention was trying avoid lock. Instead of taking readLock, I think we can check if getNode(container.getNodeId()) == null, and stop if it is. Because even if read lock is taken here, NPE would still occur later if node == null
                  /*
                   * Acquire readLock here to make sure application / node will not be
                   * removed when this is being invoked
                   */
                  readLock.lock();
            
          • CapacityScheduler#checkAccess does not need to hold the readlock, the underlying authorizer has read/write lock protection. And in fact, this checkAccess is not used by core code any more..
          Show
          jianhe Jian He added a comment - I remember previous intention was trying avoid lock. Instead of taking readLock, I think we can check if getNode(container.getNodeId()) == null, and stop if it is. Because even if read lock is taken here, NPE would still occur later if node == null /* * Acquire readLock here to make sure application / node will not be * removed when this is being invoked */ readLock.lock(); CapacityScheduler#checkAccess does not need to hold the readlock, the underlying authorizer has read/write lock protection. And in fact, this checkAccess is not used by core code any more..
          Hide
          leftnoteasy Wangda Tan added a comment -

          Test failure is not related.

          Show
          leftnoteasy Wangda Tan added a comment - Test failure is not related.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +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.
          +1 mvninstall 7m 19s trunk passed
          +1 compile 0m 34s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 40s trunk passed
          +1 mvneclipse 0m 19s trunk passed
          +1 findbugs 1m 6s trunk passed
          +1 javadoc 0m 21s trunk passed
          +1 mvninstall 0m 35s the patch passed
          +1 compile 0m 31s the patch passed
          +1 javac 0m 31s the patch passed
          -1 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 242 unchanged - 54 fixed = 248 total (was 296)
          +1 mvnsite 0m 38s the patch passed
          +1 mvneclipse 0m 15s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 5s the patch passed
          +1 javadoc 0m 19s the patch passed
          -1 unit 35m 45s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          51m 33s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830154/YARN-3139.4.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 0192fd46eb6f 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6eb700e
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13203/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13203/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13203/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/13203/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13203/console
          Powered by Apache Yetus 0.3.0 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. +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. +1 mvninstall 7m 19s trunk passed +1 compile 0m 34s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 19s trunk passed +1 findbugs 1m 6s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 35s the patch passed +1 compile 0m 31s the patch passed +1 javac 0m 31s the patch passed -1 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 242 unchanged - 54 fixed = 248 total (was 296) +1 mvnsite 0m 38s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 5s the patch passed +1 javadoc 0m 19s the patch passed -1 unit 35m 45s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 51m 33s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830154/YARN-3139.4.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0192fd46eb6f 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6eb700e Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13203/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/13203/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13203/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/13203/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13203/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Arun Suresh, updated ver.4 patch, only removed unused imports.

          There're too many similar checkstyle warnings in scheduler, I would prefer to keep them as-is. Adding suppress warning statement maybe not good for readability to me. As they're all trivial warnings too me.

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Arun Suresh , updated ver.4 patch, only removed unused imports. There're too many similar checkstyle warnings in scheduler, I would prefer to keep them as-is. Adding suppress warning statement maybe not good for readability to me. As they're all trivial warnings too me.
          Hide
          asuresh Arun Suresh added a comment -

          Thanks for addressing the comments Wangda Tan
          w.r.t to the checkstyles, the unused imports can be fixed.. and maybe use changes in HADOOP-13411 to suppress the rest of the warnings.
          +1 pending the above..

          Show
          asuresh Arun Suresh added a comment - Thanks for addressing the comments Wangda Tan w.r.t to the checkstyles, the unused imports can be fixed.. and maybe use changes in HADOOP-13411 to suppress the rest of the warnings. +1 pending the above..
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
          +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.
          +1 mvninstall 7m 15s trunk passed
          +1 compile 0m 33s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 38s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 0s trunk passed
          +1 javadoc 0m 21s trunk passed
          +1 mvninstall 0m 31s the patch passed
          +1 compile 0m 29s the patch passed
          +1 javac 0m 29s the patch passed
          -1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 241 unchanged - 54 fixed = 248 total (was 295)
          +1 mvnsite 0m 35s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 3s the patch passed
          +1 javadoc 0m 18s the patch passed
          +1 unit 33m 57s hadoop-yarn-server-resourcemanager in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          49m 20s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830090/YARN-3139.3.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 877262f43ea1 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6e849cb
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13200/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13200/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13200/console
          Powered by Apache Yetus 0.3.0 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. +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. +1 mvninstall 7m 15s trunk passed +1 compile 0m 33s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 0s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 31s the patch passed +1 compile 0m 29s the patch passed +1 javac 0m 29s the patch passed -1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 241 unchanged - 54 fixed = 248 total (was 295) +1 mvnsite 0m 35s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 3s the patch passed +1 javadoc 0m 18s the patch passed +1 unit 33m 57s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 49m 20s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830090/YARN-3139.3.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 877262f43ea1 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6e849cb Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13200/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13200/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13200/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Attached ver.3 patch.

          Show
          leftnoteasy Wangda Tan added a comment - Attached ver.3 patch.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Arun Suresh for the review, actually I'm working on the patch

          AbstractYarnScheduler::getApplicationAttempt() : Do you need the read lock ...

          Agree, fixed

          AbstractYarnScheduler::moveAllApps() : Don't think we need a writelock...

          The reason I put a writelock because we want the move operation to be serialized, for example, it may have some issue when we do two moveAll operation at the same time. To me moveApp is equivalent to remove the app and add the new one. Same as we protect addAppAttempt, we should use the same writelock for safety.

          SchedulerApplicationAttempt::pendingRelease :Maybe use a ConcurrentSkipListSet ?

          Unless we need to order the set, I think use ConcurrentHashSet should be enough, SkiplistSet gets worse performance (O(logN) vs. O(1))

          Considering that initScheduler is called exactly once, and the Scheduler cannot be used before it is initted, do we need a write lock there ?

          I would prefer keep it just for safety and consistency, as it doesn't affect performance. Same for stop and startSchedulerThreads

          FairScheduler 2.. 3..

          Addressed

          And Jian He,
          I revisited checkSchedContainerChangeRequest, it is actually safe, so I removed comment of the original method.

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Arun Suresh for the review, actually I'm working on the patch AbstractYarnScheduler::getApplicationAttempt() : Do you need the read lock ... Agree, fixed AbstractYarnScheduler::moveAllApps() : Don't think we need a writelock... The reason I put a writelock because we want the move operation to be serialized, for example, it may have some issue when we do two moveAll operation at the same time. To me moveApp is equivalent to remove the app and add the new one. Same as we protect addAppAttempt, we should use the same writelock for safety. SchedulerApplicationAttempt::pendingRelease :Maybe use a ConcurrentSkipListSet ? Unless we need to order the set, I think use ConcurrentHashSet should be enough, SkiplistSet gets worse performance (O(logN) vs. O(1)) Considering that initScheduler is called exactly once, and the Scheduler cannot be used before it is initted, do we need a write lock there ? I would prefer keep it just for safety and consistency, as it doesn't affect performance. Same for stop and startSchedulerThreads FairScheduler 2.. 3.. Addressed And Jian He , I revisited checkSchedContainerChangeRequest , it is actually safe, so I removed comment of the original method.
          Hide
          asuresh Arun Suresh added a comment -

          Thanks for the patch Daniel Templeton..

          AbstractYarnScheduler:

          1. AbstractYarnScheduler::getApplicationAttempt() : Do you need the read lock, since applications is already a ConcurrentHashMap ?
          2. AbstractYarnScheduler::moveAllApps() : Don't think we need a writelock there since the only state change is affected via an RMAppMoveEvent. A readLock should suffice, but if getAppsInQueue is synchronized, even that might not be required.
          3. Same for the AbstractYarnScheduler:: killAllAppsInQueue()

          SchedulerApplicationAttempt:

          1. SchedulerApplicationAttempt::pendingRelease : Maybe use a ConcurrentSkipListSet ?

          CapacityScheduler:

          1. Considering that initScheduler is called exactly once, and the Scheduler cannot be used before it is initted, do we need a write lock there ?
          2. Same goes for startSchedulerThreads() and stopService().. Wangda Tan Thoughts ?

          FairScheduler:

          1. In multiple places, maybe it is possible to reduce the granuality of the write locks... for eg. in FairScheduler::addApplication(), you dont really need the lock before line 664.
          2. removeApplication(), do you need to do a get and then remove... the remove will give you the previous app and you can log if null right ? Then you wont event need a lock since applications is a concurrent Hashmap.
          3. In removeApplicationAttempt
                SchedulerApplication<FSAppAttempt> application =
                    applications.get(applicationAttemptId.getApplicationId());
                FSAppAttempt attempt = getSchedulerApp(applicationAttemptId);
            

            can be replaced with applications.get(applicationAttemptId.getApplicationId()).getCurrentAppAttempt() right?

          Show
          asuresh Arun Suresh added a comment - Thanks for the patch Daniel Templeton .. AbstractYarnScheduler: AbstractYarnScheduler::getApplicationAttempt() : Do you need the read lock, since applications is already a ConcurrentHashMap ? AbstractYarnScheduler::moveAllApps() : Don't think we need a writelock there since the only state change is affected via an RMAppMoveEvent . A readLock should suffice, but if getAppsInQueue is synchronized, even that might not be required. Same for the AbstractYarnScheduler:: killAllAppsInQueue() SchedulerApplicationAttempt: SchedulerApplicationAttempt::pendingRelease : Maybe use a ConcurrentSkipListSet ? CapacityScheduler: Considering that initScheduler is called exactly once, and the Scheduler cannot be used before it is initted, do we need a write lock there ? Same goes for startSchedulerThreads() and stopService() .. Wangda Tan Thoughts ? FairScheduler: In multiple places, maybe it is possible to reduce the granuality of the write locks... for eg. in FairScheduler::addApplication() , you dont really need the lock before line 664. removeApplication() , do you need to do a get and then remove... the remove will give you the previous app and you can log if null right ? Then you wont event need a lock since applications is a concurrent Hashmap. In removeApplicationAttempt SchedulerApplication<FSAppAttempt> application = applications.get(applicationAttemptId.getApplicationId()); FSAppAttempt attempt = getSchedulerApp(applicationAttemptId); can be replaced with applications.get(applicationAttemptId.getApplicationId()).getCurrentAppAttempt() right?
          Hide
          kasha Karthik Kambatla added a comment -

          Sorry Wangda Tan. I will be traveling and won't have a chance to look at this until about 2 weeks from now.

          Show
          kasha Karthik Kambatla added a comment - Sorry Wangda Tan . I will be traveling and won't have a chance to look at this until about 2 weeks from now.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Daniel Templeton, but I really want to get this done sooner as it blocks review of other patches. Arun Suresh / Karthik Kambatla, do you have time to look at this patch, this is a big patch but logic should be fairly straightforward

          Thanks,

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Daniel Templeton , but I really want to get this done sooner as it blocks review of other patches. Arun Suresh / Karthik Kambatla , do you have time to look at this patch, this is a big patch but logic should be fairly straightforward Thanks,
          Hide
          leftnoteasy Wangda Tan added a comment -

          Tsuyoshi Ozawa,

          Yeah we plan to add multi-threading scheduling implementation. This change is first step of the YARN-5139. You can check design doc, and implementation notes for details.

          To avoid disruptive changes, we just want to simply convert synchronized lock to R/W lock for this patch, will do more fine-grained improvements in separate patches.

          Show
          leftnoteasy Wangda Tan added a comment - Tsuyoshi Ozawa , Yeah we plan to add multi-threading scheduling implementation. This change is first step of the YARN-5139 . You can check design doc , and implementation notes for details. To avoid disruptive changes, we just want to simply convert synchronized lock to R/W lock for this patch, will do more fine-grained improvements in separate patches.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Actually we need to hold application's lock when this is called. Will update a patch later.

          Show
          leftnoteasy Wangda Tan added a comment - Actually we need to hold application's lock when this is called. Will update a patch later.
          Hide
          templedf Daniel Templeton added a comment -

          Yep. I added it to my todo list, but I won't get there until next week.

          Show
          templedf Daniel Templeton added a comment - Yep. I added it to my todo list, but I won't get there until next week.
          Hide
          ozawa Tsuyoshi Ozawa added a comment -

          Wangda Tan Jian He thanks for taking this issue.

          Summary:
          No regression in performance, didn't see deadlock happens.
          No significant performance improvement either, because existing scheduler allocation is still in single thread.

          If the performance doesn't change, could you clarify the reason to change this? Do you plan to make the scheduler allocation multi-threaded?

          Show
          ozawa Tsuyoshi Ozawa added a comment - Wangda Tan Jian He thanks for taking this issue. Summary: No regression in performance, didn't see deadlock happens. No significant performance improvement either, because existing scheduler allocation is still in single thread. If the performance doesn't change, could you clarify the reason to change this? Do you plan to make the scheduler allocation multi-threaded?
          Hide
          jianhe Jian He added a comment -

          Daniel Templeton, would you like to check the FairScheduler changes?

          Show
          jianhe Jian He added a comment - Daniel Templeton , would you like to check the FairScheduler changes?
          Hide
          jianhe Jian He added a comment - - edited

          There are also similar comments like this... I just couldn't remember why these comments were added.. Anyway, may be it's fine..

            /**
             * Validate increase/decrease request. This function must be called under
             * the queue lock to make sure that the access to container resource is
             * atomic. Refer to LeafQueue.decreaseContainer() and
             * CapacityScheduelr.updateIncreaseRequests()
             * <pre>
             * - Throw exception when any other error happens
             * </pre>
             */
            public static void checkSchedContainerChangeRequest(
          

          May be it's related to some consistency issues with respect to queue stats?

          Show
          jianhe Jian He added a comment - - edited There are also similar comments like this... I just couldn't remember why these comments were added.. Anyway, may be it's fine.. /** * Validate increase/decrease request. This function must be called under * the queue lock to make sure that the access to container resource is * atomic. Refer to LeafQueue.decreaseContainer() and * CapacityScheduelr.updateIncreaseRequests() * <pre> * - Throw exception when any other error happens * </pre> */ public static void checkSchedContainerChangeRequest( May be it's related to some consistency issues with respect to queue stats?
          Hide
          leftnoteasy Wangda Tan added a comment -

          Failed tests aren't related to this changes, YARN-5548/YARN-5349 are tracking failed tests.

          Show
          leftnoteasy Wangda Tan added a comment - Failed tests aren't related to this changes, YARN-5548 / YARN-5349 are tracking failed tests.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +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.
          +1 mvninstall 7m 42s trunk passed
          +1 compile 0m 33s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 0m 41s trunk passed
          +1 mvneclipse 0m 18s trunk passed
          +1 findbugs 1m 2s trunk passed
          +1 javadoc 0m 23s trunk passed
          +1 mvninstall 0m 34s the patch passed
          +1 compile 0m 32s the patch passed
          +1 javac 0m 32s the patch passed
          -1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 232 unchanged - 54 fixed = 239 total (was 286)
          +1 mvnsite 0m 37s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 7s the patch passed
          +1 javadoc 0m 20s the patch passed
          -1 unit 33m 34s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          49m 33s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.TestWorkPreservingRMRestart
            hadoop.yarn.server.resourcemanager.TestRMRestart



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829664/YARN-3139.2.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c31d0679caec 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 964e546
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13179/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13179/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13179/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/13179/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13179/console
          Powered by Apache Yetus 0.3.0 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 14s Docker mode activated. +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. +1 mvninstall 7m 42s trunk passed +1 compile 0m 33s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 41s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 2s trunk passed +1 javadoc 0m 23s trunk passed +1 mvninstall 0m 34s the patch passed +1 compile 0m 32s the patch passed +1 javac 0m 32s the patch passed -1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 232 unchanged - 54 fixed = 239 total (was 286) +1 mvnsite 0m 37s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 7s the patch passed +1 javadoc 0m 20s the patch passed -1 unit 33m 34s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 49m 33s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestWorkPreservingRMRestart   hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829664/YARN-3139.2.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c31d0679caec 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 964e546 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13179/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/13179/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13179/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/13179/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13179/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment - - edited

          Thanks Jian He for review:

          Is removing these locks ok?..

          The original lock is added by YARN-4519, but I found instead of locking on leaf queue, we can simply lock the application. When an application is locked, we cannot release/increase/decrease a container while another release/increase/decrease container is in progress. So we will be safe if all these operations are protected by application's writeLock.

          And here is a summary of how scheduler/queue/app locks being used after this patch:

          rollback/decreaseContainer/completedContainer: 
              LeafQueue.writeLock -> app.writeLock
          updateRequest/updateIncreaseRequest :
              app.writeLock
          new allocation/new reservation/remove or add app/remove or add node/reinitialize :
              CS.writeLock -> PQ.writeLock -> LQ.writeLock -> app.writeLock
          

          So I think existing model looks fine.

          Also, here.. the queue lock is removed

          Same as above

          In fairscheduler, here, locking on application is changed to locking on scheduler ?

          Fixed

          the lock is outside try block...

          Actually this is a workaround for the previously reported findbugs warning: https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html
          This is safe and it is suggested, because when writeLock.lock() fails (like throw any exception), the writeLock.unlock() will not be invoked. However in practice, putting writelock inside try or out of try are both safe, because writeLock.lock() will not likely throw any exception.

          Uploading ver.2 patch, fixed a couple of unnecessary readlocks.

          Show
          leftnoteasy Wangda Tan added a comment - - edited Thanks Jian He for review: Is removing these locks ok?.. The original lock is added by YARN-4519 , but I found instead of locking on leaf queue, we can simply lock the application. When an application is locked, we cannot release/increase/decrease a container while another release/increase/decrease container is in progress. So we will be safe if all these operations are protected by application's writeLock. And here is a summary of how scheduler/queue/app locks being used after this patch: rollback/decreaseContainer/completedContainer: LeafQueue.writeLock -> app.writeLock updateRequest/updateIncreaseRequest : app.writeLock new allocation/ new reservation/remove or add app/remove or add node/reinitialize : CS.writeLock -> PQ.writeLock -> LQ.writeLock -> app.writeLock So I think existing model looks fine. Also, here.. the queue lock is removed Same as above In fairscheduler, here, locking on application is changed to locking on scheduler ? Fixed the lock is outside try block... Actually this is a workaround for the previously reported findbugs warning: https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html This is safe and it is suggested, because when writeLock.lock() fails (like throw any exception), the writeLock.unlock() will not be invoked. However in practice, putting writelock inside try or out of try are both safe, because writeLock.lock() will not likely throw any exception. Uploading ver.2 patch, fixed a couple of unnecessary readlocks.
          Hide
          jianhe Jian He added a comment -
          • Is removing these locks ok?
              // It is crucial to acquire leaf queue lock first to prevent:
              // 1. Race condition when calculating the delta resource in
              //    SchedContainerChangeRequest
              // 2. Deadlock with the scheduling thread.
            
          • Also, here.. the queue lock is removed
                synchronized(leafQueue) { 
                  SchedulerNode schedulerNode =
                      getSchedulerNode(rmContainer.getAllocatedNode());
                  SchedContainerChangeRequest decreaseRequest =
                      new SchedContainerChangeRequest(this.rmContext, schedulerNode,
                          rmContainer, rmContainer.getLastConfirmedResource());
                  decreaseContainer(decreaseRequest, application);
                }
            
          • In fairscheduler, here, locking on application is changed to locking on scheduler ?
            try {
                  readLock.lock();
                  if (!ask.isEmpty()) {
                    if (LOG.isDebugEnabled()) {
                      LOG.debug(
                          "allocate: pre-update" + " applicationAttemptId=" + appAttemptId
                              + " application=" + application.getApplicationId());
                    }
                    application.showRequests();
            
          • the lock is outside try block...
                  // Commit the reload; also create any queue defined in the alloc file
                  // if it does not already exist, so it can be displayed on the web UI.
            
                  writeLock.lock();
                  try {
            
          Show
          jianhe Jian He added a comment - Is removing these locks ok? // It is crucial to acquire leaf queue lock first to prevent: // 1. Race condition when calculating the delta resource in // SchedContainerChangeRequest // 2. Deadlock with the scheduling thread. Also, here.. the queue lock is removed synchronized (leafQueue) { SchedulerNode schedulerNode = getSchedulerNode(rmContainer.getAllocatedNode()); SchedContainerChangeRequest decreaseRequest = new SchedContainerChangeRequest( this .rmContext, schedulerNode, rmContainer, rmContainer.getLastConfirmedResource()); decreaseContainer(decreaseRequest, application); } In fairscheduler, here, locking on application is changed to locking on scheduler ? try { readLock.lock(); if (!ask.isEmpty()) { if (LOG.isDebugEnabled()) { LOG.debug( "allocate: pre-update" + " applicationAttemptId=" + appAttemptId + " application=" + application.getApplicationId()); } application.showRequests(); the lock is outside try block... // Commit the reload; also create any queue defined in the alloc file // if it does not already exist, so it can be displayed on the web UI. writeLock.lock(); try {
          Hide
          leftnoteasy Wangda Tan added a comment -

          Jian He, Karthik Kambatla, Daniel Templeton, could you take a look at the patch? Will investigate UT failure tomorrow.

          Show
          leftnoteasy Wangda Tan added a comment - Jian He , Karthik Kambatla , Daniel Templeton , could you take a look at the patch? Will investigate UT failure tomorrow.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +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.
          +1 mvninstall 7m 4s trunk passed
          +1 compile 0m 34s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 39s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 7s trunk passed
          +1 javadoc 0m 23s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 0m 33s the patch passed
          +1 javac 0m 33s the patch passed
          -1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 232 unchanged - 54 fixed = 239 total (was 286)
          +1 mvnsite 0m 47s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 1s The patch has no whitespace issues.
          +1 findbugs 1m 10s the patch passed
          +1 javadoc 0m 22s the patch passed
          -1 unit 34m 44s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          50m 35s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestNodeLabelContainerAllocation
            hadoop.yarn.server.resourcemanager.TestWorkPreservingRMRestart
            hadoop.yarn.server.resourcemanager.TestRMRestart



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829503/YARN-3139.1.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 5ca916846d2b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 964e546
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13174/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13174/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13174/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/13174/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13174/console
          Powered by Apache Yetus 0.3.0 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. +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. +1 mvninstall 7m 4s trunk passed +1 compile 0m 34s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 39s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 7s trunk passed +1 javadoc 0m 23s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 0m 33s the patch passed +1 javac 0m 33s the patch passed -1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 7 new + 232 unchanged - 54 fixed = 239 total (was 286) +1 mvnsite 0m 47s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. +1 findbugs 1m 10s the patch passed +1 javadoc 0m 22s the patch passed -1 unit 34m 44s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 50m 35s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestNodeLabelContainerAllocation   hadoop.yarn.server.resourcemanager.TestWorkPreservingRMRestart   hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12829503/YARN-3139.1.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5ca916846d2b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 964e546 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13174/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/13174/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13174/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/13174/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13174/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Attached ver.1 patch fixed findbugs warning and checkstyle.

          Show
          leftnoteasy Wangda Tan added a comment - Attached ver.1 patch fixed findbugs warning and checkstyle.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
          +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.
          +1 mvninstall 7m 44s trunk passed
          +1 compile 0m 32s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 37s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 0m 58s trunk passed
          +1 javadoc 0m 20s trunk passed
          +1 mvninstall 0m 31s the patch passed
          +1 compile 0m 29s the patch passed
          +1 javac 0m 29s the patch passed
          -1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 18 new + 236 unchanged - 51 fixed = 254 total (was 287)
          +1 mvnsite 0m 35s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 3s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 17s the patch passed
          -1 unit 35m 49s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          51m 23s



          Reason Tests
          FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
            org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler$AllocationReloadListener.onReload(AllocationConfiguration) does not release lock on all exception paths At FairScheduler.java:on all exception paths At FairScheduler.java:[line 1651]
          Failed junit tests hadoop.yarn.server.resourcemanager.TestRMHA



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828352/YARN-3139.0.patch
          JIRA Issue YARN-3139
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 82f046cef257 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / e80386d
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13166/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/13166/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13166/console
          Powered by Apache Yetus 0.3.0 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 14s Docker mode activated. +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. +1 mvninstall 7m 44s trunk passed +1 compile 0m 32s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 37s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 58s trunk passed +1 javadoc 0m 20s trunk passed +1 mvninstall 0m 31s the patch passed +1 compile 0m 29s the patch passed +1 javac 0m 29s the patch passed -1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 18 new + 236 unchanged - 51 fixed = 254 total (was 287) +1 mvnsite 0m 35s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 3s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 17s the patch passed -1 unit 35m 49s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 51m 23s Reason Tests FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager   org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler$AllocationReloadListener.onReload(AllocationConfiguration) does not release lock on all exception paths At FairScheduler.java:on all exception paths At FairScheduler.java: [line 1651] Failed junit tests hadoop.yarn.server.resourcemanager.TestRMHA Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828352/YARN-3139.0.patch JIRA Issue YARN-3139 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 82f046cef257 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e80386d Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html unit https://builds.apache.org/job/PreCommit-YARN-Build/13166/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13166/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/13166/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13166/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          I just tried to use SLS to benchmark the performance after applied patches: YARN-3139/YARN-3140/YARN-3141, I created a mini framework to run the comparison tests: https://github.com/leftnoteasy/yarn_application_synthesizer.

          Because of some unknown issue, I cannot run the benchmark with FairScheduler, filed YARN-5654. Capacity scheduler runs fine.

          Overview of test data:

          • 3000+ applications, ~1000 applications running in parallel
          • 3000 NMs, each node configured 128G capacity
          • Size of containers ranges from 8G to 32G
          • Lifespan of containers ranges from 5 sec to 2 mins
          • Total 245,301 containers launched
          • Total time spent on the test is < 10 mins (can increase #apps / #containers to increase the total time), thus, there're around 500 containers allocated for each second.

          For original CS, time:

          real	8m51.774s
          user	8m41.998s
          sys	1m28.158s
          

          For CS with R/W locking changes:

          real	8m48.426s
          user	8m31.398s
          sys	1m29.861s
          

          Summary:

          • No regression in performance, didn't see deadlock happens.
          • No significant performance improvement either, because existing scheduler allocation is still in single thread.

          Hope this can with code review.

          cc: Jian He, Karthik Kambatla, Daniel Templeton, Vinod Kumar Vavilapalli, Jason Lowe.

          Show
          leftnoteasy Wangda Tan added a comment - I just tried to use SLS to benchmark the performance after applied patches: YARN-3139 / YARN-3140 / YARN-3141 , I created a mini framework to run the comparison tests: https://github.com/leftnoteasy/yarn_application_synthesizer . Because of some unknown issue, I cannot run the benchmark with FairScheduler, filed YARN-5654 . Capacity scheduler runs fine. Overview of test data: 3000+ applications, ~1000 applications running in parallel 3000 NMs, each node configured 128G capacity Size of containers ranges from 8G to 32G Lifespan of containers ranges from 5 sec to 2 mins Total 245,301 containers launched Total time spent on the test is < 10 mins (can increase #apps / #containers to increase the total time), thus, there're around 500 containers allocated for each second. For original CS, time: real 8m51.774s user 8m41.998s sys 1m28.158s For CS with R/W locking changes: real 8m48.426s user 8m31.398s sys 1m29.861s Summary: No regression in performance, didn't see deadlock happens. No significant performance improvement either, because existing scheduler allocation is still in single thread. Hope this can with code review. cc: Jian He , Karthik Kambatla , Daniel Templeton , Vinod Kumar Vavilapalli , Jason Lowe .
          Hide
          leftnoteasy Wangda Tan added a comment -

          The patch has dependency of YARN-3141, cancel path for now.

          Show
          leftnoteasy Wangda Tan added a comment - The patch has dependency of YARN-3141 , cancel path for now.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



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



          Subsystem Report/Notes
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828352/YARN-3139.0.patch
          JIRA Issue YARN-3139
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13102/console
          Powered by Apache Yetus 0.3.0 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 6s YARN-3139 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828352/YARN-3139.0.patch JIRA Issue YARN-3139 Console output https://builds.apache.org/job/PreCommit-YARN-Build/13102/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Attached first patch to run Jenkins.

          Show
          leftnoteasy Wangda Tan added a comment - Attached first patch to run Jenkins.
          Hide
          xinxianyin Xianyin Xin added a comment -

          Any progress? or any plans?

          Show
          xinxianyin Xianyin Xin added a comment - Any progress? or any plans?

            People

            • Assignee:
              leftnoteasy Wangda Tan
              Reporter:
              leftnoteasy Wangda Tan
            • Votes:
              0 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development