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

RM web ui for dumping scheduler logs should be for admins only

    Details

    • Target Version/s:

      Description

      YARN-3294 allows users to dump scheduler logs from the web UI. This should be for admins only.

      1. YARN-3517.006.patch
        13 kB
        Varun Vasudev
      2. YARN-3517.005.patch
        12 kB
        Varun Vasudev
      3. YARN-3517.004.patch
        12 kB
        Varun Vasudev
      4. YARN-3517.003.patch
        11 kB
        Varun Vasudev
      5. YARN-3517.002.patch
        11 kB
        Varun Vasudev
      6. YARN-3517.001.patch
        6 kB
        Varun Vasudev

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2129 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2129/)
          YARN-3517. RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java
          • hadoop-yarn-project/CHANGES.txt
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2129 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2129/ ) YARN-3517 . RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java hadoop-yarn-project/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #180 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/180/)
          YARN-3517. RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #180 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/180/ ) YARN-3517 . RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #913 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/913/)
          YARN-3517. RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2)

          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #913 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/913/ ) YARN-3517 . RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2) hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #170 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/170/)
          YARN-3517. RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #170 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/170/ ) YARN-3517 . RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #179 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/179/)
          YARN-3517. RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          • hadoop-yarn-project/CHANGES.txt
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #179 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/179/ ) YARN-3517 . RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #2111 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2111/)
          YARN-3517. RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          • hadoop-yarn-project/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2111 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2111/ ) YARN-3517 . RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java hadoop-yarn-project/CHANGES.txt
          Hide
          tgraves Thomas Graves added a comment -

          thanks Vinod Kumar Vavilapalli I missed that.

          Show
          tgraves Thomas Graves added a comment - thanks Vinod Kumar Vavilapalli I missed that.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Seems like the JIRA assignee got mixed up, fixing..

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Seems like the JIRA assignee got mixed up, fixing..
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #7701 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7701/)
          YARN-3517. RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2)

          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java
          • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
          • hadoop-yarn-project/CHANGES.txt
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #7701 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7701/ ) YARN-3517 . RM web ui for dumping scheduler logs should be for admins only (Varun Vasudev via tgraves) (tgraves: rev 2e215484bd05cd5e3b7a81d3558c6879a05dd2d2) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/CapacitySchedulerPage.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServices.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/server/security/ApplicationACLsManager.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java hadoop-yarn-project/CHANGES.txt
          Hide
          tgraves Thomas Graves added a comment -

          changes look good, +1. thanks Varun Vasudev

          Show
          tgraves Thomas Graves added a comment - changes look good, +1. thanks Varun Vasudev
          Hide
          vvasudev Varun Vasudev added a comment -

          The test failure is unrelated to the patch.

          Show
          vvasudev Varun Vasudev added a comment - The test failure is unrelated to the patch.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 44s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
          +1 whitespace 0m 0s The patch has no lines that end in whitespace.
          +1 javac 7m 33s There were no new javac warning messages.
          +1 javadoc 9m 35s There were no new javadoc warning messages.
          +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
          +1 checkstyle 3m 55s There were no new checkstyle issues.
          +1 install 1m 34s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 2m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 yarn tests 1m 54s Tests passed in hadoop-yarn-common.
          -1 yarn tests 53m 7s Tests failed in hadoop-yarn-server-resourcemanager.
              96m 0s  



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



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12729158/YARN-3517.006.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 8f82970
          hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7540/artifact/patchprocess/testrun_hadoop-yarn-common.txt
          hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7540/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7540/testReport/
          Java 1.7.0_55
          uname Linux asf901.gq1.ygridcore.net 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
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7540/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 44s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 javac 7m 33s There were no new javac warning messages. +1 javadoc 9m 35s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 3m 55s There were no new checkstyle issues. +1 install 1m 34s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 2m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 1m 54s Tests passed in hadoop-yarn-common. -1 yarn tests 53m 7s Tests failed in hadoop-yarn-server-resourcemanager.     96m 0s   Reason Tests Failed unit tests hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12729158/YARN-3517.006.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 8f82970 hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7540/artifact/patchprocess/testrun_hadoop-yarn-common.txt hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7540/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7540/testReport/ Java 1.7.0_55 uname Linux asf901.gq1.ygridcore.net 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 Console output https://builds.apache.org/job/PreCommit-YARN-Build/7540/console This message was automatically generated.
          Hide
          vvasudev Varun Vasudev added a comment -

          in RMWebServices.java we don't need the isSecurityEnabled check. Just remove the entire check. My reasoning is that logLevel app does not do those checks, it simply makes sure you are an admin.

          + if (UserGroupInformation.isSecurityEnabled() && callerUGI == null)
          { + String msg = "Unable to obtain user name, user not authenticated"; + throw new AuthorizationException(msg); + }

          Removed the check.

          in the test TestRMWebServices.java. We aren't actually asserting anything. we should assert that the expected files exist. Personally I would also like to see an assert that the expected exception occurred.

          Added explicit check for the exception being thrown as well as a check for the log files existing.

          Show
          vvasudev Varun Vasudev added a comment - in RMWebServices.java we don't need the isSecurityEnabled check. Just remove the entire check. My reasoning is that logLevel app does not do those checks, it simply makes sure you are an admin. + if (UserGroupInformation.isSecurityEnabled() && callerUGI == null) { + String msg = "Unable to obtain user name, user not authenticated"; + throw new AuthorizationException(msg); + } Removed the check. in the test TestRMWebServices.java. We aren't actually asserting anything. we should assert that the expected files exist. Personally I would also like to see an assert that the expected exception occurred. Added explicit check for the exception being thrown as well as a check for the log files existing.
          Hide
          hadoopqa Hadoop QA added a comment -



          +1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 39s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
          +1 whitespace 0m 0s The patch has no lines that end in whitespace.
          +1 javac 7m 36s There were no new javac warning messages.
          +1 javadoc 9m 36s There were no new javadoc warning messages.
          +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
          +1 checkstyle 5m 20s There were no new checkstyle issues.
          +1 install 1m 34s mvn install still works.
          +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
          +1 findbugs 2m 37s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 yarn tests 1m 57s Tests passed in hadoop-yarn-common.
          +1 yarn tests 52m 10s Tests passed in hadoop-yarn-server-resourcemanager.
              96m 27s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12727691/YARN-3517.005.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 5190923
          hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7528/artifact/patchprocess/testrun_hadoop-yarn-common.txt
          hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7528/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7528/testReport/
          Java 1.7.0_55
          uname Linux asf905.gq1.ygridcore.net 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
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7528/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 39s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 javac 7m 36s There were no new javac warning messages. +1 javadoc 9m 36s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 5m 20s There were no new checkstyle issues. +1 install 1m 34s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 2m 37s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 1m 57s Tests passed in hadoop-yarn-common. +1 yarn tests 52m 10s Tests passed in hadoop-yarn-server-resourcemanager.     96m 27s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12727691/YARN-3517.005.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 5190923 hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7528/artifact/patchprocess/testrun_hadoop-yarn-common.txt hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7528/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7528/testReport/ Java 1.7.0_55 uname Linux asf905.gq1.ygridcore.net 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 Console output https://builds.apache.org/job/PreCommit-YARN-Build/7528/console This message was automatically generated.
          Hide
          tgraves Thomas Graves added a comment -

          in RMWebServices.java we don't need the isSecurityEnabled check. Just remove the entire check. My reasoning is that logLevel app does not do those checks, it simply makes sure you are an admin.

          + if (UserGroupInformation.isSecurityEnabled() && callerUGI == null)

          { + String msg = "Unable to obtain user name, user not authenticated"; + throw new AuthorizationException(msg); + }

          in the test TestRMWebServices.java. We aren't actually asserting anything. we should assert that the expected files exist. Personally I would also like to see an assert that the expected exception occurred.

          Show
          tgraves Thomas Graves added a comment - in RMWebServices.java we don't need the isSecurityEnabled check. Just remove the entire check. My reasoning is that logLevel app does not do those checks, it simply makes sure you are an admin. + if (UserGroupInformation.isSecurityEnabled() && callerUGI == null) { + String msg = "Unable to obtain user name, user not authenticated"; + throw new AuthorizationException(msg); + } in the test TestRMWebServices.java. We aren't actually asserting anything. we should assert that the expected files exist. Personally I would also like to see an assert that the expected exception occurred.
          Hide
          vvasudev Varun Vasudev added a comment -

          Thomas Graves - if possible, can you please review the latest patch?

          Show
          vvasudev Varun Vasudev added a comment - Thomas Graves - if possible, can you please review the latest patch?
          Hide
          vvasudev Varun Vasudev added a comment -

          It can be null if no authentication filter is setup. Also, the aclsManager doesn't handle a null callerUGI and ends up throwing a null pointer exception.

          Show
          vvasudev Varun Vasudev added a comment - It can be null if no authentication filter is setup. Also, the aclsManager doesn't handle a null callerUGI and ends up throwing a null pointer exception.
          Hide
          jianhe Jian He added a comment -

          I think below callerUGI can never never null

              UserGroupInformation callerUGI = getCallerUserGroupInformation(hsr, true);
              if (UserGroupInformation.isSecurityEnabled() && callerUGI == null) {
                String msg = "Unable to obtain user name, user not authenticated";
                throw new AuthorizationException(msg);
              }
          
           if (callerUGI != null && aclsManager.isAdmin(callerUGI)) {
          
          Show
          jianhe Jian He added a comment - I think below callerUGI can never never null UserGroupInformation callerUGI = getCallerUserGroupInformation(hsr, true ); if (UserGroupInformation.isSecurityEnabled() && callerUGI == null ) { String msg = "Unable to obtain user name, user not authenticated" ; throw new AuthorizationException(msg); } if (callerUGI != null && aclsManager.isAdmin(callerUGI)) {
          Hide
          hadoopqa Hadoop QA added a comment -



          +1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 52s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
          +1 whitespace 0m 0s The patch has no lines that end in whitespace.
          +1 javac 7m 34s There were no new javac warning messages.
          +1 javadoc 9m 31s There were no new javadoc warning messages.
          +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
          +1 checkstyle 5m 24s There were no new checkstyle issues.
          +1 install 1m 33s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 2m 39s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 yarn tests 1m 55s Tests passed in hadoop-yarn-common.
          +1 yarn tests 52m 23s Tests passed in hadoop-yarn-server-resourcemanager.
              96m 50s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12727691/YARN-3517.005.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 416b843
          hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7481/artifact/patchprocess/testrun_hadoop-yarn-common.txt
          hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7481/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7481/testReport/
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7481/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 52s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 javac 7m 34s There were no new javac warning messages. +1 javadoc 9m 31s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 5m 24s There were no new checkstyle issues. +1 install 1m 33s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 2m 39s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 1m 55s Tests passed in hadoop-yarn-common. +1 yarn tests 52m 23s Tests passed in hadoop-yarn-server-resourcemanager.     96m 50s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12727691/YARN-3517.005.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 416b843 hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7481/artifact/patchprocess/testrun_hadoop-yarn-common.txt hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7481/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7481/testReport/ Console output https://builds.apache.org/job/PreCommit-YARN-Build/7481/console This message was automatically generated.
          Hide
          vvasudev Varun Vasudev added a comment -

          Uploaded a new patch to fix the whitespace and checkstyle errors. The test failure is unrelated to the patch.

          Show
          vvasudev Varun Vasudev added a comment - Uploaded a new patch to fix the whitespace and checkstyle errors. The test failure is unrelated to the patch.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 38s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace.
          +1 javac 7m 30s There were no new javac warning messages.
          +1 javadoc 9m 38s There were no new javadoc warning messages.
          +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 5m 24s The applied patch generated 2 additional checkstyle issues.
          +1 install 1m 32s mvn install still works.
          +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
          +1 findbugs 2m 39s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 yarn tests 2m 2s Tests passed in hadoop-yarn-common.
          -1 yarn tests 53m 59s Tests failed in hadoop-yarn-server-resourcemanager.
              98m 18s  



          Reason Tests
          Failed unit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacitySchedulerNodeLabelUpdate



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12727652/YARN-3517.004.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 49f6e3d
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/whitespace.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/checkstyle-result-diff.txt
          hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/testrun_hadoop-yarn-common.txt
          hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7478/testReport/
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/7478/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 38s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. +1 javac 7m 30s There were no new javac warning messages. +1 javadoc 9m 38s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 5m 24s The applied patch generated 2 additional checkstyle issues. +1 install 1m 32s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 2m 39s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 2m 2s Tests passed in hadoop-yarn-common. -1 yarn tests 53m 59s Tests failed in hadoop-yarn-server-resourcemanager.     98m 18s   Reason Tests Failed unit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestCapacitySchedulerNodeLabelUpdate Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12727652/YARN-3517.004.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 49f6e3d whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/whitespace.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/checkstyle-result-diff.txt hadoop-yarn-common test log https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/testrun_hadoop-yarn-common.txt hadoop-yarn-server-resourcemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7478/artifact/patchprocess/testrun_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7478/testReport/ Console output https://builds.apache.org/job/PreCommit-YARN-Build/7478/console This message was automatically generated.
          Hide
          vvasudev Varun Vasudev added a comment -

          We don't need the isSecurityEnabled check, just keep the one for areAclsEnabled. This could be combined with the previous if, make this the else if part but that isn't a big deal.

          in QueuesBlock we are creating the AdminACLsManager every web page load. Perhaps a better way would be to use the this.rm.getApplicationACLsManager() and extend the ApplicationAclsManager to explose an isAdmin functionality

          Uploaded a new patch with changes. I also unset the affect version - the original patch didn't make it into 2.7.

          Show
          vvasudev Varun Vasudev added a comment - We don't need the isSecurityEnabled check, just keep the one for areAclsEnabled. This could be combined with the previous if, make this the else if part but that isn't a big deal. in QueuesBlock we are creating the AdminACLsManager every web page load. Perhaps a better way would be to use the this.rm.getApplicationACLsManager() and extend the ApplicationAclsManager to explose an isAdmin functionality Uploaded a new patch with changes. I also unset the affect version - the original patch didn't make it into 2.7.
          Hide
          tgraves Thomas Graves added a comment -

          + // non-secure mode with no acls enabled
          + if (!isAdmin && !UserGroupInformation.isSecurityEnabled()
          + && !adminACLsManager.areACLsEnabled())

          { + isAdmin = true; + }

          +

          We don't need the isSecurityEnabled check, just keep the one for areAclsEnabled. This could be combined with the previous if, make this the else if part but that isn't a big deal.

          in QueuesBlock we are creating the AdminACLsManager every web page load. Perhaps a better way would be to use the this.rm.getApplicationACLsManager() and extend the ApplicationAclsManager to explose an isAdmin functionality

          Show
          tgraves Thomas Graves added a comment - + // non-secure mode with no acls enabled + if (!isAdmin && !UserGroupInformation.isSecurityEnabled() + && !adminACLsManager.areACLsEnabled()) { + isAdmin = true; + } + We don't need the isSecurityEnabled check, just keep the one for areAclsEnabled. This could be combined with the previous if, make this the else if part but that isn't a big deal. in QueuesBlock we are creating the AdminACLsManager every web page load. Perhaps a better way would be to use the this.rm.getApplicationACLsManager() and extend the ApplicationAclsManager to explose an isAdmin functionality
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12726919/YARN-3517.003.patch
          against trunk revision 8ddbb8d.

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

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

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

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

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

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

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

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

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

          This message is automatically generated.

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

          Thanks Varun Vasudev
          Patch looks good.

          Show
          sunilg Sunil G added a comment - Thanks Varun Vasudev Patch looks good.
          Hide
          sunilg Sunil G added a comment -

          Thanks Varun Vasudev
          Patch looks good.

          Show
          sunilg Sunil G added a comment - Thanks Varun Vasudev Patch looks good.
          Hide
          vvasudev Varun Vasudev added a comment -

          If adminACLsManager.areACLsEnabled() is false, do we need above check?

          Fixed.

          Can be changed as "Are you sure to generate"

          I think the current statement is ok.

          Show
          vvasudev Varun Vasudev added a comment - If adminACLsManager.areACLsEnabled() is false, do we need above check? Fixed. Can be changed as "Are you sure to generate" I think the current statement is ok.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          This should go into 2.8.0 given YARN-3294 did.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - This should go into 2.8.0 given YARN-3294 did.
          Hide
          sunilg Sunil G added a comment -

          HI Varun Vasudev

          I have couple of comments on same..

          1.

          +      if (callerUGI != null && adminACLsManager.isAdmin(callerUGI)) {
          +        isAdmin = true;
          +      }
          

          If adminACLsManager.areACLsEnabled() is false, do we need above check?

          2. a minor nit

          .append(" b = confirm(\"Are you sure you wish to generate"

          Can be changed as "Are you sure to generate"

          Show
          sunilg Sunil G added a comment - HI Varun Vasudev I have couple of comments on same.. 1. + if (callerUGI != null && adminACLsManager.isAdmin(callerUGI)) { + isAdmin = true ; + } If adminACLsManager.areACLsEnabled() is false, do we need above check? 2. a minor nit .append( " b = confirm(\" Are you sure you wish to generate" Can be changed as "Are you sure to generate"
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12726890/YARN-3517.002.patch
          against trunk revision 8ddbb8d.

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

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

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

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

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

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

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

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

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

          This message is automatically generated.

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

          Uploaded a new patch to address Thomas's comments.

          Could you also change it to not show the button if you aren't an admin?

          Fixed.

          One other thing is could you add some css or something to make it look more like a button. Right now it just looks like text and I didn't know it was clickable at first. The placement of it seems a bit weird to me also but as along as its only showing up for admins that is less of an issue.

          I've added some style elements to make it look better.

          I haven't looked at the patch if details but I see we are creating a new AdminACLsManager each time. It would be nice if we didn't have to do that.

          Fixed.

          Show
          vvasudev Varun Vasudev added a comment - Uploaded a new patch to address Thomas's comments. Could you also change it to not show the button if you aren't an admin? Fixed. One other thing is could you add some css or something to make it look more like a button. Right now it just looks like text and I didn't know it was clickable at first. The placement of it seems a bit weird to me also but as along as its only showing up for admins that is less of an issue. I've added some style elements to make it look better. I haven't looked at the patch if details but I see we are creating a new AdminACLsManager each time. It would be nice if we didn't have to do that. Fixed.
          Hide
          tgraves Thomas Graves added a comment -

          Thanks for following up on this. Could you also change it to not show the button if you aren't an admin? I don't want to confuse users by having a button there that doesn't do anything.

          One other thing is could you add some css or something to make it look more like a button. Right now it just looks like text and I didn't know it was clickable at first. The placement of it seems a bit weird to me also but as along as its only showing up for admins that is less of an issue.

          I haven't looked at the patch if details but I see we are creating a new AdminACLsManager each time. It would be nice if we didn't have to do that.

          Show
          tgraves Thomas Graves added a comment - Thanks for following up on this. Could you also change it to not show the button if you aren't an admin? I don't want to confuse users by having a button there that doesn't do anything. One other thing is could you add some css or something to make it look more like a button. Right now it just looks like text and I didn't know it was clickable at first. The placement of it seems a bit weird to me also but as along as its only showing up for admins that is less of an issue. I haven't looked at the patch if details but I see we are creating a new AdminACLsManager each time. It would be nice if we didn't have to do that.
          Hide
          vvasudev Varun Vasudev added a comment -

          Test failure is unrelated.

          Show
          vvasudev Varun Vasudev added a comment - Test failure is unrelated.
          Hide
          hadoopqa Hadoop QA added a comment -

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

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

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

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

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

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

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

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

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

          org.apache.hadoop.yarn.server.resourcemanager.applicationsmanager.TestAMRestart

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

          This message is automatically generated.

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

          Uploaded patch with fix.

          Show
          vvasudev Varun Vasudev added a comment - Uploaded patch with fix.

            People

            • Assignee:
              vvasudev Varun Vasudev
              Reporter:
              vvasudev Varun Vasudev
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development