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

MoveApplicationAcrossQueues does not check user permission on the target queue

    Details

    • Hadoop Flags:
      Reviewed

      Description

      moveApplicationAcrossQueues operation currently does not check user permission on the target queue. This incorrectly allows one user to move his/her own applications to a queue that the user has no access to

      1. YARN-5554.10.patch
        22 kB
        Wilfred Spiegelenburg
      2. YARN-5554.11.patch
        22 kB
        Wilfred Spiegelenburg
      3. YARN-5554.12.patch
        21 kB
        Wilfred Spiegelenburg
      4. YARN-5554.13.patch
        21 kB
        Wilfred Spiegelenburg
      5. YARN-5554.14.patch
        21 kB
        Wilfred Spiegelenburg
      6. YARN-5554.2.patch
        14 kB
        Wilfred Spiegelenburg
      7. YARN-5554.3.patch
        14 kB
        Wilfred Spiegelenburg
      8. YARN-5554.4.patch
        20 kB
        Wilfred Spiegelenburg
      9. YARN-5554.5.patch
        19 kB
        Wilfred Spiegelenburg
      10. YARN-5554.6.patch
        20 kB
        Wilfred Spiegelenburg
      11. YARN-5554.7.patch
        20 kB
        Wilfred Spiegelenburg
      12. YARN-5554.8.patch
        20 kB
        Wilfred Spiegelenburg
      13. YARN-5554.9.patch
        20 kB
        Wilfred Spiegelenburg

        Activity

        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 appears to include 1 new or modified test files.
        +1 mvninstall 6m 49s trunk passed
        +1 compile 0m 32s trunk passed
        +1 checkstyle 0m 20s trunk passed
        +1 mvnsite 0m 38s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 0m 55s trunk passed
        +1 javadoc 0m 25s trunk passed
        +1 mvninstall 0m 36s the patch passed
        +1 compile 0m 31s the patch passed
        +1 javac 0m 31s the patch passed
        -1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 78 unchanged - 0 fixed = 84 total (was 78)
        +1 mvnsite 0m 34s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 2s the patch passed
        +1 javadoc 0m 18s the patch passed
        -1 unit 37m 55s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 15s The patch does not generate ASF License warnings.
        52m 35s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825183/yarn5554.001.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 4c2449cf7c8a 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / c37346d
        Default Java 1.8.0_101
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12871/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/12871/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/12871/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/12871/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/12871/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 49s trunk passed +1 compile 0m 32s trunk passed +1 checkstyle 0m 20s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 55s trunk passed +1 javadoc 0m 25s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 0m 31s the patch passed +1 javac 0m 31s the patch passed -1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 6 new + 78 unchanged - 0 fixed = 84 total (was 78) +1 mvnsite 0m 34s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 2s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 37m 55s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 52m 35s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825183/yarn5554.001.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4c2449cf7c8a 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c37346d Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12871/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/12871/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/12871/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/12871/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/12871/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        rohithsharma Rohith Sharma K S added a comment -

        +1 for the issue. And also I think there are uncovered bugs with respect to queueACL for moveToQueue operation.

        1. Along with modify app acl, user should also have access to ADMINISTER_QUEUE & SUBMIT_APPLICATIONS acl's. Basically all 3 operations should be AND operation rather than OR operation. cc :-/ Jian He

        Comments on the patch,

        1. patch appears to be do not fix the reported bug since it still check for old-queue for user permission. Basically user authorization should happen for targeted queue.
        2. The method appSubmissionToQueueAllowed can be renamed to checkUserAccessToQueue?
        Show
        rohithsharma Rohith Sharma K S added a comment - +1 for the issue. And also I think there are uncovered bugs with respect to queueACL for moveToQueue operation. Along with modify app acl, user should also have access to ADMINISTER_QUEUE & SUBMIT_APPLICATIONS acl's. Basically all 3 operations should be AND operation rather than OR operation. cc :-/ Jian He Comments on the patch, patch appears to be do not fix the reported bug since it still check for old-queue for user permission. Basically user authorization should happen for targeted queue. The method appSubmissionToQueueAllowed can be renamed to checkUserAccessToQueue ?
        Hide
        haibochen Haibo Chen added a comment -

        Thanks Rohith Sharma K S for the review! Yes, the patch does not fix the bug. Wilfred Spiegelenburg will post a correct patch later for review. Let me delete the attachment. Thanks again.

        Show
        haibochen Haibo Chen added a comment - Thanks Rohith Sharma K S for the review! Yes, the patch does not fix the bug. Wilfred Spiegelenburg will post a correct patch later for review. Let me delete the attachment. Thanks again.
        Hide
        rohithsharma Rohith Sharma K S added a comment -

        Please do not delete once attached patches. It is required for history tracking even though it is wrong patch.

        Show
        rohithsharma Rohith Sharma K S added a comment - Please do not delete once attached patches. It is required for history tracking even though it is wrong patch.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Patch that does the checks for the ACL on the target queue.
        Following checks are made:

        • user has modify application permission or is admin on the queue (existing check)
        • user has submit access or admin access on the target queue (new check)

        Tests have been added for the newly added check. The existing check has not got a test and is not covered by the newly added tests

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Patch that does the checks for the ACL on the target queue. Following checks are made: user has modify application permission or is admin on the queue (existing check) user has submit access or admin access on the target queue (new check) Tests have been added for the newly added check. The existing check has not got a test and is not covered by the newly added tests
        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 appears to include 1 new or modified test files.
        +1 mvninstall 7m 18s trunk passed
        +1 compile 0m 32s trunk passed
        +1 checkstyle 0m 21s trunk passed
        +1 mvnsite 0m 40s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 0m 57s trunk passed
        +1 javadoc 0m 22s trunk passed
        +1 mvninstall 0m 31s the patch passed
        +1 compile 0m 30s the patch passed
        -1 javac 0m 30s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 2 unchanged - 1 fixed = 3 total (was 3)
        -1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 3 new + 79 unchanged - 1 fixed = 82 total (was 80)
        +1 mvnsite 0m 36s the patch passed
        +1 mvneclipse 0m 13s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 2s the patch passed
        +1 javadoc 0m 18s the patch passed
        +1 unit 38m 28s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        53m 41s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825422/YARN-5554.2.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux f9047ef1ec8f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / ab3b727
        Default Java 1.8.0_101
        findbugs v3.0.0
        javac https://builds.apache.org/job/PreCommit-YARN-Build/12890/artifact/patchprocess/diff-compile-javac-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12890/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12890/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/12890/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 18s trunk passed +1 compile 0m 32s trunk passed +1 checkstyle 0m 21s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 57s trunk passed +1 javadoc 0m 22s trunk passed +1 mvninstall 0m 31s the patch passed +1 compile 0m 30s the patch passed -1 javac 0m 30s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 2 unchanged - 1 fixed = 3 total (was 3) -1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 3 new + 79 unchanged - 1 fixed = 82 total (was 80) +1 mvnsite 0m 36s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 2s the patch passed +1 javadoc 0m 18s the patch passed +1 unit 38m 28s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 53m 41s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825422/YARN-5554.2.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f9047ef1ec8f 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ab3b727 Default Java 1.8.0_101 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-YARN-Build/12890/artifact/patchprocess/diff-compile-javac-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/12890/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12890/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/12890/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        New patch with fixes for the checkstyle and javac test failures

        Show
        wilfreds Wilfred Spiegelenburg added a comment - New patch with fixes for the checkstyle and javac test failures
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 20s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 46s trunk passed
        +1 compile 0m 32s trunk passed
        +1 checkstyle 0m 21s trunk passed
        +1 mvnsite 0m 38s trunk passed
        +1 mvneclipse 0m 16s trunk passed
        +1 findbugs 0m 58s trunk passed
        +1 javadoc 0m 21s trunk passed
        +1 mvninstall 0m 31s the patch passed
        +1 compile 0m 29s the patch passed
        +1 javac 0m 29s the patch passed
        +1 checkstyle 0m 18s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 79 unchanged - 1 fixed = 79 total (was 80)
        +1 mvnsite 0m 35s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 2s the patch passed
        +1 javadoc 0m 18s the patch passed
        +1 unit 38m 56s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 15s The patch does not generate ASF License warnings.
        53m 33s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825458/YARN-5554.3.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 089567306fbf 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 525d52b
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12892/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/12892/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 46s trunk passed +1 compile 0m 32s trunk passed +1 checkstyle 0m 21s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 0m 58s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 31s the patch passed +1 compile 0m 29s the patch passed +1 javac 0m 29s the patch passed +1 checkstyle 0m 18s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 79 unchanged - 1 fixed = 79 total (was 80) +1 mvnsite 0m 35s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 2s the patch passed +1 javadoc 0m 18s the patch passed +1 unit 38m 56s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 53m 33s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12825458/YARN-5554.3.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 089567306fbf 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 525d52b Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/12892/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/12892/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        haibochen Haibo Chen added a comment -

        Sorry for messing up the history. I will keep it in mind.

        Show
        haibochen Haibo Chen added a comment - Sorry for messing up the history. I will keep it in mind.
        Hide
        yufeigu Yufei Gu added a comment - - edited

        Thanks Wilfred Spiegelenburg for working on this. The patch looks good to me generally. My one thought is that moveApplicationAcrossQueues doesn't check if the queue exists before ACL checking, neither do its callers. It's OK since its callee like queueACLsManager.checkAccess did the NULL check of the target queue, but the LOG information seems vague. What about to do the NULL check in moveApplicationAcrossQueues and provide the explicitly information of "target queue does't exist" and remove the NULL check in its callee like function queueACLsManager.checkAccess added in this patch.

        Show
        yufeigu Yufei Gu added a comment - - edited Thanks Wilfred Spiegelenburg for working on this. The patch looks good to me generally. My one thought is that moveApplicationAcrossQueues doesn't check if the queue exists before ACL checking, neither do its callers. It's OK since its callee like queueACLsManager.checkAccess did the NULL check of the target queue, but the LOG information seems vague. What about to do the NULL check in moveApplicationAcrossQueues and provide the explicitly information of "target queue does't exist" and remove the NULL check in its callee like function queueACLsManager.checkAccess added in this patch.
        Hide
        jianhe Jian He added a comment -

        For the permission part: should we check (submit_acl_on_target_queue || target_queue_adminAcl) && application_acl
        The first half means permission on the target queue; The second half means permission on application itself.
        I think the first half is also what being used currently in SubmitApplication

        Show
        jianhe Jian He added a comment - For the permission part: should we check (submit_acl_on_target_queue || target_queue_adminAcl) && application_acl The first half means permission on the target queue; The second half means permission on application itself. I think the first half is also what being used currently in SubmitApplication
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Sorry for the delayed response, I tried to add a new test which test just the getAccess call in the client but did not get it to work nicely.

        I have updated the patch with the check for an non existing queue including an extra test.

        I did not move the check for a non existent queue into the ClientRMService because each scheduler checks the queue existence in its own way and we would have had to introduce a number of new dependencies into the client. I left it in QueueACLsManager which already has the CS as a dependency. It now also logs that the target queue does not exists.

        For the check that Jian He mentioned: we have an existing check for MODIFY_APP in the code. That check also takes into account the administrator access for the origin queue, covering the application_acl part. The new check added handles the first part submit_acl_on_target_queue || target_queue_adminAcl) Both need to pass to move the application.

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Sorry for the delayed response, I tried to add a new test which test just the getAccess call in the client but did not get it to work nicely. I have updated the patch with the check for an non existing queue including an extra test. I did not move the check for a non existent queue into the ClientRMService because each scheduler checks the queue existence in its own way and we would have had to introduce a number of new dependencies into the client. I left it in QueueACLsManager which already has the CS as a dependency. It now also logs that the target queue does not exists. For the check that Jian He mentioned: we have an existing check for MODIFY_APP in the code. That check also takes into account the administrator access for the origin queue, covering the application_acl part. The new check added handles the first part submit_acl_on_target_queue || target_queue_adminAcl) Both need to pass to move the application.
        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 appears to include 1 new or modified test files.
        +1 mvninstall 7m 9s trunk passed
        +1 compile 0m 34s trunk passed
        +1 checkstyle 0m 23s trunk passed
        +1 mvnsite 0m 40s trunk passed
        +1 mvneclipse 0m 16s trunk passed
        +1 findbugs 1m 2s 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 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 34 new + 79 unchanged - 1 fixed = 113 total (was 80)
        +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 8s the patch passed
        +1 javadoc 0m 19s the patch passed
        +1 unit 33m 50s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 15s The patch does not generate ASF License warnings.
        49m 7s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830283/YARN-5554.4.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux a7a867919a26 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 14a696f
        Default Java 1.8.0_101
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13213/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13213/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/13213/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 9s trunk passed +1 compile 0m 34s trunk passed +1 checkstyle 0m 23s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 1m 2s 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 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 34 new + 79 unchanged - 1 fixed = 113 total (was 80) +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 8s the patch passed +1 javadoc 0m 19s the patch passed +1 unit 33m 50s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 49m 7s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830283/YARN-5554.4.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a7a867919a26 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 14a696f Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/13213/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13213/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/13213/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        updated patch to fix the checkstyle warnings

        Show
        wilfreds Wilfred Spiegelenburg added a comment - updated patch to fix the checkstyle warnings
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 21s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 37s trunk passed
        +1 compile 0m 31s trunk passed
        +1 checkstyle 0m 21s trunk passed
        +1 mvnsite 0m 38s trunk passed
        +1 mvneclipse 0m 16s trunk passed
        +1 findbugs 0m 56s 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
        +1 checkstyle 0m 18s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 79 unchanged - 1 fixed = 79 total (was 80)
        +1 mvnsite 0m 34s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 1s the patch passed
        +1 javadoc 0m 18s the patch passed
        -1 unit 33m 0s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 14s The patch does not generate ASF License warnings.
        47m 14s



        Reason Tests
        Failed junit tests hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830297/YARN-5554.5.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux afe888510cd6 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 / 14a696f
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/13215/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13215/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/13215/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/13215/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 37s trunk passed +1 compile 0m 31s trunk passed +1 checkstyle 0m 21s trunk passed +1 mvnsite 0m 38s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 0m 56s 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 +1 checkstyle 0m 18s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 79 unchanged - 1 fixed = 79 total (was 80) +1 mvnsite 0m 34s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 1s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 33m 0s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 14s The patch does not generate ASF License warnings. 47m 14s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830297/YARN-5554.5.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux afe888510cd6 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 / 14a696f Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/13215/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit test logs https://builds.apache.org/job/PreCommit-YARN-Build/13215/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/13215/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/13215/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment - - edited

        The test failure is logged as YARN-5043 and is not related to the changes made

        Show
        wilfreds Wilfred Spiegelenburg added a comment - - edited The test failure is logged as YARN-5043 and is not related to the changes made
        Hide
        yufeigu Yufei Gu added a comment -

        Thanks Wilfred Spiegelenburg's new patch. It makes sense to not introduce more dependencies. Some nits:
        1.

          
          /*
           * Check access to a targetQueue in the case of a move of an application
        

        The first line should be /**.
        2. I am wondering

            if (scheduler instanceof CapacityScheduler) {
            ...
            } else if (scheduler instanceof FairScheduler) {
            ...
            } else {
            return ...
            }
        

        will be more readable.
        3. In func moveApplicationAcrossQueues, can we use ioe.toString() instead of "Target queue does not exist " + targetQueue?

        Show
        yufeigu Yufei Gu added a comment - Thanks Wilfred Spiegelenburg 's new patch. It makes sense to not introduce more dependencies. Some nits: 1. /* * Check access to a targetQueue in the case of a move of an application The first line should be /** . 2. I am wondering if (scheduler instanceof CapacityScheduler) { ... } else if (scheduler instanceof FairScheduler) { ... } else { return ... } will be more readable. 3. In func moveApplicationAcrossQueues , can we use ioe.toString() instead of "Target queue does not exist " + targetQueue ?
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Yufei Gu updated the patch with the feedback, all 3 points have been integrated in the new patch

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Yufei Gu updated the patch with the feedback, all 3 points have been integrated in the new patch
        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 appears to include 1 new or modified test files.
        +1 mvninstall 7m 56s trunk passed
        +1 compile 0m 33s trunk passed
        +1 checkstyle 0m 24s trunk passed
        +1 mvnsite 0m 41s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 1m 5s trunk passed
        +1 javadoc 0m 23s trunk passed
        +1 mvninstall 0m 37s the patch passed
        +1 compile 0m 35s the patch passed
        +1 javac 0m 35s the patch passed
        +1 checkstyle 0m 23s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 78 unchanged - 2 fixed = 78 total (was 80)
        +1 mvnsite 0m 44s the patch passed
        +1 mvneclipse 0m 16s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 12s the patch passed
        -1 javadoc 0m 21s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 941 unchanged - 0 fixed = 942 total (was 941)
        +1 unit 34m 17s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        50m 58s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830491/YARN-5554.6.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 9bff3d3f1ca4 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / ebf528c
        Default Java 1.8.0_101
        findbugs v3.0.0
        javadoc https://builds.apache.org/job/PreCommit-YARN-Build/13223/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13223/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/13223/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 56s trunk passed +1 compile 0m 33s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 41s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 5s trunk passed +1 javadoc 0m 23s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 0m 35s the patch passed +1 javac 0m 35s the patch passed +1 checkstyle 0m 23s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 78 unchanged - 2 fixed = 78 total (was 80) +1 mvnsite 0m 44s the patch passed +1 mvneclipse 0m 16s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 12s the patch passed -1 javadoc 0m 21s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 941 unchanged - 0 fixed = 942 total (was 941) +1 unit 34m 17s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 50m 58s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830491/YARN-5554.6.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 9bff3d3f1ca4 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ebf528c Default Java 1.8.0_101 findbugs v3.0.0 javadoc https://builds.apache.org/job/PreCommit-YARN-Build/13223/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13223/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/13223/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Forgot to check the newly created java doc based on the feedback, patch updated to fix it

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Forgot to check the newly created java doc based on the feedback, patch updated to fix it
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 19s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 7m 7s trunk passed
        +1 compile 0m 33s trunk passed
        +1 checkstyle 0m 22s trunk passed
        +1 mvnsite 0m 39s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 0m 58s trunk passed
        +1 javadoc 0m 22s trunk passed
        +1 mvninstall 0m 32s the patch passed
        +1 compile 0m 30s the patch passed
        +1 javac 0m 30s the patch passed
        +1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 78 unchanged - 2 fixed = 78 total (was 80)
        +1 mvnsite 0m 38s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 4s the patch passed
        +1 javadoc 0m 19s the patch passed
        +1 unit 34m 3s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        49m 6s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830501/YARN-5554.7.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 7492166053dd 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / ebf528c
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13226/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/13226/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 7s trunk passed +1 compile 0m 33s trunk passed +1 checkstyle 0m 22s trunk passed +1 mvnsite 0m 39s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 58s trunk passed +1 javadoc 0m 22s trunk passed +1 mvninstall 0m 32s the patch passed +1 compile 0m 30s the patch passed +1 javac 0m 30s the patch passed +1 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 78 unchanged - 2 fixed = 78 total (was 80) +1 mvnsite 0m 38s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 4s the patch passed +1 javadoc 0m 19s the patch passed +1 unit 34m 3s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 49m 6s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830501/YARN-5554.7.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7492166053dd 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ebf528c Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13226/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/13226/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        yufeigu Yufei Gu added a comment -

        The latest patch looks good to me. +1(non-binding).

        Show
        yufeigu Yufei Gu added a comment - The latest patch looks good to me. +1(non-binding).
        Hide
        kasha Karthik Kambatla added a comment -

        Thanks for reporting and working on this, Wilfred Spiegelenburg.

        Main comment on the patch: QueueACLsManager#checkAccess: Instead of throwing an IOException, can we log the fact that queue does not exist and return false?

        Show
        kasha Karthik Kambatla added a comment - Thanks for reporting and working on this, Wilfred Spiegelenburg . Main comment on the patch: QueueACLsManager#checkAccess: Instead of throwing an IOException, can we log the fact that queue does not exist and return false?
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Thanks Karthik Kambatla I changed the return value to just a false instead of throwing and updated all the code that relies on it.

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Thanks Karthik Kambatla I changed the return value to just a false instead of throwing and updated all the code that relies on it.
        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 appears to include 1 new or modified test files.
        +1 mvninstall 7m 45s trunk passed
        +1 compile 0m 40s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 0m 46s trunk passed
        +1 mvneclipse 0m 20s trunk passed
        +1 findbugs 1m 10s trunk passed
        +1 javadoc 0m 27s 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 23s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 77 unchanged - 3 fixed = 77 total (was 80)
        +1 mvnsite 0m 43s the patch passed
        +1 mvneclipse 0m 16s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 14s the patch passed
        +1 javadoc 0m 21s the patch passed
        +1 unit 44m 17s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        61m 23s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831676/YARN-5554.8.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 323ca0d0bd1d 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 / 31f8da2
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13286/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/13286/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 45s trunk passed +1 compile 0m 40s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 46s trunk passed +1 mvneclipse 0m 20s trunk passed +1 findbugs 1m 10s trunk passed +1 javadoc 0m 27s 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 23s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 77 unchanged - 3 fixed = 77 total (was 80) +1 mvnsite 0m 43s the patch passed +1 mvneclipse 0m 16s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 14s the patch passed +1 javadoc 0m 21s the patch passed +1 unit 44m 17s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 61m 23s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831676/YARN-5554.8.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 323ca0d0bd1d 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 / 31f8da2 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13286/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/13286/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        bibinchundatt Bibin A Chundatt added a comment -

        Thank you Wilfred Spiegelenburg for patch.
        In Audit logger and Remote Exception can we add queue doesn't exists message too.

        +      RMAuditLogger.logFailure(callerUGI.getShortUserName(),
        +          AuditConstants.MOVE_APP_REQUEST,
        +          "User doesn't have permissions to move application to queue "
        +              + targetQueue, "ClientRMService",
        +          AuditConstants.UNAUTHORIZED_USER, applicationId);
        +      throw RPCUtil.getRemoteException(new AccessControlException("User "
        +          + callerUGI.getShortUserName()
        +          + " doesn't have permissions to move application to queue "
        +          + targetQueue + " on " + applicationId));
        
        Show
        bibinchundatt Bibin A Chundatt added a comment - Thank you Wilfred Spiegelenburg for patch. In Audit logger and Remote Exception can we add queue doesn't exists message too. + RMAuditLogger.logFailure(callerUGI.getShortUserName(), + AuditConstants.MOVE_APP_REQUEST, + "User doesn't have permissions to move application to queue " + + targetQueue, "ClientRMService", + AuditConstants.UNAUTHORIZED_USER, applicationId); + throw RPCUtil.getRemoteException(new AccessControlException("User " + + callerUGI.getShortUserName() + + " doesn't have permissions to move application to queue " + + targetQueue + " on " + applicationId));
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        updated the text in the messages, it does make sense to include it not just in the message of the queue manager. Does the message look OK Bibin A Chundatt ?

        Show
        wilfreds Wilfred Spiegelenburg added a comment - updated the text in the messages, it does make sense to include it not just in the message of the queue manager. Does the message look OK Bibin A Chundatt ?
        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 appears to include 1 new or modified test files.
        +1 mvninstall 8m 15s trunk passed
        +1 compile 0m 38s trunk passed
        +1 checkstyle 0m 22s trunk passed
        +1 mvnsite 0m 48s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 1m 6s trunk passed
        +1 javadoc 0m 25s trunk passed
        +1 mvninstall 0m 37s the patch passed
        +1 compile 0m 34s the patch passed
        +1 javac 0m 34s the patch passed
        +1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 77 unchanged - 3 fixed = 77 total (was 80)
        +1 mvnsite 0m 39s the patch passed
        +1 mvneclipse 0m 16s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 12s the patch passed
        +1 javadoc 0m 21s the patch passed
        +1 unit 47m 27s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        64m 35s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831683/YARN-5554.9.patch
        JIRA Issue YARN-5554
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux b2ca74d02a86 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 / 31f8da2
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13287/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/13287/console
        Powered by Apache Yetus 0.3.0 http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 8m 15s trunk passed +1 compile 0m 38s trunk passed +1 checkstyle 0m 22s trunk passed +1 mvnsite 0m 48s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 6s trunk passed +1 javadoc 0m 25s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 0m 34s the patch passed +1 javac 0m 34s the patch passed +1 checkstyle 0m 21s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 77 unchanged - 3 fixed = 77 total (was 80) +1 mvnsite 0m 39s the patch passed +1 mvneclipse 0m 16s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 12s the patch passed +1 javadoc 0m 21s the patch passed +1 unit 47m 27s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 64m 35s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831683/YARN-5554.9.patch JIRA Issue YARN-5554 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b2ca74d02a86 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 / 31f8da2 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13287/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/13287/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
        Hide
        templedf Daniel Templeton added a comment -

        Thanks for all the patch updates, Wilfred Spiegelenburg. My turn.

        • " doesn't have permissions submit to target queue: " is missing a "to" before the "submit."
        • In QueueACLsManager.checkAccess(), I don't see why you need to do the scheduler-dependent if. Can't you just call checkAccess() in all cases?
        • In your tests, I would feel better if you tested that the app is in the right queue after the successful moves.
        • Note that your use of a lambda in createClientRMServiceForMoveApplicationRequest() means this patch can only go into trunk.
        Show
        templedf Daniel Templeton added a comment - Thanks for all the patch updates, Wilfred Spiegelenburg . My turn. " doesn't have permissions submit to target queue: " is missing a "to" before the "submit." In QueueACLsManager.checkAccess() , I don't see why you need to do the scheduler-dependent if . Can't you just call checkAccess() in all cases? In your tests, I would feel better if you tested that the app is in the right queue after the successful moves. Note that your use of a lambda in createClientRMServiceForMoveApplicationRequest() means this patch can only go into trunk.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        " doesn't have permissions submit to target queue: " is missing a "to" before the "submit."

        fixed the typo

        In QueueACLsManager.checkAccess(), I don't see why you need to do the scheduler-dependent if. Can't you just call checkAccess() in all cases?

        The capacity scheduler part is a copy of the checkAccess() that is already there. The change to not use the checkAccess() of the scheduler for the capacity scheduler was made as part of YARN-4571. Bringing the FairScheduler and the CapacityScheduler in sync is more work than we can just push into this jira. I think it is better to open a follow up jira to refactor this and bring the two schedulers in sync again. Let me know if you agree with that approach.

        In your tests, I would feel better if you tested that the app is in the right queue after the successful moves.

        Because of the way the tests are mocked up the current tests can not do that. We create a ClientRMService which does not have a scheduler or an application manager. The test are focussed on the ACL managers and making sure that they stop the move in the service. We can extend the tests to do the app checks but that would introduce scheduler specific testing into the client service.

        Note that your use of a lambda in createClientRMServiceForMoveApplicationRequest() means this patch can only go into trunk.

        oops did not think about that. I'll have rewritten the tests to remove the lambda. I now really appreciate the simplicity of using a lambda

        Show
        wilfreds Wilfred Spiegelenburg added a comment - " doesn't have permissions submit to target queue: " is missing a "to" before the "submit." fixed the typo In QueueACLsManager.checkAccess(), I don't see why you need to do the scheduler-dependent if. Can't you just call checkAccess() in all cases? The capacity scheduler part is a copy of the checkAccess() that is already there. The change to not use the checkAccess() of the scheduler for the capacity scheduler was made as part of YARN-4571 . Bringing the FairScheduler and the CapacityScheduler in sync is more work than we can just push into this jira. I think it is better to open a follow up jira to refactor this and bring the two schedulers in sync again. Let me know if you agree with that approach. In your tests, I would feel better if you tested that the app is in the right queue after the successful moves. Because of the way the tests are mocked up the current tests can not do that. We create a ClientRMService which does not have a scheduler or an application manager. The test are focussed on the ACL managers and making sure that they stop the move in the service. We can extend the tests to do the app checks but that would introduce scheduler specific testing into the client service. Note that your use of a lambda in createClientRMServiceForMoveApplicationRequest() means this patch can only go into trunk. oops did not think about that. I'll have rewritten the tests to remove the lambda. I now really appreciate the simplicity of using a lambda
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        New patch with the changes from the review

        Show
        wilfreds Wilfred Spiegelenburg added a comment - New patch with the changes from the review
        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 appears to include 1 new or modified test files.
        +1 mvninstall 7m 5s trunk passed
        +1 compile 0m 33s trunk passed
        +1 checkstyle 0m 22s trunk passed
        +1 mvnsite 0m 39s trunk passed
        +1 mvneclipse 0m 18s trunk passed
        +1 findbugs 0m 59s trunk passed
        +1 javadoc 0m 21s trunk passed
        +1 mvninstall 0m 34s the patch passed
        +1 compile 0m 32s the patch passed
        +1 javac 0m 32s the patch passed
        -0 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 2 new + 77 unchanged - 3 fixed = 79 total (was 80)
        +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 6s the patch passed
        +1 javadoc 0m 20s the patch passed
        +1 unit 42m 37s hadoop-yarn-server-resourcemanager in the patch passed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        58m 23s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5554
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841522/YARN-5554.10.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 4cf6c0be66fb 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 / c87b3a4
        Default Java 1.8.0_111
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14163/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14163/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/14163/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 appears to include 1 new or modified test files. +1 mvninstall 7m 5s trunk passed +1 compile 0m 33s trunk passed +1 checkstyle 0m 22s trunk passed +1 mvnsite 0m 39s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 0m 59s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 34s the patch passed +1 compile 0m 32s the patch passed +1 javac 0m 32s the patch passed -0 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 2 new + 77 unchanged - 3 fixed = 79 total (was 80) +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 6s the patch passed +1 javadoc 0m 20s the patch passed +1 unit 42m 37s hadoop-yarn-server-resourcemanager in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 58m 23s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5554 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841522/YARN-5554.10.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4cf6c0be66fb 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 / c87b3a4 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14163/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/14163/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/14163/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        removing unused imports to fix checkstyle warnings

        Show
        wilfreds Wilfred Spiegelenburg added a comment - removing unused imports to fix checkstyle warnings
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 19s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 50s trunk passed
        +1 compile 0m 32s trunk passed
        +1 checkstyle 0m 22s trunk passed
        +1 mvnsite 0m 39s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 0m 59s trunk passed
        +1 javadoc 0m 21s trunk passed
        +1 mvninstall 0m 32s the patch passed
        +1 compile 0m 33s the patch passed
        +1 javac 0m 33s the patch passed
        +1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 77 unchanged - 3 fixed = 77 total (was 80)
        +1 mvnsite 0m 39s the patch passed
        +1 mvneclipse 0m 16s the patch passed
        +1 whitespace 0m 1s The patch has no whitespace issues.
        +1 findbugs 1m 7s the patch passed
        +1 javadoc 0m 19s the patch passed
        -1 unit 43m 11s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 16s The patch does not generate ASF License warnings.
        58m 50s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5554
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841865/YARN-5554.11.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 0b2be41230d2 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 / dcedb72
        Default Java 1.8.0_111
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-YARN-Build/14192/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/14192/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/14192/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 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 50s trunk passed +1 compile 0m 32s trunk passed +1 checkstyle 0m 22s trunk passed +1 mvnsite 0m 39s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 0m 59s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 32s the patch passed +1 compile 0m 33s the patch passed +1 javac 0m 33s the patch passed +1 checkstyle 0m 20s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 77 unchanged - 3 fixed = 77 total (was 80) +1 mvnsite 0m 39s the patch passed +1 mvneclipse 0m 16s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. +1 findbugs 1m 7s the patch passed +1 javadoc 0m 19s the patch passed -1 unit 43m 11s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 58m 50s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5554 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841865/YARN-5554.11.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0b2be41230d2 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 / dcedb72 Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-YARN-Build/14192/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/14192/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/14192/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 -

        The change to not use the checkAccess() of the scheduler for the capacity scheduler was made as part of YARN-4571.

        I'm looking at YARN-4571, and I can't see any reason why checkAccess() was modified to special case the capacity scheduler. The only difference I see from the code is that the new checkAccess() plows ahead even if the queue is null. Maybe there were differences at the time that have since disappeared? (Jian He, can you shed any light?)

        Looking at your new checkAccess() method, I also don't see where it does anything different from the schedulers' checkAccess() methods. What problem is it that you're solving with the new method?

        Show
        templedf Daniel Templeton added a comment - The change to not use the checkAccess() of the scheduler for the capacity scheduler was made as part of YARN-4571 . I'm looking at YARN-4571 , and I can't see any reason why checkAccess() was modified to special case the capacity scheduler. The only difference I see from the code is that the new checkAccess() plows ahead even if the queue is null. Maybe there were differences at the time that have since disappeared? ( Jian He , can you shed any light?) Looking at your new checkAccess() method, I also don't see where it does anything different from the schedulers' checkAccess() methods. What problem is it that you're solving with the new method?
        Hide
        jianhe Jian He added a comment -

        In YARN-4571, do you mean the QueueACLsManager#checkAccess method ? it has difference between CS and FS: in CS it calls YarnAuthorizationProvider#checkPermission, whereas FS doesn't.

        Show
        jianhe Jian He added a comment - In YARN-4571 , do you mean the QueueACLsManager#checkAccess method ? it has difference between CS and FS: in CS it calls YarnAuthorizationProvider#checkPermission, whereas FS doesn't.
        Hide
        templedf Daniel Templeton added a comment -

        My question is why the QueueACLsManager.checkAccess() method replicates what's in CapacityScheduler.checkAccess() instead of just calling that method (which would mean that there's no need to special case for the scheduler type).

        Show
        templedf Daniel Templeton added a comment - My question is why the QueueACLsManager.checkAccess() method replicates what's in CapacityScheduler.checkAccess() instead of just calling that method (which would mean that there's no need to special case for the scheduler type).
        Hide
        jianhe Jian He added a comment -

        oh, I think it was because the CapacityScheduler#checkAccess is actually not needed for CS now, it cannot be removed because it's a public interface. And CS requires additional parameters like remoteAddress, forwardedAddresses which is not that appropriate to add that in the public interface, because FS inherits the same interface and it does not require these parameters.

        Show
        jianhe Jian He added a comment - oh, I think it was because the CapacityScheduler#checkAccess is actually not needed for CS now, it cannot be removed because it's a public interface. And CS requires additional parameters like remoteAddress, forwardedAddresses which is not that appropriate to add that in the public interface, because FS inherits the same interface and it does not require these parameters.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        The main point is that the ClientRMService does not have direct access to the Scheduler. All access checks run through the QueueACLsManager or the ApplicationACLsManager. Any change must thus go through that. In this case the new method was introduced because the current method does not have the destination queue available. We need to check the destination queue the originating queue is already checked earlier by calling the existing method. The passed in application has not been moved yet and thus still has the original queue. Updating the application is not possible because that would pre-empt the fact that the application can and will be moved.

        The target queue checks are performed because it comes out of the move request and has not been checked at the time the access check is performed. To be able to distinguish between an access denied and a queue that does not exist the log message was added if the queue returned is empty. Without that check, and the log entries, at that point we would not be able to trace back that difference.

        I looked at folding the two methods into one to remove some code duplication but stopped with that. The small but important differences between the two methods required a number of if ... else ... constructs which made the code really difficult to read and understand.

        Show
        wilfreds Wilfred Spiegelenburg added a comment - The main point is that the ClientRMService does not have direct access to the Scheduler. All access checks run through the QueueACLsManager or the ApplicationACLsManager . Any change must thus go through that. In this case the new method was introduced because the current method does not have the destination queue available. We need to check the destination queue the originating queue is already checked earlier by calling the existing method. The passed in application has not been moved yet and thus still has the original queue. Updating the application is not possible because that would pre-empt the fact that the application can and will be moved. The target queue checks are performed because it comes out of the move request and has not been checked at the time the access check is performed. To be able to distinguish between an access denied and a queue that does not exist the log message was added if the queue returned is empty. Without that check, and the log entries, at that point we would not be able to trace back that difference. I looked at folding the two methods into one to remove some code duplication but stopped with that. The small but important differences between the two methods required a number of if ... else ... constructs which made the code really difficult to read and understand.
        Hide
        templedf Daniel Templeton added a comment -

        What you're saying then, is that the FairScheduler.checkAccess() method does the same thing, including the null check, but if the queue is null the message is logged as debug, which is not loud enough. Since it doesn't make sense to have the checkAccess() method always log louder, you're replicating the null check so that you can log it as a warn. Am I correct?

        Show
        templedf Daniel Templeton added a comment - What you're saying then, is that the FairScheduler.checkAccess() method does the same thing, including the null check, but if the queue is null the message is logged as debug, which is not loud enough. Since it doesn't make sense to have the checkAccess() method always log louder, you're replicating the null check so that you can log it as a warn. Am I correct?
        Hide
        templedf Daniel Templeton added a comment -

        Since this patch is having to replicate that code again, any objection to moving it into an overloaded CapacityScheduler.checkAccess() method (that isn't inherited from the interface)?

        Show
        templedf Daniel Templeton added a comment - Since this patch is having to replicate that code again, any objection to moving it into an overloaded CapacityScheduler.checkAccess() method (that isn't inherited from the interface)?
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Correct the checkAccess() methods does not have a way to communicate back that the queue does not exist and says that access is denied. There is no way to distinguish the two and we really want to leave some clue behind in the logs which case we have seen.
        In the normal checkAccess() case a queue that does not exist is not likely, maybe not even possible, since the queue is set on the application.

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Correct the checkAccess() methods does not have a way to communicate back that the queue does not exist and says that access is denied. There is no way to distinguish the two and we really want to leave some clue behind in the logs which case we have seen. In the normal checkAccess() case a queue that does not exist is not likely, maybe not even possible, since the queue is set on the application.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        I am all for it but I think we should do that from a follow up jira and not as part of this one.

        The reason I think that we should do it in a separate jira is that within the FairScheduler when you dig deeper the access check performed in the queue is exactly what is now done for the CapacityScheduler. The FSQueue.hasAccess() is using the same call to an YarnAuthorizationProvider as we have now in the in the QueueACLsManager for CS.

        Show
        wilfreds Wilfred Spiegelenburg added a comment - I am all for it but I think we should do that from a follow up jira and not as part of this one. The reason I think that we should do it in a separate jira is that within the FairScheduler when you dig deeper the access check performed in the queue is exactly what is now done for the CapacityScheduler. The FSQueue.hasAccess() is using the same call to an YarnAuthorizationProvider as we have now in the in the QueueACLsManager for CS.
        Hide
        templedf Daniel Templeton added a comment -

        Yep, I noticed that as well. The remoteAddress and forwardedAddress parameters that are the reason for the special casing are actually ignored under the covers, resulting in all the schedulers doing the same thing anyway.

        Show
        templedf Daniel Templeton added a comment - Yep, I noticed that as well. The remoteAddress and forwardedAddress parameters that are the reason for the special casing are actually ignored under the covers, resulting in all the schedulers doing the same thing anyway.
        Hide
        templedf Daniel Templeton added a comment -

        Let's get this thing closed out. A few more comments:

        • In ClientRMService,
                      Server.getRemoteAddress(), null, targetQueue)||

          should have a space before the pipes

        • In the new QueueACLsManager.checkAccess(), I'd really appreciate a comment that sums up the previous discussion on this JIRA so that the next person is less confused than I was
        • In TestClientRMService.getQueueAclManager(), the answer() method in the anonymous inner class should have an @Override annotation. Also, I think you'll run into problems with Java 7 and the non-final parameters being used inside the anonymous inner class
        • Same comments for createClientRMServiceForMoveApplicationRequest(), plus you shouldn't need the suppress warnings annotation now that YARN-4457 is in. You will need it in branch-2, though.
        Show
        templedf Daniel Templeton added a comment - Let's get this thing closed out. A few more comments: In ClientRMService , Server.getRemoteAddress(), null , targetQueue)|| should have a space before the pipes In the new QueueACLsManager.checkAccess() , I'd really appreciate a comment that sums up the previous discussion on this JIRA so that the next person is less confused than I was In TestClientRMService.getQueueAclManager() , the answer() method in the anonymous inner class should have an @Override annotation. Also, I think you'll run into problems with Java 7 and the non-final parameters being used inside the anonymous inner class Same comments for createClientRMServiceForMoveApplicationRequest() , plus you shouldn't need the suppress warnings annotation now that YARN-4457 is in. You will need it in branch-2, though.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Updated patch:

        • rebased to make sure it still applies to trunk after YARN-5932
        • In ClientRMService added the missing space before the pipes
        • Added a comment to the QueueACLsManager.checkAccess() to explain the versions and the scheduler dependency
        • Added @Override in TestClientRMService.getQueueAclManager() and TestClientRMService.createClientRMServiceForMoveApplicationRequest()
        • removed the suppress warnings annotations (not needed after rebase)

        The other two remarks I have left for the backport to branch-2:

        • problems with Java 7 and the non-final parameters being used inside the anonymous inner class (yes it exists)
        • suppress warnings annotation should not be needed after YARN-5932

        I will log a new jira to work on the change for the QueueACLsManager.

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Updated patch: rebased to make sure it still applies to trunk after YARN-5932 In ClientRMService added the missing space before the pipes Added a comment to the QueueACLsManager.checkAccess() to explain the versions and the scheduler dependency Added @Override in TestClientRMService.getQueueAclManager() and TestClientRMService.createClientRMServiceForMoveApplicationRequest() removed the suppress warnings annotations (not needed after rebase) The other two remarks I have left for the backport to branch-2: problems with Java 7 and the non-final parameters being used inside the anonymous inner class (yes it exists) suppress warnings annotation should not be needed after YARN-5932 I will log a new jira to work on the change for the QueueACLsManager.
        Hide
        templedf Daniel Templeton added a comment -

        Thanks, Wilfred Spiegelenburg. Can you move that checkAccess() comment into the checkAccess() method, right before the if it's talking about? While you're at it, could you fix the language/grammar of the comment text you reformatted in the pre-existing checkAccess() method?

        Show
        templedf Daniel Templeton added a comment - Thanks, Wilfred Spiegelenburg . Can you move that checkAccess() comment into the checkAccess() method, right before the if it's talking about? While you're at it, could you fix the language/grammar of the comment text you reformatted in the pre-existing checkAccess() method?
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        Fixed up the comments as per the feedback

        Show
        wilfreds Wilfred Spiegelenburg added a comment - Fixed up the comments as per the feedback
        Hide
        templedf Daniel Templeton added a comment -

        One more detail: in the new comment on the old checkAccess(), "allow users access and view the apps" should be "allow users to access and view the apps."

        I feel like we're asymptotically approaching the finish line. Thanks for hanging in there.

        Show
        templedf Daniel Templeton added a comment - One more detail: in the new comment on the old checkAccess() , "allow users access and view the apps" should be "allow users to access and view the apps." I feel like we're asymptotically approaching the finish line. Thanks for hanging in there.
        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 appears to include 1 new or modified test files.
        +1 mvninstall 13m 31s trunk passed
        +1 compile 0m 36s trunk passed
        +1 checkstyle 0m 23s trunk passed
        +1 mvnsite 0m 40s trunk passed
        +1 mvneclipse 0m 17s trunk passed
        +1 findbugs 1m 2s trunk passed
        +1 javadoc 0m 21s trunk passed
        +1 mvninstall 0m 32s the patch passed
        +1 compile 0m 30s the patch passed
        +1 javac 0m 30s the patch passed
        -0 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 1 new + 77 unchanged - 4 fixed = 78 total (was 81)
        +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 8s the patch passed
        +1 javadoc 0m 18s the patch passed
        -1 unit 39m 58s hadoop-yarn-server-resourcemanager in the patch failed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        62m 7s



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



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:a9ad5d6
        JIRA Issue YARN-5554
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845659/YARN-5554.13.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux d32e68812e1d 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 / a0a2761
        Default Java 1.8.0_111
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14554/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/14554/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/14554/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/14554/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 appears to include 1 new or modified test files. +1 mvninstall 13m 31s trunk passed +1 compile 0m 36s trunk passed +1 checkstyle 0m 23s trunk passed +1 mvnsite 0m 40s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 2s trunk passed +1 javadoc 0m 21s trunk passed +1 mvninstall 0m 32s the patch passed +1 compile 0m 30s the patch passed +1 javac 0m 30s the patch passed -0 checkstyle 0m 19s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 1 new + 77 unchanged - 4 fixed = 78 total (was 81) +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 8s the patch passed +1 javadoc 0m 18s the patch passed -1 unit 39m 58s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 62m 7s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue YARN-5554 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845659/YARN-5554.13.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d32e68812e1d 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 / a0a2761 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/14554/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/14554/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/14554/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/14554/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        fixed one checkstyle issue introduced and the one remark from the review

        Show
        wilfreds Wilfred Spiegelenburg added a comment - fixed one checkstyle issue introduced and the one remark from the review
        Hide
        templedf Daniel Templeton added a comment -

        In doing a last pass, I have two questions on the test code:

        1. In testMoveApplicationSubmitTargetQueue() and testMoveApplicationAdminTargetQueue(), would it make sense to test that the moves that are supposed to work do actually work?
        2. Why a ConcurrentHashMap in createClientRMServiceForMoveApplicationRequest() instead of Collections.singletonMap()?
        Show
        templedf Daniel Templeton added a comment - In doing a last pass, I have two questions on the test code: In testMoveApplicationSubmitTargetQueue() and testMoveApplicationAdminTargetQueue() , would it make sense to test that the moves that are supposed to work do actually work? Why a ConcurrentHashMap in createClientRMServiceForMoveApplicationRequest() instead of Collections.singletonMap() ?
        Hide
        wilfreds Wilfred Spiegelenburg added a comment -

        In testMoveApplicationSubmitTargetQueue() and testMoveApplicationAdminTargetQueue(), would it make sense to test that the moves that are supposed to work do actually work?

        The application object itself is hidden and mocked up as an object. The testing from this side is purely for the ACL checks and enforcement. The application object that would be changed is not reachable from the ClientRMService at all. It would require a lot of changes that test underlying the underlying code base more than the client service.
        Now that I think about it: we might even be able to make this far simpler if we move things around. Now that we have the move in the RMAppManager we could even think about moving all these ACL checks etc into the pre-validate check, or a security check, that is performed in the app manager. It does make more sense to have it there.

        Why a ConcurrentHashMap in createClientRMServiceForMoveApplicationRequest() instead of Collections.singletonMap()?

        I used that because the thenReturn expects a ConcurrentHashMap. The apps variable must be declared like it is. To use th singletonMap I then have to cast in the code which does not make it any more readable or maintainable. The code that works would look like this:

            ConcurrentHashMap<ApplicationId, RMApp> apps = (ConcurrentHashMap) Collections.singletonMap(applicationId, app);
            when(rmContext.getRMApps()).thenReturn(apps);
        

        That does not look any nicer than what we now have does it?

        Show
        wilfreds Wilfred Spiegelenburg added a comment - In testMoveApplicationSubmitTargetQueue() and testMoveApplicationAdminTargetQueue(), would it make sense to test that the moves that are supposed to work do actually work? The application object itself is hidden and mocked up as an object. The testing from this side is purely for the ACL checks and enforcement. The application object that would be changed is not reachable from the ClientRMService at all. It would require a lot of changes that test underlying the underlying code base more than the client service. Now that I think about it: we might even be able to make this far simpler if we move things around. Now that we have the move in the RMAppManager we could even think about moving all these ACL checks etc into the pre-validate check, or a security check, that is performed in the app manager. It does make more sense to have it there. Why a ConcurrentHashMap in createClientRMServiceForMoveApplicationRequest() instead of Collections.singletonMap()? I used that because the thenReturn expects a ConcurrentHashMap . The apps variable must be declared like it is. To use th singletonMap I then have to cast in the code which does not make it any more readable or maintainable. The code that works would look like this: ConcurrentHashMap<ApplicationId, RMApp> apps = (ConcurrentHashMap) Collections.singletonMap(applicationId, app); when(rmContext.getRMApps()).thenReturn(apps); That does not look any nicer than what we now have does it?
        Hide
        templedf Daniel Templeton added a comment -

        Sorry. Looks like I started repeating myself. TMJ! (Too Many JIRAs!)

        Fine by me. +1. I'll commit when I get a chance.

        Show
        templedf Daniel Templeton added a comment - Sorry. Looks like I started repeating myself. TMJ! (Too Many JIRAs!) Fine by me. +1. I'll commit when I get a chance.
        Hide
        templedf Daniel Templeton added a comment -

        Looks like we'll need a branch-2 patch. Can you take care of that, Wilfred Spiegelenburg?

        Show
        templedf Daniel Templeton added a comment - Looks like we'll need a branch-2 patch. Can you take care of that, Wilfred Spiegelenburg ?
        Hide
        templedf Daniel Templeton added a comment -

        Thanks for the patch, Wilfred Spiegelenburg. Committed to trunk. Waiting for the branch-2 patch to commit there.

        Show
        templedf Daniel Templeton added a comment - Thanks for the patch, Wilfred Spiegelenburg . Committed to trunk. Waiting for the branch-2 patch to commit there.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #11108 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11108/)
        YARN-5554. MoveApplicationAcrossQueues does not check user permission on (templedf: rev 7979939428ad5df213846e11bc1489bdf94ed9f8)

        • (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/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestClientRMService.java
        • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/QueueACLsManager.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #11108 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11108/ ) YARN-5554 . MoveApplicationAcrossQueues does not check user permission on (templedf: rev 7979939428ad5df213846e11bc1489bdf94ed9f8) (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/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestClientRMService.java (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/QueueACLsManager.java

          People

          • Assignee:
            wilfreds Wilfred Spiegelenburg
            Reporter:
            haibochen Haibo Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            15 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development