Details

    • Type: Sub-task
    • Status: Patch Available
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      As detailed in the proposal in the umbrella JIRA, we are introducing a new component that routes client request to appropriate ResourceManager(s). This JIRA tracks the creation of a proxy for ResourceManager REST API in the Router. This provides a placeholder for:
      1) throttling mis-behaving clients (YARN-1546)
      3) mask the access to multiple RMs (YARN-3659)

      We are planning to follow the interceptor pattern like we did in YARN-2884 to generalize the approach and have only dynamically coupling for Federation.

      1. YARN-5412-YARN-2915.1.patch
        257 kB
        Giovanni Matteo Fumarola
      2. YARN-5412-YARN-2915.2.patch
        213 kB
        Giovanni Matteo Fumarola
      3. YARN-5412-YARN-2915.3.patch
        200 kB
        Giovanni Matteo Fumarola
      4. YARN-5412-YARN-2915.4.patch
        204 kB
        Giovanni Matteo Fumarola
      5. YARN-5412-YARN-2915.proto.patch
        258 kB
        Giovanni Matteo Fumarola

        Issue Links

          Activity

          Hide
          curino Carlo Curino added a comment -

          Giovanni Matteo Fumarola I rebased the branch YARN-2915 to pick YARN-6587.

          Show
          curino Carlo Curino added a comment - Giovanni Matteo Fumarola I rebased the branch YARN-2915 to pick YARN-6587 .
          Hide
          giovanni.fumarola Giovanni Matteo Fumarola added a comment -

          Attached a proto version.
          Some considerations:

          • NodeIDsInfo contained a bug - fixed in this patch.
          • LabelsToNodesInfo missed a constructor - fixed in this patch.
          • I created in TestRouterWebServicesREST something similar to a MiniYarnCluster. I am really curious to see what Jenkins acts with this.
          • Some Javadoc are missing - I will add them in v1.
          • it may contains some Yetus warnings - I will fix them in v1.
          Show
          giovanni.fumarola Giovanni Matteo Fumarola added a comment - Attached a proto version. Some considerations: NodeIDsInfo contained a bug - fixed in this patch. LabelsToNodesInfo missed a constructor - fixed in this patch. I created in TestRouterWebServicesREST something similar to a MiniYarnCluster. I am really curious to see what Jenkins acts with this. Some Javadoc are missing - I will add them in v1. it may contains some Yetus warnings - I will fix them in v1.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 8 new or modified test files.
                YARN-2915 Compile Tests
          0 mvndep 0m 23s Maven dependency ordering for branch
          +1 mvninstall 16m 54s YARN-2915 passed
          +1 compile 8m 40s YARN-2915 passed
          +1 checkstyle 0m 56s YARN-2915 passed
          +1 mvnsite 2m 16s YARN-2915 passed
          +1 findbugs 3m 44s YARN-2915 passed
          +1 javadoc 1m 47s YARN-2915 passed
                Patch Compile Tests
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 42s the patch passed
          +1 compile 5m 21s the patch passed
          +1 javac 5m 21s the patch passed
          -0 checkstyle 0m 54s hadoop-yarn-project/hadoop-yarn: The patch generated 27 new + 235 unchanged - 1 fixed = 262 total (was 236)
          +1 mvnsite 2m 13s the patch passed
          -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 4m 14s the patch passed
          -1 javadoc 0m 19s hadoop-yarn-server-router in the patch failed.
                Other Tests
          -1 unit 0m 33s hadoop-yarn-api in the patch failed.
          +1 unit 2m 24s hadoop-yarn-common in the patch passed.
          -1 unit 39m 32s hadoop-yarn-server-resourcemanager in the patch failed.
          -1 unit 0m 26s hadoop-yarn-server-router in the patch failed.
          +1 asflicense 0m 35s The patch does not generate ASF License warnings.
          103m 18s



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



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-5412
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876287/YARN-5412-YARN-2915.proto.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux d25ecc5184fc 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-2915 / 590d959
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/whitespace-eol.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16339/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16339/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 8 new or modified test files.       YARN-2915 Compile Tests 0 mvndep 0m 23s Maven dependency ordering for branch +1 mvninstall 16m 54s YARN-2915 passed +1 compile 8m 40s YARN-2915 passed +1 checkstyle 0m 56s YARN-2915 passed +1 mvnsite 2m 16s YARN-2915 passed +1 findbugs 3m 44s YARN-2915 passed +1 javadoc 1m 47s YARN-2915 passed       Patch Compile Tests 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 42s the patch passed +1 compile 5m 21s the patch passed +1 javac 5m 21s the patch passed -0 checkstyle 0m 54s hadoop-yarn-project/hadoop-yarn: The patch generated 27 new + 235 unchanged - 1 fixed = 262 total (was 236) +1 mvnsite 2m 13s the patch passed -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 4m 14s the patch passed -1 javadoc 0m 19s hadoop-yarn-server-router in the patch failed.       Other Tests -1 unit 0m 33s hadoop-yarn-api in the patch failed. +1 unit 2m 24s hadoop-yarn-common in the patch passed. -1 unit 39m 32s hadoop-yarn-server-resourcemanager in the patch failed. -1 unit 0m 26s hadoop-yarn-server-router in the patch failed. +1 asflicense 0m 35s The patch does not generate ASF License warnings. 103m 18s Reason Tests Failed junit tests hadoop.yarn.conf.TestYarnConfigurationFields   hadoop.yarn.server.resourcemanager.TestRMRestart Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-5412 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876287/YARN-5412-YARN-2915.proto.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux d25ecc5184fc 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-2915 / 590d959 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/whitespace-eol.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16339/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16339/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/16339/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          giovanni.fumarola Giovanni Matteo Fumarola added a comment -

          Attached v1 - with some fixed checkstyle warnings and the correct pom.xml for hadoop-yarn-server.router package.

          Show
          giovanni.fumarola Giovanni Matteo Fumarola added a comment - Attached v1 - with some fixed checkstyle warnings and the correct pom.xml for hadoop-yarn-server.router package.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 8 new or modified test files.
                YARN-2915 Compile Tests
          0 mvndep 0m 24s Maven dependency ordering for branch
          +1 mvninstall 18m 3s YARN-2915 passed
          +1 compile 9m 17s YARN-2915 passed
          +1 checkstyle 0m 56s YARN-2915 passed
          +1 mvnsite 2m 18s YARN-2915 passed
          +1 findbugs 3m 55s YARN-2915 passed
          +1 javadoc 1m 49s YARN-2915 passed
                Patch Compile Tests
          0 mvndep 0m 13s Maven dependency ordering for patch
          +1 mvninstall 2m 4s the patch passed
          +1 compile 6m 18s the patch passed
          +1 javac 6m 18s the patch passed
          -0 checkstyle 0m 59s hadoop-yarn-project/hadoop-yarn: The patch generated 22 new + 235 unchanged - 1 fixed = 257 total (was 236)
          +1 mvnsite 2m 30s the patch passed
          -1 whitespace 0m 0s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          -1 whitespace 0m 0s The patch 4 line(s) with tabs.
          +1 xml 0m 4s The patch has no ill-formed XML file.
          +1 findbugs 4m 59s the patch passed
          -1 javadoc 0m 21s hadoop-yarn-server-router in the patch failed.
                Other Tests
          +1 unit 0m 38s hadoop-yarn-api in the patch passed.
          +1 unit 2m 44s hadoop-yarn-common in the patch passed.
          -1 unit 41m 4s hadoop-yarn-server-resourcemanager in the patch failed.
          -1 unit 15m 31s hadoop-yarn-server-router in the patch failed.
          +1 asflicense 0m 32s The patch does not generate ASF License warnings.
          125m 9s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesHttpStaticUserPermissions
            hadoop.yarn.server.resourcemanager.webapp.TestRMWebappAuthentication
            hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesDelegationTokenAuthentication
            hadoop.yarn.server.router.webapp.TestRouterWebServices
            hadoop.yarn.server.router.rmadmin.TestRouterRMAdminService
            hadoop.yarn.server.router.clientrm.TestRouterClientRMService
          Timed out junit tests org.apache.hadoop.yarn.server.router.webapp.TestRouterWebServicesREST



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-5412
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876454/YARN-5412-YARN-2915.1.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux e71bac3f0d41 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-2915 / 590d959
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/whitespace-eol.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/whitespace-tabs.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16351/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16351/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 8 new or modified test files.       YARN-2915 Compile Tests 0 mvndep 0m 24s Maven dependency ordering for branch +1 mvninstall 18m 3s YARN-2915 passed +1 compile 9m 17s YARN-2915 passed +1 checkstyle 0m 56s YARN-2915 passed +1 mvnsite 2m 18s YARN-2915 passed +1 findbugs 3m 55s YARN-2915 passed +1 javadoc 1m 49s YARN-2915 passed       Patch Compile Tests 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 2m 4s the patch passed +1 compile 6m 18s the patch passed +1 javac 6m 18s the patch passed -0 checkstyle 0m 59s hadoop-yarn-project/hadoop-yarn: The patch generated 22 new + 235 unchanged - 1 fixed = 257 total (was 236) +1 mvnsite 2m 30s the patch passed -1 whitespace 0m 0s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply -1 whitespace 0m 0s The patch 4 line(s) with tabs. +1 xml 0m 4s The patch has no ill-formed XML file. +1 findbugs 4m 59s the patch passed -1 javadoc 0m 21s hadoop-yarn-server-router in the patch failed.       Other Tests +1 unit 0m 38s hadoop-yarn-api in the patch passed. +1 unit 2m 44s hadoop-yarn-common in the patch passed. -1 unit 41m 4s hadoop-yarn-server-resourcemanager in the patch failed. -1 unit 15m 31s hadoop-yarn-server-router in the patch failed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 125m 9s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesHttpStaticUserPermissions   hadoop.yarn.server.resourcemanager.webapp.TestRMWebappAuthentication   hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesDelegationTokenAuthentication   hadoop.yarn.server.router.webapp.TestRouterWebServices   hadoop.yarn.server.router.rmadmin.TestRouterRMAdminService   hadoop.yarn.server.router.clientrm.TestRouterClientRMService Timed out junit tests org.apache.hadoop.yarn.server.router.webapp.TestRouterWebServicesREST Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-5412 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876454/YARN-5412-YARN-2915.1.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux e71bac3f0d41 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-2915 / 590d959 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/whitespace-eol.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/whitespace-tabs.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16351/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16351/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/16351/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          curino Carlo Curino added a comment -

          Giovanni, thanks for the contribution. Please find below a partial review (patch is large going through it slowly, and posting comments
          so you can parallelize addressing them).

          BUG?

          1. DefaultRequestInterceptorREST.init does not propagate correctly to downstream interceptors... you might want to have a "localInit" method that is implemented to locally initialize the components, while the AbstractRESTRequestInterceptor takes care of propagating to downstream, by invoking the outward init

          MUST HAVE CLEANUPS

          1. In the next version of patch, please remove the changes you already pushed in YARN-6792
          2. DefaultRequestInterceptorREST there is lots of repeting code doing the "check status" stuff, which could be solved with a single method that receives in input a string-constant and the .class of the expected outcome.
          3. RESTRequestInterceptor there are a few (uncommented) methods, which I am not sure belong here. Shall we move them to RMWebServiceProtocol? Help me understand why e.g., getContainer should be here.

          NITS

          1. yarn-default.xml the comment is not very clear about how the list of classes make up the pipeline.
          2. AbstractRESTRequestInterceptor javadoc errors "can can"
          3. (RMWSConsts.STATES is a confusing name, something like CONTAINER_STATES?
          4. RESTRequestInterceptor the javadoc of setNextInterceptor is confusing. It says the last interceptor does not receive this call, while it will likely receive it with a null value, correct?

          GOOD TO HAVE

          1. RMWebAppUtil.isStaticUser is invoked a lot in RMWebServices, and every time it performs a read from conf. Can we cache the value of staticUser, so this method gets much lighter to invoke?
          2. DefaultRequestInterceptorREST Can we try to use generics to remove lots of the almost-code-repetition we currently have?

          QUESTIONS:

          1. can we scope-down to tests the import of nodemanager project?
          2. why does RESTRequestInterceptor.init has a "user" param? Javadoc is incomplete, and it is not used by DefaultRequestInterceptorREST so hard to understand.

          [...] more to come (I will re-post the entire list adding to it, so numbering remains consistent... we can strike-out elements as you do them

          Show
          curino Carlo Curino added a comment - Giovanni, thanks for the contribution. Please find below a partial review (patch is large going through it slowly, and posting comments so you can parallelize addressing them). BUG? DefaultRequestInterceptorREST.init does not propagate correctly to downstream interceptors... you might want to have a "localInit" method that is implemented to locally initialize the components, while the AbstractRESTRequestInterceptor takes care of propagating to downstream, by invoking the outward init MUST HAVE CLEANUPS In the next version of patch, please remove the changes you already pushed in YARN-6792 DefaultRequestInterceptorREST there is lots of repeting code doing the "check status" stuff, which could be solved with a single method that receives in input a string-constant and the .class of the expected outcome. RESTRequestInterceptor there are a few (uncommented) methods, which I am not sure belong here. Shall we move them to RMWebServiceProtocol? Help me understand why e.g., getContainer should be here. NITS yarn-default.xml the comment is not very clear about how the list of classes make up the pipeline. AbstractRESTRequestInterceptor javadoc errors "can can" (RMWSConsts.STATES is a confusing name, something like CONTAINER_STATES? RESTRequestInterceptor the javadoc of setNextInterceptor is confusing. It says the last interceptor does not receive this call, while it will likely receive it with a null value, correct? GOOD TO HAVE RMWebAppUtil.isStaticUser is invoked a lot in RMWebServices , and every time it performs a read from conf. Can we cache the value of staticUser, so this method gets much lighter to invoke? DefaultRequestInterceptorREST Can we try to use generics to remove lots of the almost-code-repetition we currently have? QUESTIONS: can we scope-down to tests the import of nodemanager project? why does RESTRequestInterceptor.init has a "user" param? Javadoc is incomplete, and it is not used by DefaultRequestInterceptorREST so hard to understand. [...] more to come (I will re-post the entire list adding to it, so numbering remains consistent... we can strike-out elements as you do them
          Hide
          giovanni.fumarola Giovanni Matteo Fumarola added a comment - - edited

          Thanks Carlo Curino for the feedback - I will fix them in the next iteration.
          I attached v2 - It reduces the code from DefaultRequestInterceptorREST and fixes the new junit tests.

          Show
          giovanni.fumarola Giovanni Matteo Fumarola added a comment - - edited Thanks Carlo Curino for the feedback - I will fix them in the next iteration. I attached v2 - It reduces the code from DefaultRequestInterceptorREST and fixes the new junit tests.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 8 new or modified test files.
                YARN-2915 Compile Tests
          0 mvndep 0m 26s Maven dependency ordering for branch
          +1 mvninstall 16m 48s YARN-2915 passed
          +1 compile 9m 36s YARN-2915 passed
          +1 checkstyle 0m 57s YARN-2915 passed
          +1 mvnsite 2m 14s YARN-2915 passed
          +1 findbugs 3m 44s YARN-2915 passed
          +1 javadoc 1m 47s YARN-2915 passed
                Patch Compile Tests
          0 mvndep 0m 11s Maven dependency ordering for patch
          +1 mvninstall 1m 44s the patch passed
          +1 compile 5m 39s the patch passed
          +1 javac 5m 39s the patch passed
          -0 checkstyle 1m 1s hadoop-yarn-project/hadoop-yarn: The patch generated 25 new + 223 unchanged - 1 fixed = 248 total (was 224)
          +1 mvnsite 2m 19s the patch passed
          -1 whitespace 0m 0s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          -1 whitespace 0m 0s The patch 15 line(s) with tabs.
          +1 xml 0m 4s The patch has no ill-formed XML file.
          +1 findbugs 4m 16s the patch passed
          -1 javadoc 0m 20s hadoop-yarn-server-router in the patch failed.
                Other Tests
          +1 unit 0m 32s hadoop-yarn-api in the patch passed.
          +1 unit 2m 32s hadoop-yarn-common in the patch passed.
          -1 unit 40m 57s hadoop-yarn-server-resourcemanager in the patch failed.
          -1 unit 15m 31s hadoop-yarn-server-router in the patch failed.
          +1 asflicense 0m 32s The patch does not generate ASF License warnings.
          121m 26s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesHttpStaticUserPermissions
            hadoop.yarn.server.resourcemanager.TestRMRestart
            hadoop.yarn.server.resourcemanager.webapp.TestRMWebappAuthentication
            hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesDelegationTokenAuthentication
            hadoop.yarn.server.router.webapp.TestRouterWebServices
            hadoop.yarn.server.router.rmadmin.TestRouterRMAdminService
            hadoop.yarn.server.router.clientrm.TestRouterClientRMService
          Timed out junit tests org.apache.hadoop.yarn.server.router.webapp.TestRouterWebServicesREST



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-5412
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876907/YARN-5412-YARN-2915.2.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 4f38b3f2d9d1 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-2915 / 590d959
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/whitespace-eol.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/whitespace-tabs.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16388/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16388/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 8 new or modified test files.       YARN-2915 Compile Tests 0 mvndep 0m 26s Maven dependency ordering for branch +1 mvninstall 16m 48s YARN-2915 passed +1 compile 9m 36s YARN-2915 passed +1 checkstyle 0m 57s YARN-2915 passed +1 mvnsite 2m 14s YARN-2915 passed +1 findbugs 3m 44s YARN-2915 passed +1 javadoc 1m 47s YARN-2915 passed       Patch Compile Tests 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 1m 44s the patch passed +1 compile 5m 39s the patch passed +1 javac 5m 39s the patch passed -0 checkstyle 1m 1s hadoop-yarn-project/hadoop-yarn: The patch generated 25 new + 223 unchanged - 1 fixed = 248 total (was 224) +1 mvnsite 2m 19s the patch passed -1 whitespace 0m 0s The patch has 4 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply -1 whitespace 0m 0s The patch 15 line(s) with tabs. +1 xml 0m 4s The patch has no ill-formed XML file. +1 findbugs 4m 16s the patch passed -1 javadoc 0m 20s hadoop-yarn-server-router in the patch failed.       Other Tests +1 unit 0m 32s hadoop-yarn-api in the patch passed. +1 unit 2m 32s hadoop-yarn-common in the patch passed. -1 unit 40m 57s hadoop-yarn-server-resourcemanager in the patch failed. -1 unit 15m 31s hadoop-yarn-server-router in the patch failed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 121m 26s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesHttpStaticUserPermissions   hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.webapp.TestRMWebappAuthentication   hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesDelegationTokenAuthentication   hadoop.yarn.server.router.webapp.TestRouterWebServices   hadoop.yarn.server.router.rmadmin.TestRouterRMAdminService   hadoop.yarn.server.router.clientrm.TestRouterClientRMService Timed out junit tests org.apache.hadoop.yarn.server.router.webapp.TestRouterWebServicesREST Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-5412 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12876907/YARN-5412-YARN-2915.2.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 4f38b3f2d9d1 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-2915 / 590d959 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/whitespace-eol.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/whitespace-tabs.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16388/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16388/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/16388/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          curino Carlo Curino added a comment -

          Giovanni Matteo Fumarola, per our offline discussion:
          You can probably fix the tests issues by:

          1. add a capacity-scheduler.xml to test/resources,
          2. fix the yarn-default.xml (mistakenly setting rm.webaddress instead of router.webaddress),
          3. add timeouts to the tests, so it timeouts locally and invokes shutdown, avoiding other tests to fail due to binding issues (more local errors help)

          Also completing a first pass on the review:

          1. TestRouterWebServices remove LOG.info that are redundant to asserts
          2. TestRouterWebServicesREST factor out code in setup to a isWebAppRunning generic method (with a path in input)
          3. TestRouterWebServicesREST factor out all the invocations to something like private List<T> performTest(Path, T.class), the return list allows you to add specific size checks outside where appropriate.
          Show
          curino Carlo Curino added a comment - Giovanni Matteo Fumarola , per our offline discussion: You can probably fix the tests issues by: add a capacity-scheduler.xml to test/resources, fix the yarn-default.xml (mistakenly setting rm.webaddress instead of router.webaddress), add timeouts to the tests, so it timeouts locally and invokes shutdown, avoiding other tests to fail due to binding issues (more local errors help) Also completing a first pass on the review: TestRouterWebServices remove LOG.info that are redundant to asserts TestRouterWebServicesREST factor out code in setup to a isWebAppRunning generic method (with a path in input) TestRouterWebServicesREST factor out all the invocations to something like private List<T> performTest(Path, T.class) , the return list allows you to add specific size checks outside where appropriate.
          Hide
          giovanni.fumarola Giovanni Matteo Fumarola added a comment -

          Carlo Curino thanks for your feedback.
          v3 addresses all of these.

          Show
          giovanni.fumarola Giovanni Matteo Fumarola added a comment - Carlo Curino thanks for your feedback. v3 addresses all of these.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 22s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 9 new or modified test files.
                YARN-2915 Compile Tests
          0 mvndep 0m 23s Maven dependency ordering for branch
          +1 mvninstall 17m 21s YARN-2915 passed
          +1 compile 10m 40s YARN-2915 passed
          +1 checkstyle 1m 2s YARN-2915 passed
          +1 mvnsite 2m 26s YARN-2915 passed
          +1 findbugs 4m 0s YARN-2915 passed
          +1 javadoc 1m 49s YARN-2915 passed
                Patch Compile Tests
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 53s the patch passed
          +1 compile 6m 33s the patch passed
          +1 javac 6m 33s the patch passed
          -0 checkstyle 0m 54s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 217 unchanged - 0 fixed = 221 total (was 217)
          +1 mvnsite 2m 19s the patch passed
          -1 whitespace 0m 0s The patch has 11 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          -1 whitespace 0m 1s The patch 15 line(s) with tabs.
          +1 xml 0m 5s The patch has no ill-formed XML file.
          +1 findbugs 4m 34s the patch passed
          -1 javadoc 0m 19s hadoop-yarn-server-router in the patch failed.
                Other Tests
          -1 unit 0m 35s hadoop-yarn-api in the patch failed.
          +1 unit 2m 28s hadoop-yarn-common in the patch passed.
          -1 unit 43m 39s hadoop-yarn-server-resourcemanager in the patch failed.
          -1 unit 0m 59s hadoop-yarn-server-router in the patch failed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          113m 9s



          Reason Tests
          Failed junit tests hadoop.yarn.conf.TestYarnConfigurationFields
            hadoop.yarn.server.resourcemanager.TestRMRestart
            hadoop.yarn.server.resourcemanager.scheduler.fair.TestFSAppStarvation
            hadoop.yarn.server.router.webapp.TestRouterWebServicesREST
          Timed out junit tests org.apache.hadoop.yarn.server.resourcemanager.TestSubmitApplicationWithRMHA



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-5412
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12877157/YARN-5412-YARN-2915.3.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 82cb757b6203 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-2915 / 590d959
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/whitespace-eol.txt
          whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/whitespace-tabs.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16429/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16429/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 22s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 9 new or modified test files.       YARN-2915 Compile Tests 0 mvndep 0m 23s Maven dependency ordering for branch +1 mvninstall 17m 21s YARN-2915 passed +1 compile 10m 40s YARN-2915 passed +1 checkstyle 1m 2s YARN-2915 passed +1 mvnsite 2m 26s YARN-2915 passed +1 findbugs 4m 0s YARN-2915 passed +1 javadoc 1m 49s YARN-2915 passed       Patch Compile Tests 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 53s the patch passed +1 compile 6m 33s the patch passed +1 javac 6m 33s the patch passed -0 checkstyle 0m 54s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 217 unchanged - 0 fixed = 221 total (was 217) +1 mvnsite 2m 19s the patch passed -1 whitespace 0m 0s The patch has 11 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply -1 whitespace 0m 1s The patch 15 line(s) with tabs. +1 xml 0m 5s The patch has no ill-formed XML file. +1 findbugs 4m 34s the patch passed -1 javadoc 0m 19s hadoop-yarn-server-router in the patch failed.       Other Tests -1 unit 0m 35s hadoop-yarn-api in the patch failed. +1 unit 2m 28s hadoop-yarn-common in the patch passed. -1 unit 43m 39s hadoop-yarn-server-resourcemanager in the patch failed. -1 unit 0m 59s hadoop-yarn-server-router in the patch failed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 113m 9s Reason Tests Failed junit tests hadoop.yarn.conf.TestYarnConfigurationFields   hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.scheduler.fair.TestFSAppStarvation   hadoop.yarn.server.router.webapp.TestRouterWebServicesREST Timed out junit tests org.apache.hadoop.yarn.server.resourcemanager.TestSubmitApplicationWithRMHA Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-5412 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12877157/YARN-5412-YARN-2915.3.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 82cb757b6203 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-2915 / 590d959 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/whitespace-eol.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/whitespace-tabs.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16429/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/16429/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/16429/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          giovanni.fumarola Giovanni Matteo Fumarola added a comment -

          By introducing capacity-scheduler.xml Jerkins is able to execute the new test class TestRouterWebServicesREST.
          As soon as YARN-6792 is checked in, I will submit a new patch that will address all the remaining Yetus warnings and Carlo's feedback.

          Show
          giovanni.fumarola Giovanni Matteo Fumarola added a comment - By introducing capacity-scheduler.xml Jerkins is able to execute the new test class TestRouterWebServicesREST . As soon as YARN-6792 is checked in, I will submit a new patch that will address all the remaining Yetus warnings and Carlo's feedback.
          Hide
          giovanni.fumarola Giovanni Matteo Fumarola added a comment -

          Attached v4. It addresses the remaining Yetus warnings and it is a full rebase after YARN-6792 and YARN-6280.

          Show
          giovanni.fumarola Giovanni Matteo Fumarola added a comment - Attached v4. It addresses the remaining Yetus warnings and it is a full rebase after YARN-6792 and YARN-6280 .
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 9 new or modified test files.
                YARN-2915 Compile Tests
          0 mvndep 0m 28s Maven dependency ordering for branch
          +1 mvninstall 13m 28s YARN-2915 passed
          +1 compile 8m 44s YARN-2915 passed
          +1 checkstyle 0m 58s YARN-2915 passed
          +1 mvnsite 2m 25s YARN-2915 passed
          -1 findbugs 0m 24s hadoop-yarn-server-router in YARN-2915 failed.
          +1 javadoc 1m 54s YARN-2915 passed
                Patch Compile Tests
          0 mvndep 0m 11s Maven dependency ordering for patch
          +1 mvninstall 1m 38s the patch passed
          +1 compile 5m 20s the patch passed
          +1 javac 5m 20s the patch passed
          -0 checkstyle 0m 54s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 231 unchanged - 0 fixed = 235 total (was 231)
          +1 mvnsite 2m 17s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 5s The patch has no ill-formed XML file.
          -1 findbugs 0m 23s hadoop-yarn-server-router in the patch failed.
          -1 javadoc 0m 21s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router generated 17 new + 0 unchanged - 0 fixed = 17 total (was 0)
                Other Tests
          +1 unit 0m 33s hadoop-yarn-api in the patch passed.
          +1 unit 2m 34s hadoop-yarn-common in the patch passed.
          -1 unit 45m 8s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 unit 1m 4s hadoop-yarn-server-router in the patch passed.
          +1 asflicense 0m 35s The patch does not generate ASF License warnings.
          106m 34s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart
            hadoop.yarn.server.resourcemanager.metrics.TestSystemMetricsPublisher
            hadoop.yarn.server.resourcemanager.federation.TestFederationRMStateStoreService



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-5412
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12877418/YARN-5412-YARN-2915.4.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux f82610737726 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-2915 / 4e890f2
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/patch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/16453/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/16453/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/16453/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 9 new or modified test files.       YARN-2915 Compile Tests 0 mvndep 0m 28s Maven dependency ordering for branch +1 mvninstall 13m 28s YARN-2915 passed +1 compile 8m 44s YARN-2915 passed +1 checkstyle 0m 58s YARN-2915 passed +1 mvnsite 2m 25s YARN-2915 passed -1 findbugs 0m 24s hadoop-yarn-server-router in YARN-2915 failed. +1 javadoc 1m 54s YARN-2915 passed       Patch Compile Tests 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 1m 38s the patch passed +1 compile 5m 20s the patch passed +1 javac 5m 20s the patch passed -0 checkstyle 0m 54s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 231 unchanged - 0 fixed = 235 total (was 231) +1 mvnsite 2m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 5s The patch has no ill-formed XML file. -1 findbugs 0m 23s hadoop-yarn-server-router in the patch failed. -1 javadoc 0m 21s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router generated 17 new + 0 unchanged - 0 fixed = 17 total (was 0)       Other Tests +1 unit 0m 33s hadoop-yarn-api in the patch passed. +1 unit 2m 34s hadoop-yarn-common in the patch passed. -1 unit 45m 8s hadoop-yarn-server-resourcemanager in the patch failed. +1 unit 1m 4s hadoop-yarn-server-router in the patch passed. +1 asflicense 0m 35s The patch does not generate ASF License warnings. 106m 34s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.TestRMRestart   hadoop.yarn.server.resourcemanager.metrics.TestSystemMetricsPublisher   hadoop.yarn.server.resourcemanager.federation.TestFederationRMStateStoreService Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-5412 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12877418/YARN-5412-YARN-2915.4.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux f82610737726 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-2915 / 4e890f2 Default Java 1.8.0_131 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/patch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/16453/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/16453/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/16453/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/16453/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          curino Carlo Curino added a comment -

          Giovanni Matteo Fumarola, thanks for addressing the feedback. Here some more, based on your latest patch.

          1. hadoop-yarn-server-router/pom.xml can you validate the list you add is minimal and as narrowly scoped as possible?
          2. I think that in DefaultRequestInterceptorREST the implementations of getNodes, getLabelsToNodes, replaceLabelsOnNode don't forward all the inputs correctly
          3. Related to the above, your tests should be tighter and attempt to catch these things
          4. Add comments for the one not-needed as part of HSR
          5. Why the DefaultRequestInterceptorREST does not implement getAppAttempt, getContainers, getContainere?
          6. RouterWebApp what happens if the router is not initialized?
          7. RouterWebServiceUtil (89 shouldn't you throw an exception in this case?
          8. RouterWebServiceUtil (105 is 200 the only "correct" code ever returned? (What about 201/202/204...) Maybe just check is in 2XX series.
          9. RouterWebServices.init() the init name is a bit misleading, as it looks more like a "clearResponse". Also is it enough to set content type to null, to clear?
          10. TestRouterWebServicesREST seem to still have a large amount of redundancy in the code, the same "generics" tricks you played in DefaultRequestInterceptorREST should apply here as well.
          11. TestRouterWebServicesREST seem to mostly/only? test correct paths. It would be good to verify also wrong/null inputs and wrong-paths, validating the system catches and throws correclty in those cases.
          12. TestRouterWebServicesREST it should be easy to make this test parametric, and have it run with both .jsom an .xml formats (broaden the coverage with little work).
          1. Please comment on / address the Yetus issues.
          Show
          curino Carlo Curino added a comment - Giovanni Matteo Fumarola , thanks for addressing the feedback. Here some more, based on your latest patch. hadoop-yarn-server-router/pom.xml can you validate the list you add is minimal and as narrowly scoped as possible? I think that in DefaultRequestInterceptorREST the implementations of getNodes , getLabelsToNodes , replaceLabelsOnNode don't forward all the inputs correctly Related to the above, your tests should be tighter and attempt to catch these things Add comments for the one not-needed as part of HSR Why the DefaultRequestInterceptorREST does not implement getAppAttempt, getContainers, getContainere ? RouterWebApp what happens if the router is not initialized? RouterWebServiceUtil (89 shouldn't you throw an exception in this case? RouterWebServiceUtil (105 is 200 the only "correct" code ever returned? (What about 201/202/204...) Maybe just check is in 2XX series. RouterWebServices.init() the init name is a bit misleading, as it looks more like a "clearResponse". Also is it enough to set content type to null, to clear? TestRouterWebServicesREST seem to still have a large amount of redundancy in the code, the same "generics" tricks you played in DefaultRequestInterceptorREST should apply here as well. TestRouterWebServicesREST seem to mostly/only? test correct paths. It would be good to verify also wrong/null inputs and wrong-paths, validating the system catches and throws correclty in those cases. TestRouterWebServicesREST it should be easy to make this test parametric, and have it run with both .jsom an .xml formats (broaden the coverage with little work). Please comment on / address the Yetus issues.

            People

            • Assignee:
              giovanni.fumarola Giovanni Matteo Fumarola
              Reporter:
              subru Subru Krishnan
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development