Uploaded image for project: 'Hadoop YARN'
  1. Hadoop YARN
  2. YARN-5956

Refactor ClientRMService to unify error handling across apis

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha2
    • Fix Version/s: 2.9.0, 3.0.0-alpha4
    • Component/s: resourcemanager
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Some refactoring can be done in ClientRMService.

      • Remove redundant variable declaration
      • Fill in missing javadocs
      • Proper variable access modifier
      • Fix some typos in method name and exception messages
      1. YARN-5956.01.patch
        10 kB
        Kai Sasaki
      2. YARN-5956.02.patch
        25 kB
        Kai Sasaki
      3. YARN-5956.03.patch
        25 kB
        Kai Sasaki
      4. YARN-5956.04.patch
        25 kB
        Kai Sasaki
      5. YARN-5956.05.patch
        23 kB
        Kai Sasaki
      6. YARN-5956.06.patch
        23 kB
        Kai Sasaki
      7. YARN-5956.07.patch
        23 kB
        Kai Sasaki
      8. YARN-5956.08.patch
        23 kB
        Kai Sasaki
      9. YARN-5956.09.patch
        26 kB
        Kai Sasaki
      10. YARN-5956.10.patch
        27 kB
        Kai Sasaki
      11. YARN-5956.11.patch
        27 kB
        Kai Sasaki
      12. YARN-5956.12.patch
        28 kB
        Kai Sasaki
      13. YARN-5956.13.patch
        28 kB
        Kai Sasaki
      14. YARN-5956.14.patch
        27 kB
        Kai Sasaki
      15. YARN-5956.15.patch
        27 kB
        Kai Sasaki
      16. YARN-5956-branch-2.01.patch
        25 kB
        Kai Sasaki

        Activity

        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 8m 52s trunk passed
        +1 compile 0m 40s trunk passed
        +1 checkstyle 0m 24s trunk passed
        +1 mvnsite 0m 48s trunk passed
        +1 mvneclipse 0m 26s trunk passed
        +1 findbugs 1m 14s trunk passed
        +1 javadoc 0m 26s trunk passed
        +1 mvninstall 0m 41s the patch passed
        +1 compile 0m 43s the patch passed
        +1 javac 0m 43s the patch passed
        -0 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 2 new + 31 unchanged - 2 fixed = 33 total (was 33)
        +1 mvnsite 0m 49s the patch passed
        +1 mvneclipse 0m 19s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 31s the patch passed
        +1 javadoc 0m 23s the patch passed
        -1 unit 42m 20s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 18s The patch does not generate ASF License warnings.
        61m 57s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841295/YARN-5956.01.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux fb30686453d3 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 / e0fa492
        Default Java 1.8.0_111
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14144/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/14144/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/14144/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/14144/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +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 52s trunk passed +1 compile 0m 40s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 48s trunk passed +1 mvneclipse 0m 26s trunk passed +1 findbugs 1m 14s trunk passed +1 javadoc 0m 26s trunk passed +1 mvninstall 0m 41s the patch passed +1 compile 0m 43s the patch passed +1 javac 0m 43s the patch passed -0 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 2 new + 31 unchanged - 2 fixed = 33 total (was 33) +1 mvnsite 0m 49s the patch passed +1 mvneclipse 0m 19s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 31s the patch passed +1 javadoc 0m 23s the patch passed -1 unit 42m 20s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 61m 57s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer   hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesDelegationTokens Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841295/YARN-5956.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux fb30686453d3 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 / e0fa492 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14144/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/14144/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/14144/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/14144/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        templedf Daniel Templeton added a comment -

        Thanks for the patch, Kai Sasaki! Looks good except that the @param and @return messages should start with lower case letters.

        Show
        templedf Daniel Templeton added a comment - Thanks for the patch, Kai Sasaki ! Looks good except that the @param and @return messages should start with lower case letters.
        Hide
        rohithsharma Rohith Sharma K S added a comment -

        As part of this JIRA, we can remove duplicate code which exist in most of the methods for checking UGI and getting application.
        Could we do something similar which has done in updateApplication***** methods like below? This reduces majority of duplicated code!!

           UserGroupInformation callerUGI =
                getCallerUgi(applicationId, AuditConstants.UPDATE_APP_TIMEOUTS);
            RMApp application = verifyUserAccessForRMApp(applicationId, callerUGI,
                AuditConstants.UPDATE_APP_TIMEOUTS);
        
        Show
        rohithsharma Rohith Sharma K S added a comment - As part of this JIRA, we can remove duplicate code which exist in most of the methods for checking UGI and getting application. Could we do something similar which has done in updateApplication***** methods like below? This reduces majority of duplicated code!! UserGroupInformation callerUGI = getCallerUgi(applicationId, AuditConstants.UPDATE_APP_TIMEOUTS); RMApp application = verifyUserAccessForRMApp(applicationId, callerUGI, AuditConstants.UPDATE_APP_TIMEOUTS);
        Hide
        sunilg Sunil G added a comment -

        Adding few more possible cleanups:

        • COMPLETED_APP_STATES and ACTIVE_APP_STATES states are locally defined here. This ideally has to come from RMAppImpl. There are few apis in RMAppImpl as of today, we could try to use RMApp#isAppInCompletedStates, isAppInFinalState etc. Its better if these are coming from RMApp itself.
        • signalToContainer javadoc could be corrected with param and return
        Show
        sunilg Sunil G added a comment - Adding few more possible cleanups: COMPLETED_APP_STATES and ACTIVE_APP_STATES states are locally defined here. This ideally has to come from RMAppImpl. There are few apis in RMAppImpl as of today, we could try to use RMApp#isAppInCompletedStates, isAppInFinalState etc. Its better if these are coming from RMApp itself. signalToContainer javadoc could be corrected with param and return
        Hide
        lewuathe Kai Sasaki added a comment -

        Daniel Templeton Rohith Sharma K S Sunil G
        Thanks for feedback. I'll update accordingly.

        Show
        lewuathe Kai Sasaki added a comment - Daniel Templeton Rohith Sharma K S Sunil G Thanks for feedback. I'll update accordingly.
        Hide
        lewuathe Kai Sasaki added a comment -

        I updated the patch but keeps ACTIVE_APP_STATES as it is.

        COMPLETED_APP_STATES and ACTIVE_APP_STATES states are locally defined here. This ideally has to come from RMAppImpl.

        It might be difficult to define ACTIVE_APP_STATES as public in RMAppImpl because the class is an implementation not an interface. Implementing an API like RMApp#isActiveState can be an option but it requires the change of interface. It cannot be done as refactoring I think. So keep ACTIVE_APP_STATES as it is can be a reasonable way. We should do that in another track if necessary.

        Show
        lewuathe Kai Sasaki added a comment - I updated the patch but keeps ACTIVE_APP_STATES as it is. COMPLETED_APP_STATES and ACTIVE_APP_STATES states are locally defined here. This ideally has to come from RMAppImpl. It might be difficult to define ACTIVE_APP_STATES as public in RMAppImpl because the class is an implementation not an interface. Implementing an API like RMApp#isActiveState can be an option but it requires the change of interface. It cannot be done as refactoring I think. So keep ACTIVE_APP_STATES as it is can be a reasonable way. We should do that in another track if necessary.
        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 8m 30s trunk passed
        +1 compile 0m 40s trunk passed
        +1 checkstyle 0m 24s trunk passed
        +1 mvnsite 0m 48s trunk passed
        +1 mvneclipse 0m 20s trunk passed
        +1 findbugs 1m 11s trunk passed
        +1 javadoc 0m 23s trunk passed
        +1 mvninstall 0m 37s the patch passed
        +1 compile 0m 38s the patch passed
        +1 javac 0m 38s the patch passed
        -0 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 5 new + 54 unchanged - 2 fixed = 59 total (was 56)
        +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 20s the patch passed
        -1 javadoc 0m 21s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 2 new + 913 unchanged - 0 fixed = 915 total (was 913)
        -1 unit 41m 14s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        59m 40s



        Reason Tests
        Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMService
          hadoop.yarn.server.resourcemanager.TestRMRestart
          hadoop.yarn.server.resourcemanager.TestKillApplicationWithRMHA
          hadoop.yarn.server.resourcemanager.TestApplicationACLs



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841630/YARN-5956.02.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 638e84387489 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / f885160
        Default Java 1.8.0_111
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14180/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        javadoc https://builds.apache.org/job/PreCommit-YARN-Build/14180/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14180/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/14180/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/14180/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 8m 30s trunk passed +1 compile 0m 40s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 48s trunk passed +1 mvneclipse 0m 20s trunk passed +1 findbugs 1m 11s trunk passed +1 javadoc 0m 23s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 0m 38s the patch passed +1 javac 0m 38s the patch passed -0 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 5 new + 54 unchanged - 2 fixed = 59 total (was 56) +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 20s the patch passed -1 javadoc 0m 21s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 2 new + 913 unchanged - 0 fixed = 915 total (was 913) -1 unit 41m 14s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 59m 40s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestClientRMService   hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.TestKillApplicationWithRMHA   hadoop.yarn.server.resourcemanager.TestApplicationACLs Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841630/YARN-5956.02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 638e84387489 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f885160 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14180/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/14180/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/14180/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/14180/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/14180/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sunilg Sunil G added a comment -

        Kai Sasaki

        few comments.

        1.

        	   
             if (application.isAppInCompletedStates()) {	      
                  RMAuditLogger.logSuccess(callerUGI.getShortUserName(),
        	          AuditConstants.FAIL_ATTEMPT_REQUEST, "ClientRMService"
        	          applicationId);
        	      return response;
          }	
        

        For failApplicationAttempt, this is not correct. We want to send normal response only for those apps whihc are in final states. But if app is in NEW_SAVING etc, we must throw exception. That is missed now.

        2. updateApplicationTimeouts has some unwanted code. Pls remove it.

        1634	    if (application.isAppInCompletedStates()) {
        1635	
        1636	    }
        
        Show
        sunilg Sunil G added a comment - Kai Sasaki few comments. 1. if (application.isAppInCompletedStates()) { RMAuditLogger.logSuccess(callerUGI.getShortUserName(), AuditConstants.FAIL_ATTEMPT_REQUEST, "ClientRMService" applicationId); return response; } For failApplicationAttempt , this is not correct. We want to send normal response only for those apps whihc are in final states. But if app is in NEW_SAVING etc, we must throw exception. That is missed now. 2. updateApplicationTimeouts has some unwanted code. Pls remove it. 1634 if (application.isAppInCompletedStates()) { 1635 1636 }
        Hide
        lewuathe Kai Sasaki added a comment -
         private static final EnumSet<RMAppState> COMPLETED_APP_STATES = EnumSet.of(
             RMAppState.FINISHED, RMAppState.FINISHING, RMAppState.FAILED,
             RMAppState.KILLED, RMAppState.FINAL_SAVING, RMAppState.KILLING);
        
          @Override
          public boolean isAppInCompletedStates() {
            RMAppState appState = getState();
            return appState == RMAppState.FINISHED || appState == RMAppState.FINISHING
                || appState == RMAppState.FAILED || appState == RMAppState.KILLED
                || appState == RMAppState.FINAL_SAVING
                || appState == RMAppState.KILLING;
          }
        

        Since COMPLETED_APP_STATE and isAppInCompletedStates looks same, it doesn't change the behaviour. Isn't it correct?

        Final states seems to define yet another states.

          public static boolean isAppInFinalState(RMApp rmApp) {
            RMAppState appState = ((RMAppImpl) rmApp).getRecoveredFinalState();
            if (appState == null) {
              appState = rmApp.getState();
            }
            return appState == RMAppState.FAILED || appState == RMAppState.FINISHED
                || appState == RMAppState.KILLED;
          }
        
        Show
        lewuathe Kai Sasaki added a comment - private static final EnumSet<RMAppState> COMPLETED_APP_STATES = EnumSet.of( RMAppState.FINISHED, RMAppState.FINISHING, RMAppState.FAILED, RMAppState.KILLED, RMAppState.FINAL_SAVING, RMAppState.KILLING); @Override public boolean isAppInCompletedStates() { RMAppState appState = getState(); return appState == RMAppState.FINISHED || appState == RMAppState.FINISHING || appState == RMAppState.FAILED || appState == RMAppState.KILLED || appState == RMAppState.FINAL_SAVING || appState == RMAppState.KILLING; } Since COMPLETED_APP_STATE and isAppInCompletedStates looks same, it doesn't change the behaviour. Isn't it correct? Final states seems to define yet another states. public static boolean isAppInFinalState(RMApp rmApp) { RMAppState appState = ((RMAppImpl) rmApp).getRecoveredFinalState(); if (appState == null ) { appState = rmApp.getState(); } return appState == RMAppState.FAILED || appState == RMAppState.FINISHED || appState == RMAppState.KILLED; }
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 12s 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 22s trunk passed
        +1 compile 0m 34s trunk passed
        +1 checkstyle 0m 22s trunk passed
        +1 mvnsite 0m 42s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 1m 8s trunk passed
        +1 javadoc 0m 22s trunk passed
        +1 mvninstall 0m 33s the patch passed
        +1 compile 0m 32s the patch passed
        +1 javac 0m 32s the patch passed
        +1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 55 unchanged - 2 fixed = 55 total (was 57)
        +1 mvnsite 0m 36s 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 20s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 2 new + 913 unchanged - 0 fixed = 915 total (was 913)
        -1 unit 42m 48s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        59m 8s



        Reason Tests
        Failed junit tests hadoop.yarn.server.resourcemanager.TestApplicationACLs
          hadoop.yarn.server.resourcemanager.TestKillApplicationWithRMHA
          hadoop.yarn.server.resourcemanager.TestRMRestart
          hadoop.yarn.server.resourcemanager.TestClientRMService



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842345/YARN-5956.03.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 47dc1fd4c82e 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 / 0ef7961
        Default Java 1.8.0_111
        findbugs v3.0.0
        javadoc https://builds.apache.org/job/PreCommit-YARN-Build/14224/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14224/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/14224/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/14224/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s 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 22s trunk passed +1 compile 0m 34s trunk passed +1 checkstyle 0m 22s trunk passed +1 mvnsite 0m 42s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 8s trunk passed +1 javadoc 0m 22s trunk passed +1 mvninstall 0m 33s the patch passed +1 compile 0m 32s the patch passed +1 javac 0m 32s the patch passed +1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 55 unchanged - 2 fixed = 55 total (was 57) +1 mvnsite 0m 36s 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 20s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 2 new + 913 unchanged - 0 fixed = 915 total (was 913) -1 unit 42m 48s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 59m 8s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestApplicationACLs   hadoop.yarn.server.resourcemanager.TestKillApplicationWithRMHA   hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.TestClientRMService Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842345/YARN-5956.03.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 47dc1fd4c82e 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 / 0ef7961 Default Java 1.8.0_111 findbugs v3.0.0 javadoc https://builds.apache.org/job/PreCommit-YARN-Build/14224/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/14224/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/14224/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/14224/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        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 6m 50s trunk passed
        +1 compile 0m 32s trunk passed
        +1 checkstyle 0m 21s trunk passed
        +1 mvnsite 0m 39s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 0m 57s trunk passed
        +1 javadoc 0m 21s trunk passed
        +1 mvninstall 0m 30s the patch passed
        +1 compile 0m 30s the patch passed
        +1 javac 0m 30s the patch passed
        +1 checkstyle 0m 18s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 55 unchanged - 2 fixed = 55 total (was 57)
        +1 mvnsite 0m 36s the patch passed
        +1 mvneclipse 0m 15s 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 38m 40s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        54m 0s



        Reason Tests
        Failed junit tests hadoop.yarn.server.resourcemanager.TestApplicationACLs
          hadoop.yarn.server.resourcemanager.TestClientRMService
          hadoop.yarn.server.resourcemanager.TestKillApplicationWithRMHA
          hadoop.yarn.server.resourcemanager.TestRMRestart



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842706/YARN-5956.04.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 33539cb05ba7 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 4c38f11
        Default Java 1.8.0_111
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14254/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/14254/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/14254/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 6m 50s trunk passed +1 compile 0m 32s trunk passed +1 checkstyle 0m 21s trunk passed +1 mvnsite 0m 39s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 57s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 30s the patch passed +1 compile 0m 30s the patch passed +1 javac 0m 30s the patch passed +1 checkstyle 0m 18s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 55 unchanged - 2 fixed = 55 total (was 57) +1 mvnsite 0m 36s the patch passed +1 mvneclipse 0m 15s 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 38m 40s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 54m 0s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestApplicationACLs   hadoop.yarn.server.resourcemanager.TestClientRMService   hadoop.yarn.server.resourcemanager.TestKillApplicationWithRMHA   hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842706/YARN-5956.04.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 33539cb05ba7 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 4c38f11 Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/14254/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/14254/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/14254/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sunilg Sunil G added a comment -

        Since COMPLETED_APP_STATE and isAppInCompletedStates looks same.

        Yes.. we could use this.

        Show
        sunilg Sunil G added a comment - Since COMPLETED_APP_STATE and isAppInCompletedStates looks same. Yes.. we could use this.
        Hide
        sunilg Sunil G added a comment -

        Test case failures looks related. Kai Sasaki, could you please take a look.

        Show
        sunilg Sunil G added a comment - Test case failures looks related. Kai Sasaki , could you please take a look.
        Hide
        lewuathe Kai Sasaki added a comment -

        Sure, I'll take a look. Thanks!

        Show
        lewuathe Kai Sasaki added a comment - Sure, I'll take a look. 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 6m 51s trunk passed
        +1 compile 0m 34s trunk passed
        +1 checkstyle 0m 21s trunk passed
        +1 mvnsite 0m 40s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 0m 59s trunk passed
        +1 javadoc 0m 22s 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 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 55 unchanged - 2 fixed = 55 total (was 57)
        +1 mvnsite 0m 37s the patch passed
        +1 mvneclipse 0m 15s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 7s the patch passed
        +1 javadoc 0m 18s the patch passed
        -1 unit 43m 32s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        59m 11s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842799/YARN-5956.05.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 460557161fae 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 / 4c38f11
        Default Java 1.8.0_111
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14255/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/14255/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/14255/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 6m 51s trunk passed +1 compile 0m 34s trunk passed +1 checkstyle 0m 21s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 59s trunk passed +1 javadoc 0m 22s 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 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 55 unchanged - 2 fixed = 55 total (was 57) +1 mvnsite 0m 37s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 7s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 43m 32s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 59m 11s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842799/YARN-5956.05.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 460557161fae 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 / 4c38f11 Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/14255/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/14255/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/14255/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        lewuathe Kai Sasaki added a comment -

        Sunil G Test failure of TestRMRestart cannot be reproduced on local. Is it intermittent failure?

        Show
        lewuathe Kai Sasaki added a comment - Sunil G Test failure of TestRMRestart cannot be reproduced on local. Is it intermittent failure?
        Hide
        sunilg Sunil G added a comment -

        Yes. This test failure is not related.
        Generally patch looks fine for me. I will wait for Rohith Sharma K S and Daniel Templeton if they have some more comments. Thank you.

        Show
        sunilg Sunil G added a comment - Yes. This test failure is not related. Generally patch looks fine for me. I will wait for Rohith Sharma K S and Daniel Templeton if they have some more comments. Thank you.
        Hide
        sunilg Sunil G added a comment -

        I went through patch one more time.

        Some doubts:
        1. In checkAccess java doc comment, you mentioned uhe user. Could you please help to use real name instead of abbreviation (I could not understand this )
        2. If we could pass ApplicationAccessType also to verifyUserAccessForRMApp, we can also have this code used in getApplicationReport. Ideally we should be able to use verifyUserAccessForRMApp and getCallerUgi in all places to avoid common code duplicated. Is that possible?

        Show
        sunilg Sunil G added a comment - I went through patch one more time. Some doubts: 1. In checkAccess java doc comment, you mentioned uhe user . Could you please help to use real name instead of abbreviation (I could not understand this ) 2. If we could pass ApplicationAccessType also to verifyUserAccessForRMApp , we can also have this code used in getApplicationReport . Ideally we should be able to use verifyUserAccessForRMApp and getCallerUgi in all places to avoid common code duplicated. Is that possible?
        Hide
        lewuathe Kai Sasaki added a comment - - edited

        Sunil G Thanks for checking again.

        In checkAccess java doc comment, you mentioned uhe user

        Sorry that's just typo. I fixed.

        Ideally we should be able to use verifyUserAccessForRMApp and getCallerUgi in all places to avoid common code duplicated.

        Yes, I think so. But it's difficult to do here because error message would be changed if verifyUserAccessForRMApp is used all places.
        Though we need to change the test code, it's beyond the refactoring. So keeping as it was where the error message would be changed otherwise might be better.

        Show
        lewuathe Kai Sasaki added a comment - - edited Sunil G Thanks for checking again. In checkAccess java doc comment, you mentioned uhe user Sorry that's just typo. I fixed. Ideally we should be able to use verifyUserAccessForRMApp and getCallerUgi in all places to avoid common code duplicated. Yes, I think so. But it's difficult to do here because error message would be changed if verifyUserAccessForRMApp is used all places. Though we need to change the test code, it's beyond the refactoring. So keeping as it was where the error message would be changed otherwise might be better.
        Hide
        sunilg Sunil G added a comment -

        Thanks Kai Sasaki. After checking a bit more about verifyUserAccessForRMApp, looks like it also call checkAccess and audit logs if any failure (access control violations) are there. And many existing caller of this verifyUserAccessForRMApp is again calling checkAccess independently. For eg: failApplicationAttempt. It invokes verifyUserAccessForRMApp and also calls checkAccess later. This is not correct.

        For few apis, we can still call verifyUserAccessForRMApp and remove checkAccess. But for some apis, it may not be possible. Could you please check the same.

        Show
        sunilg Sunil G added a comment - Thanks Kai Sasaki . After checking a bit more about verifyUserAccessForRMApp , looks like it also call checkAccess and audit logs if any failure (access control violations) are there. And many existing caller of this verifyUserAccessForRMApp is again calling checkAccess independently. For eg: failApplicationAttempt . It invokes verifyUserAccessForRMApp and also calls checkAccess later. This is not correct. For few apis, we can still call verifyUserAccessForRMApp and remove checkAccess . But for some apis, it may not be possible. Could you please check the same.
        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 14m 39s trunk passed
        +1 compile 0m 41s trunk passed
        +1 checkstyle 0m 24s trunk passed
        +1 mvnsite 0m 40s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 1m 13s trunk passed
        +1 javadoc 0m 25s trunk passed
        +1 mvninstall 0m 38s the patch passed
        +1 compile 0m 39s the patch passed
        +1 javac 0m 39s the patch passed
        +1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 53 unchanged - 2 fixed = 53 total (was 55)
        +1 mvnsite 0m 41s the patch passed
        +1 mvneclipse 0m 17s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 13s the patch passed
        +1 javadoc 0m 23s the patch passed
        -1 unit 42m 4s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        66m 34s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12847536/YARN-5956.07.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 81c4fd5c846b 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / ed09c14
        Default Java 1.8.0_111
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14660/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/14660/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/14660/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 14m 39s trunk passed +1 compile 0m 41s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 13s trunk passed +1 javadoc 0m 25s trunk passed +1 mvninstall 0m 38s the patch passed +1 compile 0m 39s the patch passed +1 javac 0m 39s the patch passed +1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 53 unchanged - 2 fixed = 53 total (was 55) +1 mvnsite 0m 41s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 13s the patch passed +1 javadoc 0m 23s the patch passed -1 unit 42m 4s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 66m 34s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12847536/YARN-5956.07.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 81c4fd5c846b 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ed09c14 Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/14660/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/14660/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/14660/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 33s 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 16m 27s trunk passed
        +1 compile 0m 44s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 0m 47s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 1m 24s trunk passed
        +1 javadoc 0m 28s trunk passed
        +1 mvninstall 0m 44s the patch passed
        +1 compile 0m 42s the patch passed
        +1 javac 0m 42s the patch passed
        +1 checkstyle 0m 24s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 54 unchanged - 2 fixed = 54 total (was 56)
        +1 mvnsite 0m 45s the patch passed
        +1 mvneclipse 0m 15s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 36s the patch passed
        +1 javadoc 0m 25s the patch passed
        -1 unit 41m 48s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        69m 33s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12851392/YARN-5956.08.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 8bc479779109 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 9dbfab1
        Default Java 1.8.0_121
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14846/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/14846/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/14846/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 33s 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 16m 27s trunk passed +1 compile 0m 44s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 47s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 24s trunk passed +1 javadoc 0m 28s trunk passed +1 mvninstall 0m 44s the patch passed +1 compile 0m 42s the patch passed +1 javac 0m 42s the patch passed +1 checkstyle 0m 24s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 54 unchanged - 2 fixed = 54 total (was 56) +1 mvnsite 0m 45s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 36s the patch passed +1 javadoc 0m 25s the patch passed -1 unit 41m 48s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 69m 33s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12851392/YARN-5956.08.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 8bc479779109 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 9dbfab1 Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/14846/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/14846/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/14846/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sunilg Sunil G added a comment - - edited

        HI Kai Sasaki

        Thanks for the patch. I have some doubts in this patch. GetApplicationReportResponse getApplicationReport is now invoking getCallerUgi and verifyUserAccessForRMApp. Internally verifyUserAccessForRMApp is already doing checkAccess check. But its still invoked from getApplicationReport.

        I think same comment applies to few more apis as well. Could you please check the same.

        Show
        sunilg Sunil G added a comment - - edited HI Kai Sasaki Thanks for the patch. I have some doubts in this patch. GetApplicationReportResponse getApplicationReport is now invoking getCallerUgi and verifyUserAccessForRMApp . Internally verifyUserAccessForRMApp is already doing checkAccess check. But its still invoked from getApplicationReport . I think same comment applies to few more apis as well. Could you please check the same.
        Hide
        lewuathe Kai Sasaki added a comment -

        Sunil G Sorry for late. I checked redundant checkAccess call and updated. Failed tests seems not related to the patch. I confirmed it passed at local.
        Could you check again please?

        Show
        lewuathe Kai Sasaki added a comment - Sunil G Sorry for late. I checked redundant checkAccess call and updated. Failed tests seems not related to the patch. I confirmed it passed at local. Could you check again please?
        Hide
        sunilg Sunil G added a comment -

        Thanks Kai Sasaki.

        Were u mentioning about YARN-5956.08.patch. IN that patch also, verifyUserAccessForRMApp internally calling checkAccess, correct? Am I missing something.

        Show
        sunilg Sunil G added a comment - Thanks Kai Sasaki . Were u mentioning about YARN-5956 .08.patch . IN that patch also, verifyUserAccessForRMApp internally calling checkAccess , correct? Am I missing something.
        Hide
        lewuathe Kai Sasaki added a comment -

        Sunil G Thanks.

        GetApplicationReportResponse getApplicationReport is now invoking getCallerUgi

        Is it correct? getCallerUgi does not seem to be called in getApplicationReport.

        verifyUserAccessForRMApp internally calling checkAccess, correct?

        Yes, verifyUserAccessForUgi still need to call checkAccess because it throws exception with specific error message. But I checked and removed checkAccess like failApplicationAttempt since it does not do something other than logging.

        Show
        lewuathe Kai Sasaki added a comment - Sunil G Thanks. GetApplicationReportResponse getApplicationReport is now invoking getCallerUgi Is it correct? getCallerUgi does not seem to be called in getApplicationReport . verifyUserAccessForRMApp internally calling checkAccess, correct? Yes, verifyUserAccessForUgi still need to call checkAccess because it throws exception with specific error message. But I checked and removed checkAccess like failApplicationAttempt since it does not do something other than logging.
        Hide
        sunilg Sunil G added a comment -

        I think there is a bit confusion. Lemme give a detailed concern from my end.

        verifyUserAccessForRMApp is defined as below.

          private RMApp verifyUserAccessForRMApp(ApplicationId applicationId,
              UserGroupInformation callerUGI, String operation) throws YarnException {
            RMApp application = this.rmContext.getRMApps().get(applicationId);
            if (application == null) {
              RMAuditLogger.logFailure(callerUGI.getUserName(), operation, "UNKNOWN",
                  "ClientRMService",
                  "Trying to " + operation + " of an absent application",
                  applicationId);
              throw new ApplicationNotFoundException("Trying to " + operation
                  + " of an absent application " + applicationId);
            }
        
            if (!checkAccess(callerUGI, application.getUser(),
                ApplicationAccessType.MODIFY_APP, application)) {
              RMAuditLogger.logFailure(callerUGI.getShortUserName(), operation,
                  "User doesn't have permissions to "
                      + ApplicationAccessType.MODIFY_APP.toString(),
                  "ClientRMService", AuditConstants.UNAUTHORIZED_USER, applicationId);
              throw RPCUtil.getRemoteException(new AccessControlException("User "
                  + callerUGI.getShortUserName() + " cannot perform operation "
                  + ApplicationAccessType.MODIFY_APP.name() + " on " + applicationId));
            }
            return application;
          }
        

        Now lets see each of the api.
        1. In getApplicationAttemptReport, getApplicationAttempts, getContainerReport and getContaines, below code is added.

            UserGroupInformation callerUGI = getCallerUgi(applicationId,
                AuditConstants.GET_APP_ATTEMPT_REPORT);
            RMApp application = verifyUserAccessForRMApp(applicationId, callerUGI,
                AuditConstants.GET_APP_ATTEMPT_REPORT);
        
            boolean allowAccess = checkAccess(callerUGI, application.getUser(),
                ApplicationAccessType.VIEW_APP, application);
        

        Inside verifyUserAccessForRMApp, we do a checkAccess for MODIFY_APP. Then in each of this apis ,we do one more time for VIEW_APP (see 3rd line in above code snippet). This may cause issue because for any getter api, we just need VIEW_APP permission. This change enforce a MODIFY_APP and will break many valid use cases and breaks compatibility.

        2. Change done to failApplicationAttempt looks correct to me as you avoided a repeated call to checkAccess

        Show
        sunilg Sunil G added a comment - I think there is a bit confusion. Lemme give a detailed concern from my end. verifyUserAccessForRMApp is defined as below. private RMApp verifyUserAccessForRMApp(ApplicationId applicationId, UserGroupInformation callerUGI, String operation) throws YarnException { RMApp application = this .rmContext.getRMApps().get(applicationId); if (application == null ) { RMAuditLogger.logFailure(callerUGI.getUserName(), operation, "UNKNOWN" , "ClientRMService" , "Trying to " + operation + " of an absent application" , applicationId); throw new ApplicationNotFoundException( "Trying to " + operation + " of an absent application " + applicationId); } if (!checkAccess(callerUGI, application.getUser(), ApplicationAccessType.MODIFY_APP, application)) { RMAuditLogger.logFailure(callerUGI.getShortUserName(), operation, "User doesn't have permissions to " + ApplicationAccessType.MODIFY_APP.toString(), "ClientRMService" , AuditConstants.UNAUTHORIZED_USER, applicationId); throw RPCUtil.getRemoteException( new AccessControlException( "User " + callerUGI.getShortUserName() + " cannot perform operation " + ApplicationAccessType.MODIFY_APP.name() + " on " + applicationId)); } return application; } Now lets see each of the api. 1. In getApplicationAttemptReport , getApplicationAttempts , getContainerReport and getContaines , below code is added. UserGroupInformation callerUGI = getCallerUgi(applicationId, AuditConstants.GET_APP_ATTEMPT_REPORT); RMApp application = verifyUserAccessForRMApp(applicationId, callerUGI, AuditConstants.GET_APP_ATTEMPT_REPORT); boolean allowAccess = checkAccess(callerUGI, application.getUser(), ApplicationAccessType.VIEW_APP, application); Inside verifyUserAccessForRMApp , we do a checkAccess for MODIFY_APP . Then in each of this apis ,we do one more time for VIEW_APP (see 3rd line in above code snippet). This may cause issue because for any getter api, we just need VIEW_APP permission. This change enforce a MODIFY_APP and will break many valid use cases and breaks compatibility. 2. Change done to failApplicationAttempt looks correct to me as you avoided a repeated call to checkAccess
        Hide
        lewuathe Kai Sasaki added a comment -

        This may cause issue because for any getter api, we just need VIEW_APP permission.

        Sunil G Thank you so much for clear explanation. I understand your concern. I think that passing ApplicationAccessType to verifyUserAccessForRMApp might be a solution. But considering it's refactoring, we should avoid to change method behaviour further. How about keeping the code calling verifyUserAccessForRMApp as it is?

        Show
        lewuathe Kai Sasaki added a comment - This may cause issue because for any getter api, we just need VIEW_APP permission. Sunil G Thank you so much for clear explanation. I understand your concern. I think that passing ApplicationAccessType to verifyUserAccessForRMApp might be a solution. But considering it's refactoring, we should avoid to change method behaviour further. How about keeping the code calling verifyUserAccessForRMApp as it is?
        Hide
        sunilg Sunil G added a comment -

        I think that passing ApplicationAccessType to verifyUserAccessForRMApp might be a solution.

        Its perfectly fine to do this. For getter apis, you can pass VIEW_APP and for other apis like forceKill etc, you could pass MODIFY_APP to verifyUserAccessForRMApp

        Show
        sunilg Sunil G added a comment - I think that passing ApplicationAccessType to verifyUserAccessForRMApp might be a solution. Its perfectly fine to do this. For getter apis, you can pass VIEW_APP and for other apis like forceKill etc, you could pass MODIFY_APP to verifyUserAccessForRMApp
        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 12m 53s trunk passed
        +1 compile 0m 35s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 0m 35s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 1m 2s trunk passed
        +1 javadoc 0m 22s 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 23s the patch passed
        +1 mvnsite 0m 34s the patch passed
        +1 mvneclipse 0m 13s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 11s the patch passed
        +1 javadoc 0m 20s the patch passed
        -1 unit 43m 25s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        65m 8s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12852824/YARN-5956.09.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux adcd56acda3d 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 859bd15
        Default Java 1.8.0_121
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14959/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/14959/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/14959/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 12m 53s trunk passed +1 compile 0m 35s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 35s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 2s trunk passed +1 javadoc 0m 22s 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 23s the patch passed +1 mvnsite 0m 34s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 11s the patch passed +1 javadoc 0m 20s the patch passed -1 unit 43m 25s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 65m 8s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12852824/YARN-5956.09.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux adcd56acda3d 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 859bd15 Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/14959/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/14959/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/14959/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 12s 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 12m 48s trunk passed
        +1 compile 0m 33s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 0m 37s trunk passed
        +1 mvneclipse 0m 16s trunk passed
        +1 findbugs 1m 10s trunk passed
        +1 javadoc 0m 23s trunk passed
        +1 mvninstall 0m 35s the patch passed
        +1 compile 0m 32s the patch passed
        +1 javac 0m 32s the patch passed
        +1 checkstyle 0m 24s the patch passed
        +1 mvnsite 0m 35s the patch passed
        +1 mvneclipse 0m 13s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 14s the patch passed
        +1 javadoc 0m 20s the patch passed
        -1 unit 41m 22s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        63m 26s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12854220/YARN-5956.10.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux b77627a948aa 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / a207aa9
        Default Java 1.8.0_121
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/15059/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/15059/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/15059/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s 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 12m 48s trunk passed +1 compile 0m 33s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 37s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 1m 10s trunk passed +1 javadoc 0m 23s trunk passed +1 mvninstall 0m 35s the patch passed +1 compile 0m 32s the patch passed +1 javac 0m 32s the patch passed +1 checkstyle 0m 24s the patch passed +1 mvnsite 0m 35s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 14s the patch passed +1 javadoc 0m 20s the patch passed -1 unit 41m 22s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 63m 26s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12854220/YARN-5956.10.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b77627a948aa 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / a207aa9 Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/15059/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/15059/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/15059/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sunilg Sunil G added a comment -

        Thanks Kai Sasaki for updating the patch. We are almost there.

        A major comment, In getApplicationAttemptReport, getApplicationAttempts, getContainerReport and getContaines, below code is added.

        	    UserGroupInformation callerUGI = getCallerUgi(appId,
        	        AuditConstants.GET_CONTAINER_REPORT);
        	    RMApp application = verifyUserAccessForRMApp(appId, callerUGI,
        	        AuditConstants.GET_CONTAINER_REPORT, ApplicationAccessType.VIEW_APP);
        
        	    boolean allowAccess = checkAccess(callerUGI, application.getUser(),
        	        ApplicationAccessType.VIEW_APP, application);
        

        Here verifyUserAccessForRMApp is already checking checkAccess for VIEW_APP internally. SO we can avoid duplicate check in all these getters. Then some more cleanup could be done as allowAccess will be true after the new verifyUserAccessForRMApp call. Do you agree?

        Show
        sunilg Sunil G added a comment - Thanks Kai Sasaki for updating the patch. We are almost there. A major comment, In getApplicationAttemptReport, getApplicationAttempts, getContainerReport and getContaines, below code is added. UserGroupInformation callerUGI = getCallerUgi(appId, AuditConstants.GET_CONTAINER_REPORT); RMApp application = verifyUserAccessForRMApp(appId, callerUGI, AuditConstants.GET_CONTAINER_REPORT, ApplicationAccessType.VIEW_APP); boolean allowAccess = checkAccess(callerUGI, application.getUser(), ApplicationAccessType.VIEW_APP, application); Here verifyUserAccessForRMApp is already checking checkAccess for VIEW_APP internally. SO we can avoid duplicate check in all these getters. Then some more cleanup could be done as allowAccess will be true after the new verifyUserAccessForRMApp call. Do you agree?
        Hide
        lewuathe Kai Sasaki added a comment - - edited

        Sunil G Thanks for checking again. I agree with you. I think checkAccess and exception handling inside verifyUserAccessForRMApp can be exported as other function. But it changes the behaviour of verifyUserAccessForRMApp. Do you think we can do as refactoring? I think it might be reasonable do it as separate because it changes the function behaviour and might need test code modifications too.

        Show
        lewuathe Kai Sasaki added a comment - - edited Sunil G Thanks for checking again. I agree with you. I think checkAccess and exception handling inside verifyUserAccessForRMApp can be exported as other function. But it changes the behaviour of verifyUserAccessForRMApp . Do you think we can do as refactoring ? I think it might be reasonable do it as separate because it changes the function behaviour and might need test code modifications too.
        Hide
        sunilg Sunil G added a comment -

        Thanks Kai Sasaki
        With current patch, checkAccess inside verifyUserAccessForRMApp will hit first and an AccessControlException will be thrown.

        I checked more on this, and there are some potential issues with this approach.

        1. getApplicationReport: For this api, if VIEW_APP checkAccess fails, exception wont be thrown. Rather user will get back a trimmed version of app report. No exception thrown here on checkAccess failure.
        2. getApplicationAttemptReport: YarnException will be thrown back if VIEW_APP checkAcces fails.
        3. getContainerReport: YarnException will be thrown back if VIEW_APP checkAcces fails.
        4. getContainers: YarnException will be thrown back if VIEW_APP checkAcces fails.
        5. failApplicationAttempt is throwing AccessControlException, hence the changes in patch is fine for this api.
        6. moveApplicationAcrossQueues is throwing AccessControlException, hence the changes in patch is fine for this api.

        So I think we have issues or behavior change for first 4 apis w.r.t current patch. A simple fix would be to inform verifyUserAccessForRMApp to NOT to do checkAccess with a boolean trigger and current checkAccess in all these apis could be kept as it is.

        Show
        sunilg Sunil G added a comment - Thanks Kai Sasaki With current patch, checkAccess inside verifyUserAccessForRMApp will hit first and an AccessControlException will be thrown. I checked more on this, and there are some potential issues with this approach. getApplicationReport : For this api, if VIEW_APP checkAccess fails, exception wont be thrown. Rather user will get back a trimmed version of app report. No exception thrown here on checkAccess failure. getApplicationAttemptReport : YarnException will be thrown back if VIEW_APP checkAcces fails. getContainerReport : YarnException will be thrown back if VIEW_APP checkAcces fails. getContainers : YarnException will be thrown back if VIEW_APP checkAcces fails. failApplicationAttempt is throwing AccessControlException , hence the changes in patch is fine for this api. moveApplicationAcrossQueues is throwing AccessControlException , hence the changes in patch is fine for this api. So I think we have issues or behavior change for first 4 apis w.r.t current patch. A simple fix would be to inform verifyUserAccessForRMApp to NOT to do checkAccess with a boolean trigger and current checkAccess in all these apis could be kept as it is.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 23s 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 12m 37s trunk passed
        +1 compile 0m 31s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 0m 33s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 0m 58s trunk passed
        +1 javadoc 0m 20s trunk passed
        +1 mvninstall 0m 30s the patch passed
        +1 compile 0m 28s the patch passed
        +1 javac 0m 28s the patch passed
        -0 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 8 new + 57 unchanged - 5 fixed = 65 total (was 62)
        +1 mvnsite 0m 30s the patch passed
        +1 mvneclipse 0m 11s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        -1 findbugs 1m 6s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 4 new + 0 unchanged - 0 fixed = 4 total (was 0)
        +1 javadoc 0m 19s the patch passed
        +1 unit 40m 34s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        61m 51s



        Reason Tests
        FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
          Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttemptReport(GetApplicationAttemptReportRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttemptReport(GetApplicationAttemptReportRequest) Redundant null check at ClientRMService.java:[line 390]
          Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttempts(GetApplicationAttemptsRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttempts(GetApplicationAttemptsRequest) Redundant null check at ClientRMService.java:[line 427]
          Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainerReport(GetContainerReportRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainerReport(GetContainerReportRequest) Redundant null check at ClientRMService.java:[line 472]
          Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainers(GetContainersRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainers(GetContainersRequest) Redundant null check at ClientRMService.java:[line 519]



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855632/YARN-5956.11.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 51866db2f55c 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 4e14ead
        Default Java 1.8.0_121
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15133/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/15133/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15133/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/15133/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 23s 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 12m 37s trunk passed +1 compile 0m 31s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 33s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 0m 58s trunk passed +1 javadoc 0m 20s trunk passed +1 mvninstall 0m 30s the patch passed +1 compile 0m 28s the patch passed +1 javac 0m 28s the patch passed -0 checkstyle 0m 22s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 8 new + 57 unchanged - 5 fixed = 65 total (was 62) +1 mvnsite 0m 30s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 6s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 4 new + 0 unchanged - 0 fixed = 4 total (was 0) +1 javadoc 0m 19s the patch passed +1 unit 40m 34s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 61m 51s Reason Tests FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager   Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttemptReport(GetApplicationAttemptReportRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttemptReport(GetApplicationAttemptReportRequest) Redundant null check at ClientRMService.java: [line 390]   Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttempts(GetApplicationAttemptsRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getApplicationAttempts(GetApplicationAttemptsRequest) Redundant null check at ClientRMService.java: [line 427]   Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainerReport(GetContainerReportRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainerReport(GetContainerReportRequest) Redundant null check at ClientRMService.java: [line 472]   Redundant nullcheck of application, which is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainers(GetContainersRequest) Redundant null check at ClientRMService.java:is known to be non-null in org.apache.hadoop.yarn.server.resourcemanager.ClientRMService.getContainers(GetContainersRequest) Redundant null check at ClientRMService.java: [line 519] Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855632/YARN-5956.11.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 51866db2f55c 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 4e14ead Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15133/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/15133/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15133/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/15133/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sunilg Sunil G added a comment -

        Thanks Kai Sasaki.
        Generally looks fine. Do you mind checking whether findbugs are valid or not?

        Show
        sunilg Sunil G added a comment - Thanks Kai Sasaki . Generally looks fine. Do you mind checking whether findbugs are valid or not?
        Hide
        lewuathe Kai Sasaki added a comment -

        Sunil G Sure, let me check findbugs and check style issues.

        Show
        lewuathe Kai Sasaki added a comment - Sunil G Sure, let me check findbugs and check style issues.
        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 12m 53s trunk passed
        +1 compile 0m 34s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 0m 38s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 1m 1s trunk passed
        +1 javadoc 0m 21s trunk passed
        +1 mvninstall 0m 31s the patch passed
        +1 compile 0m 30s the patch passed
        +1 javac 0m 30s the patch passed
        -0 checkstyle 0m 24s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 2 new + 57 unchanged - 5 fixed = 59 total (was 62)
        +1 mvnsite 0m 32s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 7s the patch passed
        +1 javadoc 0m 19s the patch passed
        -1 unit 40m 17s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        61m 53s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855876/YARN-5956.12.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux a3986906fa4e 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / e58fc76
        Default Java 1.8.0_121
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15154/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/15154/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/15154/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/15154/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 12m 53s trunk passed +1 compile 0m 34s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 1s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 31s the patch passed +1 compile 0m 30s the patch passed +1 javac 0m 30s the patch passed -0 checkstyle 0m 24s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 2 new + 57 unchanged - 5 fixed = 59 total (was 62) +1 mvnsite 0m 32s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 7s the patch passed +1 javadoc 0m 19s the patch passed -1 unit 40m 17s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 61m 53s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855876/YARN-5956.12.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a3986906fa4e 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e58fc76 Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15154/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/15154/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/15154/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/15154/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        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 12m 37s trunk passed
        +1 compile 0m 31s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 0m 33s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 0m 59s trunk passed
        +1 javadoc 0m 20s trunk passed
        +1 mvninstall 0m 29s the patch passed
        +1 compile 0m 29s the patch passed
        +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 0 new + 57 unchanged - 5 fixed = 57 total (was 62)
        +1 mvnsite 0m 30s the patch passed
        +1 mvneclipse 0m 11s 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 39m 35s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        60m 31s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856611/YARN-5956.13.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 335674f9d5d5 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / f597f4c
        Default Java 1.8.0_121
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/15189/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/15189/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/15189/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 12m 37s trunk passed +1 compile 0m 31s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 33s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 0m 59s trunk passed +1 javadoc 0m 20s trunk passed +1 mvninstall 0m 29s the patch passed +1 compile 0m 29s the patch passed +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 0 new + 57 unchanged - 5 fixed = 57 total (was 62) +1 mvnsite 0m 30s the patch passed +1 mvneclipse 0m 11s 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 39m 35s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 60m 31s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856611/YARN-5956.13.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 335674f9d5d5 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f597f4c Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/15189/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/15189/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/15189/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        lewuathe Kai Sasaki added a comment -

        Sunil G Thanks. I updated the patch. The failed test seems fails intermittently. I confirmed it passed at local.
        Could you check again this?

        Show
        lewuathe Kai Sasaki added a comment - Sunil G Thanks. I updated the patch. The failed test seems fails intermittently. I confirmed it passed at local. Could you check again this?
        Hide
        sunilg Sunil G added a comment -

        Latest patch looks fine for me. I can commit the patch tomorrow if there are no objections.
        Pinging Daniel Templeton Rohith Sharma K S.

        Show
        sunilg Sunil G added a comment - Latest patch looks fine for me. I can commit the patch tomorrow if there are no objections. Pinging Daniel Templeton Rohith Sharma K S .
        Hide
        rohithsharma Rohith Sharma K S added a comment -

        Overall patch looks good..

        One comment is the patch is reverting YARN-6211 for updateApplicationPriority. Could you keep as-is this change?

        Show
        rohithsharma Rohith Sharma K S added a comment - Overall patch looks good.. One comment is the patch is reverting YARN-6211 for updateApplicationPriority. Could you keep as-is this change?
        Hide
        sunilg Sunil G added a comment -

        Yes Rohith Sharma K S, Thanks for pointing out. in moveApplicationAcrossQueues and {[updateApplicationPriority}}, we should pass application.getApplicationId() instead of applicationId.

        Show
        sunilg Sunil G added a comment - Yes Rohith Sharma K S , Thanks for pointing out. in moveApplicationAcrossQueues and {[updateApplicationPriority}}, we should pass application.getApplicationId() instead of applicationId.
        Hide
        lewuathe Kai Sasaki added a comment -

        Rohith Sharma K S Sunil G Sorry I overlooked and thank you so much for checking. I updated the patch accordingly.

        Show
        lewuathe Kai Sasaki added a comment - Rohith Sharma K S Sunil G Sorry I overlooked and thank you so much for checking. I updated the patch accordingly.
        Hide
        sunilg Sunil G added a comment -

        Kicking jenkins again..

        Show
        sunilg Sunil G added a comment - Kicking jenkins again..
        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 12m 35s trunk passed
        +1 compile 0m 31s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 0m 33s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 0m 58s trunk passed
        +1 javadoc 0m 21s trunk passed
        +1 mvninstall 0m 29s the patch passed
        +1 compile 0m 29s the patch passed
        +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 0 new + 57 unchanged - 5 fixed = 57 total (was 62)
        +1 mvnsite 0m 30s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 2s the patch passed
        +1 javadoc 0m 17s the patch passed
        +1 unit 43m 11s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        64m 9s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12857013/YARN-5956.14.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 5377f23eb982 3.13.0-108-generic #155-Ubuntu SMP Wed Jan 11 16:58:52 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 9832ae0
        Default Java 1.8.0_121
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15256/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/15256/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 12m 35s trunk passed +1 compile 0m 31s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 33s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 0m 58s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 29s the patch passed +1 compile 0m 29s the patch passed +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 0 new + 57 unchanged - 5 fixed = 57 total (was 62) +1 mvnsite 0m 30s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 2s the patch passed +1 javadoc 0m 17s the patch passed +1 unit 43m 11s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 64m 9s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12857013/YARN-5956.14.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5377f23eb982 3.13.0-108-generic #155-Ubuntu SMP Wed Jan 11 16:58:52 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 9832ae0 Default Java 1.8.0_121 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15256/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/15256/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        lewuathe Kai Sasaki added a comment -

        Rebased on trunk.

        Show
        lewuathe Kai Sasaki added a comment - Rebased on trunk.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 22s 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 13m 4s trunk passed
        +1 compile 0m 39s trunk passed
        +1 checkstyle 0m 30s trunk passed
        +1 mvnsite 0m 44s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 1m 16s trunk passed
        +1 javadoc 0m 26s trunk passed
        +1 mvninstall 0m 40s the patch passed
        +1 compile 0m 38s the patch passed
        +1 javac 0m 38s the patch passed
        +1 checkstyle 0m 28s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 57 unchanged - 5 fixed = 57 total (was 62)
        +1 mvnsite 0m 41s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 27s the patch passed
        +1 javadoc 0m 24s the patch passed
        +1 unit 40m 33s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        64m 19s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859284/YARN-5956.15.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux ed1d275a59f4 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 7536815
        Default Java 1.8.0_121
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15309/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/15309/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 22s 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 13m 4s trunk passed +1 compile 0m 39s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 0m 44s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 16s trunk passed +1 javadoc 0m 26s trunk passed +1 mvninstall 0m 40s the patch passed +1 compile 0m 38s the patch passed +1 javac 0m 38s the patch passed +1 checkstyle 0m 28s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 57 unchanged - 5 fixed = 57 total (was 62) +1 mvnsite 0m 41s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 27s the patch passed +1 javadoc 0m 24s the patch passed +1 unit 40m 33s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 64m 19s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12859284/YARN-5956.15.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ed1d275a59f4 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 7536815 Default Java 1.8.0_121 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15309/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/15309/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sunilg Sunil G added a comment -

        Latest patch seems fine for me. I will wait for a day. Please share if any comments are there.

        Show
        sunilg Sunil G added a comment - Latest patch seems fine for me. I will wait for a day. Please share if any comments are there.
        Hide
        rohithsharma Rohith Sharma K S added a comment -

        +1 LGTM

        Show
        rohithsharma Rohith Sharma K S added a comment - +1 LGTM
        Hide
        ajisakaa Akira Ajisaka added a comment -

        +1 LGTM.
        I found the argument of checkReservationSystem method is not used. Can we use the argument for logging or remove the argument in a separate jira?

        Show
        ajisakaa Akira Ajisaka added a comment - +1 LGTM. I found the argument of checkReservationSystem method is not used. Can we use the argument for logging or remove the argument in a separate jira?
        Hide
        lewuathe Kai Sasaki added a comment -
        Show
        lewuathe Kai Sasaki added a comment - Akira Ajisaka Thanks. I filed a JIRA as followup. https://issues.apache.org/jira/browse/YARN-6379
        Hide
        sunilg Sunil G added a comment -

        Thanks folks. Committing shortly.

        Show
        sunilg Sunil G added a comment - Thanks folks. Committing shortly.
        Hide
        sunilg Sunil G added a comment -

        Kai Sasaki. I have committed patch in trunk. However its not cleanly getting applied in branch-2. Could you please provide a branch-2 patch.

        Show
        sunilg Sunil G added a comment - Kai Sasaki . I have committed patch in trunk. However its not cleanly getting applied in branch-2. Could you please provide a branch-2 patch.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11462 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11462/)
        YARN-5956. Refactor ClientRMService for unify error handling across (sunilg: rev cffea251be4b73ca16e5e11f0be2d22651330f73)

        • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ClientRMService.java
        • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAuditLogger.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11462 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11462/ ) YARN-5956 . Refactor ClientRMService for unify error handling across (sunilg: rev cffea251be4b73ca16e5e11f0be2d22651330f73) (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/ClientRMService.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAuditLogger.java
        Hide
        lewuathe Kai Sasaki added a comment -

        Sunil G Thank you so much.

        However its not cleanly getting applied in branch-2. Could you please provide a branch-2 patch.

        Sure will do.

        Show
        lewuathe Kai Sasaki added a comment - Sunil G Thank you so much. However its not cleanly getting applied in branch-2. Could you please provide a branch-2 patch. Sure will do.
        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 7m 1s branch-2 passed
        +1 compile 0m 32s branch-2 passed with JDK v1.8.0_121
        +1 compile 0m 33s branch-2 passed with JDK v1.7.0_121
        +1 checkstyle 0m 24s branch-2 passed
        +1 mvnsite 0m 38s branch-2 passed
        +1 mvneclipse 0m 17s branch-2 passed
        +1 findbugs 1m 14s branch-2 passed
        +1 javadoc 0m 21s branch-2 passed with JDK v1.8.0_121
        +1 javadoc 0m 25s branch-2 passed with JDK v1.7.0_121
        +1 mvninstall 0m 31s the patch passed
        +1 compile 0m 28s the patch passed with JDK v1.8.0_121
        +1 javac 0m 28s the patch passed
        +1 compile 0m 31s the patch passed with JDK v1.7.0_121
        +1 javac 0m 31s the patch passed
        -0 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 13 new + 55 unchanged - 2 fixed = 68 total (was 57)
        +1 mvnsite 0m 37s the patch passed
        +1 mvneclipse 0m 15s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 23s the patch passed
        +1 javadoc 0m 19s the patch passed with JDK v1.8.0_121
        +1 javadoc 0m 23s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_121 with JDK v1.7.0_121 generated 0 new + 1 unchanged - 1 fixed = 1 total (was 2)
        -1 unit 42m 2s hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.7.0_121.
        -1 asflicense 0m 22s The patch generated 3 ASF License warnings.
        101m 4s



        Reason Tests
        JDK v1.8.0_121 Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart
          hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer
        JDK v1.7.0_121 Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart
          hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacityScheduler



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:b59b8b7
        JIRA Issue YARN-5956
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860555/YARN-5956-branch-2.01.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux e00a1c25aa1d 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 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 / 43f23e5
        Default Java 1.7.0_121
        Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15388/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/15388/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_121.txt
        JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15388/testReport/
        asflicense https://builds.apache.org/job/PreCommit-YARN-Build/15388/artifact/patchprocess/patch-asflicense-problems.txt
        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/15388/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 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 7m 1s branch-2 passed +1 compile 0m 32s branch-2 passed with JDK v1.8.0_121 +1 compile 0m 33s branch-2 passed with JDK v1.7.0_121 +1 checkstyle 0m 24s branch-2 passed +1 mvnsite 0m 38s branch-2 passed +1 mvneclipse 0m 17s branch-2 passed +1 findbugs 1m 14s branch-2 passed +1 javadoc 0m 21s branch-2 passed with JDK v1.8.0_121 +1 javadoc 0m 25s branch-2 passed with JDK v1.7.0_121 +1 mvninstall 0m 31s the patch passed +1 compile 0m 28s the patch passed with JDK v1.8.0_121 +1 javac 0m 28s the patch passed +1 compile 0m 31s the patch passed with JDK v1.7.0_121 +1 javac 0m 31s the patch passed -0 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 13 new + 55 unchanged - 2 fixed = 68 total (was 57) +1 mvnsite 0m 37s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 23s the patch passed +1 javadoc 0m 19s the patch passed with JDK v1.8.0_121 +1 javadoc 0m 23s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_121 with JDK v1.7.0_121 generated 0 new + 1 unchanged - 1 fixed = 1 total (was 2) -1 unit 42m 2s hadoop-yarn-server-resourcemanager in the patch failed with JDK v1.7.0_121. -1 asflicense 0m 22s The patch generated 3 ASF License warnings. 101m 4s Reason Tests JDK v1.8.0_121 Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer JDK v1.7.0_121 Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacityScheduler Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue YARN-5956 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12860555/YARN-5956-branch-2.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux e00a1c25aa1d 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 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 / 43f23e5 Default Java 1.7.0_121 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_121 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/15388/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/15388/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-jdk1.7.0_121.txt JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/15388/testReport/ asflicense https://builds.apache.org/job/PreCommit-YARN-Build/15388/artifact/patchprocess/patch-asflicense-problems.txt 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/15388/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        sunilg Sunil G added a comment -

        test case failures are unrelated. And asf license too showing in some MR files which are unrelated.
        Committing to branch-2.

        Show
        sunilg Sunil G added a comment - test case failures are unrelated. And asf license too showing in some MR files which are unrelated. Committing to branch-2.
        Hide
        sunilg Sunil G added a comment -

        Committed to trunk/branch-2. Thanks Kai Sasaki for the contribution. Also thanks to Rohith Sharma K S Daniel Templeton and Akira Ajisaka for additional review.

        Show
        sunilg Sunil G added a comment - Committed to trunk/branch-2. Thanks Kai Sasaki for the contribution. Also thanks to Rohith Sharma K S Daniel Templeton and Akira Ajisaka for additional review.

          People

          • Assignee:
            lewuathe Kai Sasaki
            Reporter:
            lewuathe Kai Sasaki
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development