Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0
    • Component/s: None
    • Labels:
    • Release Note:
      Hide
      MapReduce support for the YARN shared cache allows MapReduce jobs to take advantage of additional resource caching. This saves network bandwidth between the job submission client as well as within the YARN cluster itself. This will reduce job submission time and overall job runtime.
      Show
      MapReduce support for the YARN shared cache allows MapReduce jobs to take advantage of additional resource caching. This saves network bandwidth between the job submission client as well as within the YARN cluster itself. This will reduce job submission time and overall job runtime.

      Description

      Implement the necessary changes so that the MapReduce application can leverage the new YARN shared cache (i.e. YARN-1492).

      Specifically, allow per-job configuration so that MapReduce jobs can specify which set of resources they would like to cache (i.e. jobjar, libjars, archives, files).

      1. MAPREDUCE-5951-trunk-v1.patch
        81 kB
        Chris Trezzo
      2. MAPREDUCE-5951-trunk-v2.patch
        84 kB
        Chris Trezzo
      3. MAPREDUCE-5951-trunk-v3.patch
        89 kB
        Chris Trezzo
      4. MAPREDUCE-5951-trunk-v4.patch
        89 kB
        Chris Trezzo
      5. MAPREDUCE-5951-trunk-v5.patch
        90 kB
        Chris Trezzo
      6. MAPREDUCE-5951-trunk-v6.patch
        103 kB
        Chris Trezzo
      7. MAPREDUCE-5951-trunk-v7.patch
        83 kB
        Chris Trezzo
      8. MAPREDUCE-5951-trunk-v8.patch
        86 kB
        Chris Trezzo
      9. MAPREDUCE-5951-trunk-v9.patch
        86 kB
        Chris Trezzo
      10. MAPREDUCE-5951-trunk-v10.patch
        83 kB
        Chris Trezzo
      11. MAPREDUCE-5951-trunk-v11.patch
        74 kB
        Chris Trezzo
      12. MAPREDUCE-5951-trunk-v12.patch
        72 kB
        Chris Trezzo
      13. MAPREDUCE-5951-trunk-v13.patch
        72 kB
        Chris Trezzo
      14. MAPREDUCE-5951-trunk-v14.patch
        75 kB
        Chris Trezzo
      15. MAPREDUCE-5951-trunk-v15.patch
        75 kB
        Chris Trezzo
      16. MAPREDUCE-5951-trunk.016.patch
        101 kB
        Chris Trezzo
      17. MAPREDUCE-5951-trunk.017.patch
        76 kB
        Chris Trezzo
      18. MAPREDUCE-5951-trunk.018.patch
        76 kB
        Chris Trezzo
      19. MAPREDUCE-5951-Overview.001.pdf
        68 kB
        Chris Trezzo
      20. MAPREDUCE-5951-trunk.019.patch
        78 kB
        Chris Trezzo
      21. MAPREDUCE-5951-trunk-020.patch
        90 kB
        Chris Trezzo
      22. MAPREDUCE-5951-trunk-021.patch
        109 kB
        Chris Trezzo

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #13078 (See https://builds.apache.org/job/Hadoop-trunk-Commit/13078/)
          MAPREDUCE-5951. Add support for the YARN Shared Cache. (ctrezzo: rev e46d5bb962b0c942f993afc505b165b1cd96e51b)

          • (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJobResourceUploaderWithSharedCache.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/test/java/org/apache/hadoop/mapred/TestLocalDistributedCacheManager.java
          • (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/site/markdown/SharedCacheSupport.md
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/JobResourceUploader.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TaskAttemptImpl.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/pom.xml
          • (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/LocalResourceBuilder.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJobResourceUploader.java
          • (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/SharedCacheConfig.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/JobImpl.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java
          • (edit) hadoop-project/src/site/site.xml
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/test/java/org/apache/hadoop/mapreduce/v2/util/TestMRApps.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/main/java/org/apache/hadoop/mapred/YARNRunner.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/Job.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestLocalJobSubmission.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRJobConfig.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #13078 (See https://builds.apache.org/job/Hadoop-trunk-Commit/13078/ ) MAPREDUCE-5951 . Add support for the YARN Shared Cache. (ctrezzo: rev e46d5bb962b0c942f993afc505b165b1cd96e51b) (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJobResourceUploaderWithSharedCache.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/test/java/org/apache/hadoop/mapred/TestLocalDistributedCacheManager.java (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/site/markdown/SharedCacheSupport.md (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/JobResourceUploader.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TaskAttemptImpl.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/pom.xml (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/LocalResourceBuilder.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJobResourceUploader.java (add) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/SharedCacheConfig.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/JobImpl.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/v2/TestMRJobs.java (edit) hadoop-project/src/site/site.xml (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/test/java/org/apache/hadoop/mapreduce/v2/util/TestMRApps.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/main/java/org/apache/hadoop/mapred/YARNRunner.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/Job.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestLocalJobSubmission.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRJobConfig.java
          Hide
          ctrezzo Chris Trezzo added a comment -

          Committed to trunk, branch-3.0 and branch-2. Thanks for all the help with reviews Ming Ma, Sangjin Lee, and Karthik Kambatla!

          Show
          ctrezzo Chris Trezzo added a comment - Committed to trunk, branch-3.0 and branch-2. Thanks for all the help with reviews Ming Ma , Sangjin Lee , and Karthik Kambatla !
          Hide
          sjlee0 Sangjin Lee added a comment -

          +1. Thanks for the great work and taking it to completion Chris Trezzo!

          Show
          sjlee0 Sangjin Lee added a comment - +1. Thanks for the great work and taking it to completion Chris Trezzo !
          Hide
          ctrezzo Chris Trezzo added a comment -

          Thank you Ming Ma for the review! I will wait until Thursday to commit in case there are any other comments. Otherwise, I plan to commit to trunk, branch-3.0 and branch-2.

          Show
          ctrezzo Chris Trezzo added a comment - Thank you Ming Ma for the review! I will wait until Thursday to commit in case there are any other comments. Otherwise, I plan to commit to trunk, branch-3.0 and branch-2.
          Hide
          mingma Ming Ma added a comment -

          +1.

          Show
          mingma Ming Ma added a comment - +1.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Thanks for the comment Ming Ma!

          Should any code be moved from MR to YARN to make it easier for other YARN applications to use shared cache? For example, maybe other applications can benefit from part of LocalResourceBuilder or the special care when dealing with fragment.

          I have thought about this a fair amount. Originally we started pushing more of the fragment code down into the YARN layer (see YARN-3637), but later I realized that the code dealing with fragments is purely at the MapReduce layer. YARN's api does not use fragments. Instead the ContainerLaunchContext expects a Map<String, LocalResource> localResources, where the strings are the destination file names (i.e. symlinks). We wound up pulling the fragment portion back out of YARN (see YARN-7250) because it was not consistent with the rest of the YARN api. Additionally, I think that the way MapReduce uses fragments right now is very brittle and prone to bugs. Within MapReduce, resources with fragments are converted between paths, URIs and URLs multiple times throughout the code and each of these three classes supports fragments in different ways. If you are not very careful, one could easily drop a fragment.

          I also thought about moving LocalResourceBuilder to YARN, but it has a fair amount of MapReduce specific things that would need to change. For example:

          1. All of the parameters are array based due to how MapReduce currently handles resources. We could change this, but then that would need additional refactoring at the MapReduce level.
          2. Components from the MapReduce wildcard feature are in this class. We would need to figure out if that makes sense at the yarn layer.
          3. LocalResourceBuilder currently handles fragments, which we would also need to figure out if it makes sense at the yarn layer.

          At the end of the day, it would not be simply dropping the LocalResourceBuilder into YARN and being done. We would have to think about it more. It does seem like something YARN could benefit from, along with a resource uploader. I can file another jira to cover these topics, but I think it is probably out of scope for this jira.

          I think in reality the complexity in this jira is due to the way MapReduce itself handles resources and the above mentioned issues with fragments. If we wanted to implement a generic yarn resource uploader, I think it could be much simpler. For example, this is a slightly simplified version of the code devoted to using something in the shared cache:

          String localPathChecksum = sharedCacheClient.getFileChecksum(localPath);
          URL cachedResource = sharedCacheClient.use(appId, localPathChecksum);
          LocalResource resource = LocalResource.newInstance(cachedResource,
                LocalResourceType.FILE, LocalResourceVisibility.PUBLIC
                size, timestamp, null, true);
          

          That LocalResource can then be passed directly to the ContainerLaunchContext where a symlink can be specified as a String. As you can see, there is no innate need for fragments at the YARN layer.

          Please let me know if that makes sense or if I have missed something! Thanks.

          Show
          ctrezzo Chris Trezzo added a comment - Thanks for the comment Ming Ma ! Should any code be moved from MR to YARN to make it easier for other YARN applications to use shared cache? For example, maybe other applications can benefit from part of LocalResourceBuilder or the special care when dealing with fragment. I have thought about this a fair amount. Originally we started pushing more of the fragment code down into the YARN layer (see YARN-3637 ), but later I realized that the code dealing with fragments is purely at the MapReduce layer. YARN's api does not use fragments. Instead the ContainerLaunchContext expects a Map<String, LocalResource> localResources, where the strings are the destination file names (i.e. symlinks). We wound up pulling the fragment portion back out of YARN (see YARN-7250 ) because it was not consistent with the rest of the YARN api. Additionally, I think that the way MapReduce uses fragments right now is very brittle and prone to bugs. Within MapReduce, resources with fragments are converted between paths, URIs and URLs multiple times throughout the code and each of these three classes supports fragments in different ways. If you are not very careful, one could easily drop a fragment. I also thought about moving LocalResourceBuilder to YARN, but it has a fair amount of MapReduce specific things that would need to change. For example: All of the parameters are array based due to how MapReduce currently handles resources. We could change this, but then that would need additional refactoring at the MapReduce level. Components from the MapReduce wildcard feature are in this class. We would need to figure out if that makes sense at the yarn layer. LocalResourceBuilder currently handles fragments, which we would also need to figure out if it makes sense at the yarn layer. At the end of the day, it would not be simply dropping the LocalResourceBuilder into YARN and being done. We would have to think about it more. It does seem like something YARN could benefit from, along with a resource uploader. I can file another jira to cover these topics, but I think it is probably out of scope for this jira. I think in reality the complexity in this jira is due to the way MapReduce itself handles resources and the above mentioned issues with fragments. If we wanted to implement a generic yarn resource uploader, I think it could be much simpler. For example, this is a slightly simplified version of the code devoted to using something in the shared cache: String localPathChecksum = sharedCacheClient.getFileChecksum(localPath); URL cachedResource = sharedCacheClient.use(appId, localPathChecksum); LocalResource resource = LocalResource.newInstance(cachedResource, LocalResourceType.FILE, LocalResourceVisibility.PUBLIC size, timestamp, null, true); That LocalResource can then be passed directly to the ContainerLaunchContext where a symlink can be specified as a String. As you can see, there is no innate need for fragments at the YARN layer. Please let me know if that makes sense or if I have missed something! Thanks.
          Hide
          mingma Ming Ma added a comment -

          Thanks Chris Trezzo. The code looks good overall. The only question I have at this point is if any code should be moved from MR to YARN to make it easier for other YARN applications to use shared cache. For example, maybe other applications can benefit from part of LocalResourceBuilder or the special care when dealing with fragment.

          Show
          mingma Ming Ma added a comment - Thanks Chris Trezzo . The code looks good overall. The only question I have at this point is if any code should be moved from MR to YARN to make it easier for other YARN applications to use shared cache. For example, maybe other applications can benefit from part of LocalResourceBuilder or the special care when dealing with fragment.
          Hide
          andrew.wang Andrew Wang added a comment -

          If it's going into 2.9.0, I think it's safe for 3.0.0 too. Please include it in branch-3.0 as well, thanks!

          Show
          andrew.wang Andrew Wang added a comment - If it's going into 2.9.0, I think it's safe for 3.0.0 too. Please include it in branch-3.0 as well, thanks!
          Hide
          ctrezzo Chris Trezzo added a comment -

          Hi Sangjin Lee, Karthik Kambatla, Ming Ma, Jason Lowe, and Vrushali C!

          I have made another push to get MapReduce support for the shared cache committed. I have rebased the patch, added documentation and fixed all warnings/issues that I see so far. At this point, I need a reviewer for the final review. I know this patch is a big one, so if there is anything I can do to help with the review process to make it easier to review, or if there is someone else who might be interested in the review, please let me know. Some good news:

          1. Much of the patch has already been reviewed by Karthik Kambatla Sangjin Lee and Ming Ma during previous iterations.
          2. I have ensured that the entire feature is behind a switch. As such, when disabled (default) there are no effects for the user.
          3. I have functionally tested this patch on a pseudo distributed cluster.
          4. I have deployed this patch to a larger test cluster and ran jobs with the patch.
          5. There is very similar code running in production that has been working for years at this point.

          My main goal is to commit this to trunk and branch-2 (2.9.0). If it can make it into branch-3.0 for GA that would be great as well, but I understand that the beta is already out (Andrew Wang please let me know what you think). Once I get a +1 on the patch, I would be happy to do the work to commit.

          Thanks in advance for the help and effort. I really do appreciate it!

          Show
          ctrezzo Chris Trezzo added a comment - Hi Sangjin Lee , Karthik Kambatla , Ming Ma , Jason Lowe , and Vrushali C ! I have made another push to get MapReduce support for the shared cache committed. I have rebased the patch, added documentation and fixed all warnings/issues that I see so far. At this point, I need a reviewer for the final review. I know this patch is a big one, so if there is anything I can do to help with the review process to make it easier to review, or if there is someone else who might be interested in the review, please let me know. Some good news: Much of the patch has already been reviewed by Karthik Kambatla Sangjin Lee and Ming Ma during previous iterations. I have ensured that the entire feature is behind a switch. As such, when disabled (default) there are no effects for the user. I have functionally tested this patch on a pseudo distributed cluster. I have deployed this patch to a larger test cluster and ran jobs with the patch. There is very similar code running in production that has been working for years at this point. My main goal is to commit this to trunk and branch-2 (2.9.0). If it can make it into branch-3.0 for GA that would be great as well, but I understand that the beta is already out ( Andrew Wang please let me know what you think). Once I get a +1 on the patch, I would be happy to do the work to commit. Thanks in advance for the help and effort. I really do appreciate it!
          Hide
          ctrezzo Chris Trezzo added a comment - - edited

          This is the javac warning:

          [WARNING] /testptch/hadoop/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/LocalResourceBuilder.java:[34,44] [deprecation] DistributedCache in org.apache.hadoop.mapreduce.filecache has been deprecated

          LocalResourceBuilder was a class added to fix a checkstyle warning. I have used @SuppressWarnings("deprecation") to silence the warnings around DistributedCache usage at the class level. This warning is complaining about the import statement. If anyone has an idea for how to apply the annotation to the import statement, please let me know. Furthermore, the LocalResourceBuilder is simply refactoring the MRApps#parseDistributedCacheArtifacts method, so I do not think it makes sense to fix the usage of a deprecated interface in this patch, especially since it is used in a lot of places.

          Show
          ctrezzo Chris Trezzo added a comment - - edited This is the javac warning: [WARNING] /testptch/hadoop/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/LocalResourceBuilder.java: [34,44] [deprecation] DistributedCache in org.apache.hadoop.mapreduce.filecache has been deprecated LocalResourceBuilder was a class added to fix a checkstyle warning. I have used @SuppressWarnings("deprecation") to silence the warnings around DistributedCache usage at the class level. This warning is complaining about the import statement. If anyone has an idea for how to apply the annotation to the import statement, please let me know. Furthermore, the LocalResourceBuilder is simply refactoring the MRApps#parseDistributedCacheArtifacts method, so I do not think it makes sense to fix the usage of a deprecated interface in this patch, especially since it is used in a lot of places.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s 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 6 new or modified test files.
                trunk Compile Tests
          0 mvndep 1m 26s Maven dependency ordering for branch
          +1 mvninstall 12m 46s trunk passed
          +1 compile 14m 9s trunk passed
          +1 checkstyle 2m 1s trunk passed
          +1 mvnsite 2m 17s trunk passed
          0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project
          +1 findbugs 2m 31s trunk passed
          +1 javadoc 1m 27s trunk passed
                Patch Compile Tests
          0 mvndep 0m 16s Maven dependency ordering for patch
          +1 mvninstall 1m 32s the patch passed
          +1 compile 10m 43s the patch passed
          -1 javac 10m 43s root generated 1 new + 1255 unchanged - 16 fixed = 1256 total (was 1271)
          +1 checkstyle 2m 1s root: The patch generated 0 new + 1058 unchanged - 15 fixed = 1058 total (was 1073)
          +1 mvnsite 2m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 4s The patch has no ill-formed XML file.
          0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project
          +1 findbugs 3m 6s the patch passed
          +1 javadoc 1m 35s the patch passed
                Other Tests
          +1 unit 0m 11s hadoop-project in the patch passed.
          +1 unit 2m 55s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 0m 48s hadoop-mapreduce-client-common in the patch passed.
          +1 unit 9m 3s hadoop-mapreduce-client-app in the patch passed.
          +1 unit 109m 11s hadoop-mapreduce-client-jobclient in the patch passed.
          +1 asflicense 0m 41s The patch does not generate ASF License warnings.
          182m 35s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:71bbb86
          JIRA Issue MAPREDUCE-5951
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12890320/MAPREDUCE-5951-trunk-021.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 42144958904a 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 trunk / 2df1b2a
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          javac https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7172/artifact/patchprocess/diff-compile-javac-root.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7172/testReport/
          modules C: hadoop-project hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: .
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7172/console
          Powered by Apache Yetus 0.5.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 6 new or modified test files.       trunk Compile Tests 0 mvndep 1m 26s Maven dependency ordering for branch +1 mvninstall 12m 46s trunk passed +1 compile 14m 9s trunk passed +1 checkstyle 2m 1s trunk passed +1 mvnsite 2m 17s trunk passed 0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project +1 findbugs 2m 31s trunk passed +1 javadoc 1m 27s trunk passed       Patch Compile Tests 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvninstall 1m 32s the patch passed +1 compile 10m 43s the patch passed -1 javac 10m 43s root generated 1 new + 1255 unchanged - 16 fixed = 1256 total (was 1271) +1 checkstyle 2m 1s root: The patch generated 0 new + 1058 unchanged - 15 fixed = 1058 total (was 1073) +1 mvnsite 2m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 4s The patch has no ill-formed XML file. 0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project +1 findbugs 3m 6s the patch passed +1 javadoc 1m 35s the patch passed       Other Tests +1 unit 0m 11s hadoop-project in the patch passed. +1 unit 2m 55s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 48s hadoop-mapreduce-client-common in the patch passed. +1 unit 9m 3s hadoop-mapreduce-client-app in the patch passed. +1 unit 109m 11s hadoop-mapreduce-client-jobclient in the patch passed. +1 asflicense 0m 41s The patch does not generate ASF License warnings. 182m 35s Subsystem Report/Notes Docker Image:yetus/hadoop:71bbb86 JIRA Issue MAPREDUCE-5951 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12890320/MAPREDUCE-5951-trunk-021.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 42144958904a 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 trunk / 2df1b2a Default Java 1.8.0_144 findbugs v3.1.0-RC1 javac https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7172/artifact/patchprocess/diff-compile-javac-root.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7172/testReport/ modules C: hadoop-project hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: . Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7172/console Powered by Apache Yetus 0.5.0 http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 25s 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 6 new or modified test files.
                trunk Compile Tests
          0 mvndep 1m 58s Maven dependency ordering for branch
          +1 mvninstall 19m 46s trunk passed
          +1 compile 22m 15s trunk passed
          +1 checkstyle 2m 48s trunk passed
          +1 mvnsite 3m 3s trunk passed
          0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project
          +1 findbugs 3m 5s trunk passed
          +1 javadoc 2m 6s trunk passed
                Patch Compile Tests
          0 mvndep 0m 16s Maven dependency ordering for patch
          +1 mvninstall 2m 5s the patch passed
          +1 compile 13m 14s the patch passed
          -1 javac 13m 14s root generated 1 new + 1255 unchanged - 16 fixed = 1256 total (was 1271)
          +1 checkstyle 2m 16s root: The patch generated 0 new + 1058 unchanged - 15 fixed = 1058 total (was 1073)
          +1 mvnsite 2m 38s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project
          +1 findbugs 3m 37s the patch passed
          +1 javadoc 1m 57s the patch passed
                Other Tests
          +1 unit 0m 16s hadoop-project in the patch passed.
          +1 unit 3m 2s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 0m 53s hadoop-mapreduce-client-common in the patch passed.
          +1 unit 9m 5s hadoop-mapreduce-client-app in the patch passed.
          +1 unit 114m 18s hadoop-mapreduce-client-jobclient in the patch passed.
          +1 asflicense 1m 2s The patch does not generate ASF License warnings.
          211m 44s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:71bbb86
          JIRA Issue MAPREDUCE-5951
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12890320/MAPREDUCE-5951-trunk-021.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 11a2b56aaa23 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / acf5b88
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          javac https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7170/artifact/patchprocess/diff-compile-javac-root.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7170/testReport/
          modules C: hadoop-project hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: .
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7170/console
          Powered by Apache Yetus 0.5.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 25s 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 6 new or modified test files.       trunk Compile Tests 0 mvndep 1m 58s Maven dependency ordering for branch +1 mvninstall 19m 46s trunk passed +1 compile 22m 15s trunk passed +1 checkstyle 2m 48s trunk passed +1 mvnsite 3m 3s trunk passed 0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project +1 findbugs 3m 5s trunk passed +1 javadoc 2m 6s trunk passed       Patch Compile Tests 0 mvndep 0m 16s Maven dependency ordering for patch +1 mvninstall 2m 5s the patch passed +1 compile 13m 14s the patch passed -1 javac 13m 14s root generated 1 new + 1255 unchanged - 16 fixed = 1256 total (was 1271) +1 checkstyle 2m 16s root: The patch generated 0 new + 1058 unchanged - 15 fixed = 1058 total (was 1073) +1 mvnsite 2m 38s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. 0 findbugs 0m 0s Skipped patched modules with no Java source: hadoop-project +1 findbugs 3m 37s the patch passed +1 javadoc 1m 57s the patch passed       Other Tests +1 unit 0m 16s hadoop-project in the patch passed. +1 unit 3m 2s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 53s hadoop-mapreduce-client-common in the patch passed. +1 unit 9m 5s hadoop-mapreduce-client-app in the patch passed. +1 unit 114m 18s hadoop-mapreduce-client-jobclient in the patch passed. +1 asflicense 1m 2s The patch does not generate ASF License warnings. 211m 44s Subsystem Report/Notes Docker Image:yetus/hadoop:71bbb86 JIRA Issue MAPREDUCE-5951 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12890320/MAPREDUCE-5951-trunk-021.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 11a2b56aaa23 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / acf5b88 Default Java 1.8.0_144 findbugs v3.1.0-RC1 javac https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7170/artifact/patchprocess/diff-compile-javac-root.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7170/testReport/ modules C: hadoop-project hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: . Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7170/console Powered by Apache Yetus 0.5.0 http://yetus.apache.org This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attached is trunk v21. This fixes checkstyle issues and adds documentation.

          Show
          ctrezzo Chris Trezzo added a comment - Attached is trunk v21. This fixes checkstyle issues and adds documentation.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 45s 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 6 new or modified test files.
                trunk Compile Tests
          0 mvndep 0m 54s Maven dependency ordering for branch
          +1 mvninstall 14m 25s trunk passed
          +1 compile 1m 46s trunk passed
          +1 checkstyle 0m 44s trunk passed
          +1 mvnsite 1m 46s trunk passed
          +1 findbugs 2m 44s trunk passed
          +1 javadoc 1m 17s trunk passed
                Patch Compile Tests
          0 mvndep 0m 8s Maven dependency ordering for patch
          +1 mvninstall 1m 34s the patch passed
          +1 compile 1m 41s the patch passed
          +1 javac 1m 41s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 354 unchanged - 4 fixed = 354 total (was 358)
          -1 checkstyle 0m 42s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 3 new + 1062 unchanged - 11 fixed = 1065 total (was 1073)
          +1 mvnsite 1m 43s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 3m 1s the patch passed
          +1 javadoc 1m 3s the patch passed
                Other Tests
          +1 unit 3m 0s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 0m 44s hadoop-mapreduce-client-common in the patch passed.
          +1 unit 9m 19s hadoop-mapreduce-client-app in the patch passed.
          -1 unit 114m 29s hadoop-mapreduce-client-jobclient in the patch failed.
          +1 asflicense 0m 28s The patch does not generate ASF License warnings.
          163m 25s



          Reason Tests
          Failed junit tests hadoop.mapred.TestLocalMRNotification



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:71bbb86
          JIRA Issue MAPREDUCE-5951
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12890076/MAPREDUCE-5951-trunk-020.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux f0ce73fc9e8f 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 015abcd
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt
          unit https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/artifact/patchprocess/patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/console
          Powered by Apache Yetus 0.5.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 45s 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 6 new or modified test files.       trunk Compile Tests 0 mvndep 0m 54s Maven dependency ordering for branch +1 mvninstall 14m 25s trunk passed +1 compile 1m 46s trunk passed +1 checkstyle 0m 44s trunk passed +1 mvnsite 1m 46s trunk passed +1 findbugs 2m 44s trunk passed +1 javadoc 1m 17s trunk passed       Patch Compile Tests 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 1m 34s the patch passed +1 compile 1m 41s the patch passed +1 javac 1m 41s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 354 unchanged - 4 fixed = 354 total (was 358) -1 checkstyle 0m 42s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 3 new + 1062 unchanged - 11 fixed = 1065 total (was 1073) +1 mvnsite 1m 43s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 3m 1s the patch passed +1 javadoc 1m 3s the patch passed       Other Tests +1 unit 3m 0s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 44s hadoop-mapreduce-client-common in the patch passed. +1 unit 9m 19s hadoop-mapreduce-client-app in the patch passed. -1 unit 114m 29s hadoop-mapreduce-client-jobclient in the patch failed. +1 asflicense 0m 28s The patch does not generate ASF License warnings. 163m 25s Reason Tests Failed junit tests hadoop.mapred.TestLocalMRNotification Subsystem Report/Notes Docker Image:yetus/hadoop:71bbb86 JIRA Issue MAPREDUCE-5951 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12890076/MAPREDUCE-5951-trunk-020.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux f0ce73fc9e8f 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 015abcd Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt unit https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/artifact/patchprocess/patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7169/console Powered by Apache Yetus 0.5.0 http://yetus.apache.org This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attached is a v20 trunk patch.

          Show
          ctrezzo Chris Trezzo added a comment - Attached is a v20 trunk patch.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Erik Krogen

          Is this so that the uploading to SCM can be done by the NM, which is a privileged user, to have more secure control over it?

          Yes exactly. We wanted to ensure that only trusted entities (i.e. the SCM and the node manager) were modifying the shared cached directories in HDFS. Additionally, we wanted to make sure that the checksum used when adding a resource to the cache was computed by a trusted entity as well.

          Show
          ctrezzo Chris Trezzo added a comment - Erik Krogen Is this so that the uploading to SCM can be done by the NM, which is a privileged user, to have more secure control over it? Yes exactly. We wanted to ensure that only trusted entities (i.e. the SCM and the node manager) were modifying the shared cached directories in HDFS. Additionally, we wanted to make sure that the checksum used when adding a resource to the cache was computed by a trusted entity as well.
          Hide
          xkrogen Erik Krogen added a comment -

          Ah, excellent point, Jason Lowe... I actually would love to hear the reasoning behind the current strategy of <client uploads resource to HDFS -> AM downloads resource -> AM uploads resource to SCM> rather than the seemingly more obvious/simpler <client uploads resource to SCM>. Is this so that the uploading to SCM can be done by the NM, which is a privileged user, to have more secure control over it?

          Chris Trezzo, first off thanks for getting back so quickly! And for the pointer to YARN-5727; that's an interesting issue. The public visibility solution is certainly simpler from the YARN side and seems pretty reasonable from a point of expectation of burden on an application ("you want a publicly shared resource? put it somewhere public"). It doesn't add too much complexity on the MR side, though having a separate staging directory just for public resources is a bit cumbersome. It also means that other application developers will have to build the same type of logic - in general I would lean towards more logic pushed into the YARN level so that it is easy for application devs to support. I don't have good insight into how difficult your initially proposed solution in YARN-5727 would be to implement, though.

          Show
          xkrogen Erik Krogen added a comment - Ah, excellent point, Jason Lowe ... I actually would love to hear the reasoning behind the current strategy of <client uploads resource to HDFS -> AM downloads resource -> AM uploads resource to SCM> rather than the seemingly more obvious/simpler <client uploads resource to SCM>. Is this so that the uploading to SCM can be done by the NM, which is a privileged user, to have more secure control over it? Chris Trezzo , first off thanks for getting back so quickly! And for the pointer to YARN-5727 ; that's an interesting issue. The public visibility solution is certainly simpler from the YARN side and seems pretty reasonable from a point of expectation of burden on an application ("you want a publicly shared resource? put it somewhere public"). It doesn't add too much complexity on the MR side, though having a separate staging directory just for public resources is a bit cumbersome. It also means that other application developers will have to build the same type of logic - in general I would lean towards more logic pushed into the YARN level so that it is easy for application devs to support. I don't have good insight into how difficult your initially proposed solution in YARN-5727 would be to implement, though.
          Hide
          jlowe Jason Lowe added a comment -

          I don't think it really matters whether the jar resource uploaded by the client is public or private. In both cases the HDFS path to which the client posts the resource will be removed when the job completes. If any subsequent jobs come along and figure out via the SCM that they can avoid uploading their own, redundant copy of the same resource then they will receive a resource path within the SCM area which is a different path than the one used by the first job. That means the resource is going to get downloaded to the node again because it's in a different location than the first job's resource.

          Even if the first job's client uploads the resource to a public directory, no other job is going to ask for that resource under the same path. It will be uploaded to a public staging directory which is specific to that app and whose path exists only as long as the app. The problem with having jobs try to share resources automatically just from the job client is knowing when the resource can be removed, otherwise we could yank it just as another app tries to localize it or never clean it up. That's why the SCM does the necessary ref counting to know what's being used and when resources can be freed safely. If we want to avoid the double-download of the resource then the job client will need to upload the resource to the SCM directly and then submit the job after it has received the public resource path from the SCM.

          Show
          jlowe Jason Lowe added a comment - I don't think it really matters whether the jar resource uploaded by the client is public or private. In both cases the HDFS path to which the client posts the resource will be removed when the job completes. If any subsequent jobs come along and figure out via the SCM that they can avoid uploading their own, redundant copy of the same resource then they will receive a resource path within the SCM area which is a different path than the one used by the first job. That means the resource is going to get downloaded to the node again because it's in a different location than the first job's resource. Even if the first job's client uploads the resource to a public directory, no other job is going to ask for that resource under the same path. It will be uploaded to a public staging directory which is specific to that app and whose path exists only as long as the app. The problem with having jobs try to share resources automatically just from the job client is knowing when the resource can be removed, otherwise we could yank it just as another app tries to localize it or never clean it up. That's why the SCM does the necessary ref counting to know what's being used and when resources can be freed safely. If we want to avoid the double-download of the resource then the job client will need to upload the resource to the SCM directly and then submit the job after it has received the public resource path from the SCM.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Thanks Erik Krogen for the comment!

          is this an oversight, or is this behavior desired?

          Originally we just left it private because we wanted to avoid having to change the staging directory and that portion of how MapReduce uploaded resources. As I am looking more at YARN-5727, I think it makes more sense to do this so that the resources are initially uploaded to a public place and explicitly set with a public visibility by the MapReduce client. I was thinking of potentially adding a public staging directory that is created and cleaned up by the MapReduce client along with the current staging directory. Erik Krogen would you have any thoughts on this? Jason Lowe would you have any thoughts on this as well?

          Show
          ctrezzo Chris Trezzo added a comment - Thanks Erik Krogen for the comment! is this an oversight, or is this behavior desired? Originally we just left it private because we wanted to avoid having to change the staging directory and that portion of how MapReduce uploaded resources. As I am looking more at YARN-5727 , I think it makes more sense to do this so that the resources are initially uploaded to a public place and explicitly set with a public visibility by the MapReduce client. I was thinking of potentially adding a public staging directory that is created and cleaned up by the MapReduce client along with the current staging directory. Erik Krogen would you have any thoughts on this? Jason Lowe would you have any thoughts on this as well?
          Hide
          xkrogen Erik Krogen added a comment - - edited

          Hey Chris Trezzo, I have a question about the behavior of this patch. Currently the old logic for resource visibility is used, so if a resource is world-readable, it will be marked as PUBLIC, else PRIVATE. Given my current understanding of this patch's behavior, I see the following scenario:

          • Client submits a job with libjar X, which has never been used before. Client contacts SCM to mark X as "used", SCM responds that it does not have X.
          • Client uploads X to staging directory, which I assume here is not world-readable. X is marked as PRIVATE.
          • MR-AM localizes X, then uploads it to the shared cache. Other NMs all localize X as PRIVATE and do not share it with other applications.
          • Client then submits the same job with the same X. Client contacts SCM, and SCM responds with a world-readable (755 dirs / 555 file) path inside of the shared cache.
          • Client does not upload X, and marks X as PUBLIC, since it is currently in a world-readable location.
          • MR-AM and NMs all localize X as PUBLIC and share it with other applications.

          Please correct me if I am wrong on any of these steps. It seems that it is the expected behavior that X is eventually PUBLIC, given that we asked for it to be uploaded to the publicly shared cache, but it seems unnecessary for it to be marked as PRIVATE the first time around. Do we do this just to avoid changing the existing logic for marking a resource as PRIVATE vs PUBLIC, is this an oversight, or is this behavior desired?

          Show
          xkrogen Erik Krogen added a comment - - edited Hey Chris Trezzo , I have a question about the behavior of this patch. Currently the old logic for resource visibility is used, so if a resource is world-readable, it will be marked as PUBLIC, else PRIVATE. Given my current understanding of this patch's behavior, I see the following scenario: Client submits a job with libjar X, which has never been used before. Client contacts SCM to mark X as "used", SCM responds that it does not have X. Client uploads X to staging directory, which I assume here is not world-readable. X is marked as PRIVATE. MR-AM localizes X, then uploads it to the shared cache. Other NMs all localize X as PRIVATE and do not share it with other applications. Client then submits the same job with the same X. Client contacts SCM, and SCM responds with a world-readable (755 dirs / 555 file) path inside of the shared cache. Client does not upload X, and marks X as PUBLIC, since it is currently in a world-readable location. MR-AM and NMs all localize X as PUBLIC and share it with other applications. Please correct me if I am wrong on any of these steps. It seems that it is the expected behavior that X is eventually PUBLIC, given that we asked for it to be uploaded to the publicly shared cache, but it seems unnecessary for it to be marked as PRIVATE the first time around. Do we do this just to avoid changing the existing logic for marking a resource as PRIVATE vs PUBLIC, is this an oversight, or is this behavior desired?
          Hide
          ctrezzo Chris Trezzo added a comment -

          This issue is currently waiting on MAPREDUCE-6862 and MAPREDUCE-6846. I would like to get those two patches in so I don't have to rebase this multiple times.

          Show
          ctrezzo Chris Trezzo added a comment - This issue is currently waiting on MAPREDUCE-6862 and MAPREDUCE-6846 . I would like to get those two patches in so I don't have to rebase this multiple times.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Just to reflect the current status (waiting for an updated patch). Let me know if that is not accurate. Thanks!

          Show
          sjlee0 Sangjin Lee added a comment - Just to reflect the current status (waiting for an updated patch). Let me know if that is not accurate. Thanks!
          Hide
          ctrezzo Chris Trezzo added a comment -

          Note: currently this patch depends on how YARN-3637 is implemented. I will adjust this patch once it is committed.

          Show
          ctrezzo Chris Trezzo added a comment - Note: currently this patch depends on how YARN-3637 is implemented. I will adjust this patch once it is committed.
          Hide
          ctrezzo Chris Trezzo added a comment -

          These are the same 3 checkstyle warnings I mentioned in the comment above.

          Show
          ctrezzo Chris Trezzo added a comment - These are the same 3 checkstyle warnings I mentioned in the comment above.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 25s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
          0 mvndep 1m 13s Maven dependency ordering for branch
          +1 mvninstall 18m 29s trunk passed
          +1 compile 2m 14s trunk passed
          +1 checkstyle 1m 1s trunk passed
          +1 mvnsite 2m 50s trunk passed
          +1 mvneclipse 1m 24s trunk passed
          +1 findbugs 3m 29s trunk passed
          +1 javadoc 1m 33s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 59s the patch passed
          +1 compile 2m 37s the patch passed
          +1 javac 2m 37s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 341 unchanged - 4 fixed = 341 total (was 345)
          -1 checkstyle 0m 52s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 3 new + 1086 unchanged - 11 fixed = 1089 total (was 1097)
          +1 mvnsite 2m 18s the patch passed
          +1 mvneclipse 1m 23s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 4s The patch has no ill-formed XML file.
          +1 findbugs 3m 49s the patch passed
          +1 javadoc 1m 15s the patch passed
          +1 unit 3m 30s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 0m 50s hadoop-mapreduce-client-common in the patch passed.
          +1 unit 9m 21s hadoop-mapreduce-client-app in the patch passed.
          +1 unit 110m 54s hadoop-mapreduce-client-jobclient in the patch passed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          173m 35s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12848192/MAPREDUCE-5951-trunk.019.patch
          JIRA Issue MAPREDUCE-5951
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 3f6e6dcb73b4 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 383aa9c
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6871/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6871/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6871/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 25s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files. 0 mvndep 1m 13s Maven dependency ordering for branch +1 mvninstall 18m 29s trunk passed +1 compile 2m 14s trunk passed +1 checkstyle 1m 1s trunk passed +1 mvnsite 2m 50s trunk passed +1 mvneclipse 1m 24s trunk passed +1 findbugs 3m 29s trunk passed +1 javadoc 1m 33s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 59s the patch passed +1 compile 2m 37s the patch passed +1 javac 2m 37s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 341 unchanged - 4 fixed = 341 total (was 345) -1 checkstyle 0m 52s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 3 new + 1086 unchanged - 11 fixed = 1089 total (was 1097) +1 mvnsite 2m 18s the patch passed +1 mvneclipse 1m 23s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 4s The patch has no ill-formed XML file. +1 findbugs 3m 49s the patch passed +1 javadoc 1m 15s the patch passed +1 unit 3m 30s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 50s hadoop-mapreduce-client-common in the patch passed. +1 unit 9m 21s hadoop-mapreduce-client-app in the patch passed. +1 unit 110m 54s hadoop-mapreduce-client-jobclient in the patch passed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 173m 35s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12848192/MAPREDUCE-5951-trunk.019.patch JIRA Issue MAPREDUCE-5951 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 3f6e6dcb73b4 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 383aa9c Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6871/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6871/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6871/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attached is a v19 to address Sangjin Lee's comments. Thanks!

          Show
          ctrezzo Chris Trezzo added a comment - Attached is a v19 to address Sangjin Lee 's comments. Thanks!
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attaching a documentation that gives an overview of MapReduce support for the shared cache. This will hopefully make reviewing the patch easier!

          I am also working on an updated patch to address the comments from Sangjin Lee. Thanks!

          Show
          ctrezzo Chris Trezzo added a comment - Attaching a documentation that gives an overview of MapReduce support for the shared cache. This will hopefully make reviewing the patch easier! I am also working on an updated patch to address the comments from Sangjin Lee . Thanks!
          Hide
          ctrezzo Chris Trezzo added a comment -

          Thanks Sangjin Lee for the review! I will work on v19 of the patch to address your comments.

          Show
          ctrezzo Chris Trezzo added a comment - Thanks Sangjin Lee for the review! I will work on v19 of the patch to address your comments.
          Hide
          sjlee0 Sangjin Lee added a comment -

          I went over the patch with Chris in some detail today, and am posting the review comments here for the record.

          (MRJobConfig.java)

          • mapreduce.job.jobjar.visibility and mapreduce.job.jobjar.sharedcache.uploadpolicy are computed values that are not user-facing; in that case we should not even define the defaults so that there is no confusion that these values are computed

          (JobResourceUploader.java)

          • l.159-160: I understand lines 161-165 are there to support programmatic use cases of the distributed cache that come in outside of the job submitter code path. Can we make the comments clearer so that the intent of this comes through? We could also annotate them in MRJobConfig.
          • l.171-172: it might be slightly better to use LinkedHashMap. That way, we'd have a predictable iteration order (the order in which they are specified in the user values).
          • l.219: To be fair, this is a bug we need to fix, right? Then can we file a JIRA and add the JIRA id here?
          • l.237-240: I would not worry about handling previous values here. Having duplicate paths is not really supported and the worst case scenario here is to reset this upload policy with the same value.
          • l.260-263: I think we can improve on this, and reconcile the shared cache with the wildcard feature. We could see if any resource is uploaded to the staging directory, and if so, still preserve the wildcard entry. We also need to consider the case where the shared cache is disabled but the wildcard is enabled.
          • l.291-294: same comment as above
          • l.348-351: same comment as above
          • l.388: Since we're dealing with a local filesystem URI based on l.381-382, the authority check is not meaningful. We should remove this check.
          • On a larger note, the path/URI handling between the job jar, libjars, files, and archives is not very consistent, which is an existing behavior. We need to see if they need to get the same consistent treatment for this to work.

          (Job.java)

          • l.1446: I realized later that passing an empty map has an effect of nulling out the config value; perhaps we could make that more explicit in the javadoc and/or comments/code?
          • l.1449-1463: nit: it might be slightly easier to read by using a simple string concatenation with "+" (JVM internally uses the StringBuilder)
          • l.1490: here also it might be better to use LinkedHashMap
          Show
          sjlee0 Sangjin Lee added a comment - I went over the patch with Chris in some detail today, and am posting the review comments here for the record. (MRJobConfig.java) mapreduce.job.jobjar.visibility and mapreduce.job.jobjar.sharedcache.uploadpolicy are computed values that are not user-facing; in that case we should not even define the defaults so that there is no confusion that these values are computed (JobResourceUploader.java) l.159-160: I understand lines 161-165 are there to support programmatic use cases of the distributed cache that come in outside of the job submitter code path. Can we make the comments clearer so that the intent of this comes through? We could also annotate them in MRJobConfig . l.171-172: it might be slightly better to use LinkedHashMap . That way, we'd have a predictable iteration order (the order in which they are specified in the user values). l.219: To be fair, this is a bug we need to fix, right? Then can we file a JIRA and add the JIRA id here? l.237-240: I would not worry about handling previous values here. Having duplicate paths is not really supported and the worst case scenario here is to reset this upload policy with the same value. l.260-263: I think we can improve on this, and reconcile the shared cache with the wildcard feature. We could see if any resource is uploaded to the staging directory, and if so, still preserve the wildcard entry. We also need to consider the case where the shared cache is disabled but the wildcard is enabled. l.291-294: same comment as above l.348-351: same comment as above l.388: Since we're dealing with a local filesystem URI based on l.381-382, the authority check is not meaningful. We should remove this check. On a larger note, the path/URI handling between the job jar, libjars, files, and archives is not very consistent, which is an existing behavior. We need to see if they need to get the same consistent treatment for this to work. (Job.java) l.1446: I realized later that passing an empty map has an effect of nulling out the config value; perhaps we could make that more explicit in the javadoc and/or comments/code? l.1449-1463: nit: it might be slightly easier to read by using a simple string concatenation with "+" (JVM internally uses the StringBuilder ) l.1490: here also it might be better to use LinkedHashMap
          Hide
          ctrezzo Chris Trezzo added a comment -

          I have filed MAPREDUCE-6825 and MAPREDUCE-6824 to address the checkstyle issues around methods that are too long.

          Show
          ctrezzo Chris Trezzo added a comment - I have filed MAPREDUCE-6825 and MAPREDUCE-6824 to address the checkstyle issues around methods that are too long.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
          0 mvndep 0m 10s Maven dependency ordering for branch
          +1 mvninstall 16m 26s trunk passed
          +1 compile 2m 16s trunk passed
          +1 checkstyle 0m 51s trunk passed
          +1 mvnsite 2m 9s trunk passed
          +1 mvneclipse 1m 17s trunk passed
          +1 findbugs 3m 0s trunk passed
          +1 javadoc 1m 26s trunk passed
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 48s the patch passed
          +1 compile 2m 3s the patch passed
          +1 javac 2m 3s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 359 unchanged - 5 fixed = 359 total (was 364)
          -1 checkstyle 0m 42s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 3 new + 1086 unchanged - 11 fixed = 1089 total (was 1097)
          +1 mvnsite 1m 49s the patch passed
          +1 mvneclipse 1m 3s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 3s The patch has no ill-formed XML file.
          +1 findbugs 3m 41s the patch passed
          +1 javadoc 1m 18s the patch passed
          +1 unit 3m 33s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 0m 55s hadoop-mapreduce-client-common in the patch passed.
          +1 unit 9m 52s hadoop-mapreduce-client-app in the patch passed.
          +1 unit 110m 54s hadoop-mapreduce-client-jobclient in the patch passed.
          +1 asflicense 0m 27s The patch does not generate ASF License warnings.
          167m 29s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843632/MAPREDUCE-5951-trunk.018.patch
          JIRA Issue MAPREDUCE-5951
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 618f3b149fde 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / a956390
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6847/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6847/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6847/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files. 0 mvndep 0m 10s Maven dependency ordering for branch +1 mvninstall 16m 26s trunk passed +1 compile 2m 16s trunk passed +1 checkstyle 0m 51s trunk passed +1 mvnsite 2m 9s trunk passed +1 mvneclipse 1m 17s trunk passed +1 findbugs 3m 0s trunk passed +1 javadoc 1m 26s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 48s the patch passed +1 compile 2m 3s the patch passed +1 javac 2m 3s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 359 unchanged - 5 fixed = 359 total (was 364) -1 checkstyle 0m 42s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 3 new + 1086 unchanged - 11 fixed = 1089 total (was 1097) +1 mvnsite 1m 49s the patch passed +1 mvneclipse 1m 3s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 3s The patch has no ill-formed XML file. +1 findbugs 3m 41s the patch passed +1 javadoc 1m 18s the patch passed +1 unit 3m 33s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 55s hadoop-mapreduce-client-common in the patch passed. +1 unit 9m 52s hadoop-mapreduce-client-app in the patch passed. +1 unit 110m 54s hadoop-mapreduce-client-jobclient in the patch passed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 167m 29s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843632/MAPREDUCE-5951-trunk.018.patch JIRA Issue MAPREDUCE-5951 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 618f3b149fde 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / a956390 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6847/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6847/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6847/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment - - edited

          Attached is v18 to fix the one checkstyle issue. There are three outstanding checkstyle issues that I am leaning towards not fixing as part of the patch. Please let me know your thoughts. They are the following:

          ./hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TaskAttemptImpl.java:752: private static ContainerLaunchContext createCommonContainerLaunchContext(:3: Method length is 172 lines (max allowed is 150).

          This patch barely touches this method so it seems wrong to refactor the method as part of this jira. I can file a separate jira to fix this.

          ./hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/main/java/org/apache/hadoop/mapred/YARNRunner.java:341: public ApplicationSubmissionContext createApplicationSubmissionContext(:3: Method length is 249 lines (max allowed is 150).

          The same reasoning applies to this warning as the previous issue. I can file a separate jira to fix this.

          ./hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java:572: private static void parseDistributedCacheArtifacts(:23: More than 7 parameters (found 8).

          This issue was caused by this patch adding an additional parameter to this method. I can fix the number of parameter issues, but that forces me to touch three existing calls to the deprecated DistributedCache api, which would fix 1 warning but create 3 new ones. It is a larger change to not use the deprecated api because the existing code is not set up to use it, furthermore use of the deprecated api is currently widespread in this code. My thoughts are that I will leave this warning as is.

          Show
          ctrezzo Chris Trezzo added a comment - - edited Attached is v18 to fix the one checkstyle issue. There are three outstanding checkstyle issues that I am leaning towards not fixing as part of the patch. Please let me know your thoughts. They are the following: ./hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TaskAttemptImpl.java:752: private static ContainerLaunchContext createCommonContainerLaunchContext(:3: Method length is 172 lines (max allowed is 150). This patch barely touches this method so it seems wrong to refactor the method as part of this jira. I can file a separate jira to fix this. ./hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/main/java/org/apache/hadoop/mapred/YARNRunner.java:341: public ApplicationSubmissionContext createApplicationSubmissionContext(:3: Method length is 249 lines (max allowed is 150). The same reasoning applies to this warning as the previous issue. I can file a separate jira to fix this. ./hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java:572: private static void parseDistributedCacheArtifacts(:23: More than 7 parameters (found 8). This issue was caused by this patch adding an additional parameter to this method. I can fix the number of parameter issues, but that forces me to touch three existing calls to the deprecated DistributedCache api, which would fix 1 warning but create 3 new ones. It is a larger change to not use the deprecated api because the existing code is not set up to use it, furthermore use of the deprecated api is currently widespread in this code. My thoughts are that I will leave this warning as is.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
          0 mvndep 0m 51s Maven dependency ordering for branch
          +1 mvninstall 7m 56s trunk passed
          +1 compile 1m 37s trunk passed
          +1 checkstyle 0m 40s trunk passed
          +1 mvnsite 1m 49s trunk passed
          +1 mvneclipse 0m 59s trunk passed
          +1 findbugs 2m 21s trunk passed
          +1 javadoc 1m 8s trunk passed
          0 mvndep 0m 7s Maven dependency ordering for patch
          +1 mvninstall 1m 23s the patch passed
          +1 compile 1m 34s the patch passed
          +1 javac 1m 34s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 359 unchanged - 5 fixed = 359 total (was 364)
          -1 checkstyle 0m 38s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 4 new + 1086 unchanged - 11 fixed = 1090 total (was 1097)
          +1 mvnsite 1m 40s the patch passed
          +1 mvneclipse 0m 49s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 2m 44s the patch passed
          +1 javadoc 1m 0s the patch passed
          +1 unit 2m 54s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 0m 42s hadoop-mapreduce-client-common in the patch passed.
          +1 unit 8m 55s hadoop-mapreduce-client-app in the patch passed.
          +1 unit 104m 24s hadoop-mapreduce-client-jobclient in the patch passed.
          +1 asflicense 0m 32s The patch does not generate ASF License warnings.
          146m 3s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843339/MAPREDUCE-5951-trunk.017.patch
          JIRA Issue MAPREDUCE-5951
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 563b10e197ee 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 64a2d5b
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6845/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6845/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6845/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files. 0 mvndep 0m 51s Maven dependency ordering for branch +1 mvninstall 7m 56s trunk passed +1 compile 1m 37s trunk passed +1 checkstyle 0m 40s trunk passed +1 mvnsite 1m 49s trunk passed +1 mvneclipse 0m 59s trunk passed +1 findbugs 2m 21s trunk passed +1 javadoc 1m 8s trunk passed 0 mvndep 0m 7s Maven dependency ordering for patch +1 mvninstall 1m 23s the patch passed +1 compile 1m 34s the patch passed +1 javac 1m 34s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 359 unchanged - 5 fixed = 359 total (was 364) -1 checkstyle 0m 38s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 4 new + 1086 unchanged - 11 fixed = 1090 total (was 1097) +1 mvnsite 1m 40s the patch passed +1 mvneclipse 0m 49s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 2m 44s the patch passed +1 javadoc 1m 0s the patch passed +1 unit 2m 54s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 42s hadoop-mapreduce-client-common in the patch passed. +1 unit 8m 55s hadoop-mapreduce-client-app in the patch passed. +1 unit 104m 24s hadoop-mapreduce-client-jobclient in the patch passed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 146m 3s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843339/MAPREDUCE-5951-trunk.017.patch JIRA Issue MAPREDUCE-5951 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 563b10e197ee 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 64a2d5b Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6845/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6845/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6845/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attaching v17 to address findbugs, checkstyle and whitespace errors.

          Show
          ctrezzo Chris Trezzo added a comment - Attaching v17 to address findbugs, checkstyle and whitespace errors.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 6 new or modified test files.
          0 mvndep 0m 12s Maven dependency ordering for branch
          +1 mvninstall 7m 23s trunk passed
          +1 compile 1m 41s trunk passed
          +1 checkstyle 0m 40s trunk passed
          +1 mvnsite 1m 55s trunk passed
          +1 mvneclipse 1m 2s trunk passed
          +1 findbugs 2m 26s trunk passed
          +1 javadoc 1m 11s trunk passed
          0 mvndep 0m 9s Maven dependency ordering for patch
          +1 mvninstall 1m 30s the patch passed
          +1 compile 1m 48s the patch passed
          +1 javac 1m 48s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 359 unchanged - 5 fixed = 359 total (was 364)
          -1 checkstyle 0m 38s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 48 new + 1086 unchanged - 11 fixed = 1134 total (was 1097)
          +1 mvnsite 1m 45s the patch passed
          +1 mvneclipse 0m 51s the patch passed
          -1 whitespace 0m 1s The patch has 3 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          -1 findbugs 0m 48s hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
          +1 javadoc 1m 4s the patch passed
          +1 unit 3m 0s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 0m 44s hadoop-mapreduce-client-common in the patch passed.
          +1 unit 9m 7s hadoop-mapreduce-client-app in the patch passed.
          +1 unit 110m 26s hadoop-mapreduce-client-jobclient in the patch passed.
          +1 asflicense 0m 28s The patch does not generate ASF License warnings.
          152m 19s



          Reason Tests
          FindBugs module:hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common
            Boxed value is unboxed and then immediately reboxed in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java:then immediately reboxed in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java:[line 639]
            java.net.URI is incompatible with expected argument type String in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java:argument type String in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java:[line 637]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843109/MAPREDUCE-5951-trunk.016.patch
          JIRA Issue MAPREDUCE-5951
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 7b0e5fd931a9 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / ef34bf2
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/artifact/patchprocess/whitespace-eol.txt
          findbugs https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/artifact/patchprocess/new-findbugs-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-common.html
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 6 new or modified test files. 0 mvndep 0m 12s Maven dependency ordering for branch +1 mvninstall 7m 23s trunk passed +1 compile 1m 41s trunk passed +1 checkstyle 0m 40s trunk passed +1 mvnsite 1m 55s trunk passed +1 mvneclipse 1m 2s trunk passed +1 findbugs 2m 26s trunk passed +1 javadoc 1m 11s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 1m 30s the patch passed +1 compile 1m 48s the patch passed +1 javac 1m 48s hadoop-mapreduce-project_hadoop-mapreduce-client generated 0 new + 359 unchanged - 5 fixed = 359 total (was 364) -1 checkstyle 0m 38s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 48 new + 1086 unchanged - 11 fixed = 1134 total (was 1097) +1 mvnsite 1m 45s the patch passed +1 mvneclipse 0m 51s the patch passed -1 whitespace 0m 1s The patch has 3 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 xml 0m 2s The patch has no ill-formed XML file. -1 findbugs 0m 48s hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) +1 javadoc 1m 4s the patch passed +1 unit 3m 0s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 44s hadoop-mapreduce-client-common in the patch passed. +1 unit 9m 7s hadoop-mapreduce-client-app in the patch passed. +1 unit 110m 26s hadoop-mapreduce-client-jobclient in the patch passed. +1 asflicense 0m 28s The patch does not generate ASF License warnings. 152m 19s Reason Tests FindBugs module:hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common   Boxed value is unboxed and then immediately reboxed in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java:then immediately reboxed in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java: [line 639]   java.net.URI is incompatible with expected argument type String in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java:argument type String in org.apache.hadoop.mapreduce.v2.util.MRApps.parseDistributedCacheArtifacts(Configuration, Map, LocalResourceType, URI[], long[], long[], boolean[], Map) At MRApps.java: [line 637] Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843109/MAPREDUCE-5951-trunk.016.patch JIRA Issue MAPREDUCE-5951 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 7b0e5fd931a9 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ef34bf2 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/artifact/patchprocess/whitespace-eol.txt findbugs https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/artifact/patchprocess/new-findbugs-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-common.html Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6843/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attaching v16 for a QA run. This was a non-trivial rebase. I have also modified the patch so that it no longer uses arrays in the JobResourceUploader as requested by Karthik Kambatla.

          Show
          ctrezzo Chris Trezzo added a comment - Attaching v16 for a QA run. This was a non-trivial rebase. I have also modified the patch so that it no longer uses arrays in the JobResourceUploader as requested by Karthik Kambatla .
          Hide
          sjlee0 Sangjin Lee added a comment -

          That sounds fine with me. Thanks!

          Show
          sjlee0 Sangjin Lee added a comment - That sounds fine with me. Thanks!
          Hide
          ctrezzo Chris Trezzo added a comment -

          Sangjin Lee thanks for pointing out YARN-3116. If there isn't a strong objection, for the sake of getting this patch committed I will leverage that work in a follow on patch. I can file a separate jira.

          Show
          ctrezzo Chris Trezzo added a comment - Sangjin Lee thanks for pointing out YARN-3116 . If there isn't a strong objection, for the sake of getting this patch committed I will leverage that work in a follow on patch. I can file a separate jira.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Thanks for the clarification Chris Trezzo.

          There is a recent JIRA that got merged which now enables discovery of whether a certain container is an AM: YARN-3116. It exposes the "container type" in the container context. Taking advantage of it might simplify some of the work we do here.

          Show
          sjlee0 Sangjin Lee added a comment - Thanks for the clarification Chris Trezzo . There is a recent JIRA that got merged which now enables discovery of whether a certain container is an AM: YARN-3116 . It exposes the "container type" in the container context. Taking advantage of it might simplify some of the work we do here.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Findbugs run was clean this time so I think the previous empty result was just wrong. As perviously mentioned the whitespace errors are not part of this patch and the style errors are remaining consistent with the current code convention.

          Show
          ctrezzo Chris Trezzo added a comment - Findbugs run was clean this time so I think the previous empty result was just wrong. As perviously mentioned the whitespace errors are not part of this patch and the style errors are remaining consistent with the current code convention.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          -1 pre-patch 18m 33s Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 5 new or modified test files.
          +1 javac 7m 36s There were no new javac warning messages.
          +1 javadoc 9m 41s 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 1m 57s The applied patch generated 13 new checkstyle issues (total was 578, now 589).
          -1 whitespace 0m 25s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 23s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 4m 30s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
          +1 mapreduce tests 9m 4s Tests passed in hadoop-mapreduce-client-app.
          +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common.
          +1 mapreduce tests 1m 45s Tests passed in hadoop-mapreduce-client-core.
          +1 mapreduce tests 108m 54s Tests passed in hadoop-mapreduce-client-jobclient.
              165m 38s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12744751/MAPREDUCE-5951-trunk-v15.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 0824426
          Pre-patch Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/trunkFindbugsWarningshadoop-mapreduce-client-app.html
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/whitespace.txt
          hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt
          hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt
          hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt
          hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/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-MAPREDUCE-Build/5883/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment -1 pre-patch 18m 33s Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 5 new or modified test files. +1 javac 7m 36s There were no new javac warning messages. +1 javadoc 9m 41s 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 1m 57s The applied patch generated 13 new checkstyle issues (total was 578, now 589). -1 whitespace 0m 25s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 23s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 4m 30s The patch does not introduce any new Findbugs (version 3.0.0) warnings. +1 mapreduce tests 9m 4s Tests passed in hadoop-mapreduce-client-app. +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common. +1 mapreduce tests 1m 45s Tests passed in hadoop-mapreduce-client-core. +1 mapreduce tests 108m 54s Tests passed in hadoop-mapreduce-client-jobclient.     165m 38s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12744751/MAPREDUCE-5951-trunk-v15.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 0824426 Pre-patch Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/trunkFindbugsWarningshadoop-mapreduce-client-app.html checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/whitespace.txt hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5883/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-MAPREDUCE-Build/5883/console This message was automatically generated.
          Hide
          sjlee0 Sangjin Lee added a comment -

          I've seen several cases where the findbugs result from jenkins is empty. Yet, I think it'd be a good idea to double check by running mvn findbugs:findbugs at the said project to see if it is really clean.

          Show
          sjlee0 Sangjin Lee added a comment - I've seen several cases where the findbugs result from jenkins is empty. Yet, I think it'd be a good idea to double check by running mvn findbugs:findbugs at the said project to see if it is really clean.
          Hide
          ctrezzo Chris Trezzo added a comment -

          v15 attached.

          1. Fixed whitespace in JobResourceUploader and TestLocalJobSubmission. The rest of the whitespace errors are not in lines of code that this patch touched (they are only in the context supplied by the patch).
          2. Checkstyle shows several errors, but I did not address any of them. The Method length will be resolved by MAPREDUCE-6365. The rest of the errors are complaining about things that if I were to address I would not be conforming to current project practices. For example the redundant public modifier in MRJobConfig and the TODO comment. Karthik Kambatla, let me know if you want me to fix these anyways.
          3. Findbugs gave a -1, but if you click into the results it did not show any errors or warnings. Weird. I will ignore the -1 then.
          4. There was 1 test failure, but it passes locally and it looks like it is unrelated (NoClassDefFoundError while shutting down a MiniMRYarnCluster).
          Show
          ctrezzo Chris Trezzo added a comment - v15 attached. Fixed whitespace in JobResourceUploader and TestLocalJobSubmission. The rest of the whitespace errors are not in lines of code that this patch touched (they are only in the context supplied by the patch). Checkstyle shows several errors, but I did not address any of them. The Method length will be resolved by MAPREDUCE-6365 . The rest of the errors are complaining about things that if I were to address I would not be conforming to current project practices. For example the redundant public modifier in MRJobConfig and the TODO comment. Karthik Kambatla , let me know if you want me to fix these anyways. Findbugs gave a -1, but if you click into the results it did not show any errors or warnings. Weird. I will ignore the -1 then. There was 1 test failure, but it passes locally and it looks like it is unrelated (NoClassDefFoundError while shutting down a MiniMRYarnCluster).
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          -1 pre-patch 18m 46s Findbugs (version 3.0.0) appears to be broken on trunk.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 5 new or modified test files.
          +1 javac 7m 51s There were no new javac warning messages.
          +1 javadoc 9m 49s 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 1m 55s The applied patch generated 13 new checkstyle issues (total was 578, now 589).
          -1 whitespace 0m 25s The patch has 10 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 22s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          -1 findbugs 4m 45s The patch appears to introduce 1 new Findbugs (version 3.0.0) warnings.
          +1 mapreduce tests 9m 4s Tests passed in hadoop-mapreduce-client-app.
          +1 mapreduce tests 0m 47s Tests passed in hadoop-mapreduce-client-common.
          +1 mapreduce tests 1m 44s Tests passed in hadoop-mapreduce-client-core.
          -1 mapreduce tests 108m 35s Tests failed in hadoop-mapreduce-client-jobclient.
              166m 10s  



          Reason Tests
          FindBugs module:hadoop-mapreduce-client-app
          Failed unit tests hadoop.mapred.TestJobSysDirWithDFS



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12744577/MAPREDUCE-5951-trunk-v14.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / f4ca530
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/whitespace.txt
          Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html
          hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt
          hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt
          hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt
          hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/testReport/
          Java 1.7.0_55
          uname Linux asf903.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-MAPREDUCE-Build/5879/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment -1 pre-patch 18m 46s Findbugs (version 3.0.0) appears to be broken on trunk. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 5 new or modified test files. +1 javac 7m 51s There were no new javac warning messages. +1 javadoc 9m 49s 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 1m 55s The applied patch generated 13 new checkstyle issues (total was 578, now 589). -1 whitespace 0m 25s The patch has 10 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 22s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. -1 findbugs 4m 45s The patch appears to introduce 1 new Findbugs (version 3.0.0) warnings. +1 mapreduce tests 9m 4s Tests passed in hadoop-mapreduce-client-app. +1 mapreduce tests 0m 47s Tests passed in hadoop-mapreduce-client-common. +1 mapreduce tests 1m 44s Tests passed in hadoop-mapreduce-client-core. -1 mapreduce tests 108m 35s Tests failed in hadoop-mapreduce-client-jobclient.     166m 10s   Reason Tests FindBugs module:hadoop-mapreduce-client-app Failed unit tests hadoop.mapred.TestJobSysDirWithDFS Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12744577/MAPREDUCE-5951-trunk-v14.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / f4ca530 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/whitespace.txt Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5879/testReport/ Java 1.7.0_55 uname Linux asf903.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-MAPREDUCE-Build/5879/console This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attached v14.

          Fixed JobResourceUploader so that TestLocalJobSubmission#testLocalJobLibjarsOption passes. Also added two new test cases to TestLocalJobSubmission for files and archives.

          Show
          ctrezzo Chris Trezzo added a comment - Attached v14. Fixed JobResourceUploader so that TestLocalJobSubmission#testLocalJobLibjarsOption passes. Also added two new test cases to TestLocalJobSubmission for files and archives.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Investigating TestLocalJobSubmission test failure.

          Show
          ctrezzo Chris Trezzo added a comment - Investigating TestLocalJobSubmission test failure.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          -1 pre-patch 18m 54s Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 4 new or modified test files.
          +1 javac 7m 37s There were no new javac warning messages.
          +1 javadoc 9m 30s 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 1m 58s The applied patch generated 13 new checkstyle issues (total was 578, now 589).
          -1 whitespace 0m 21s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 38s mvn install still works.
          +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
          +1 findbugs 4m 23s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
          +1 mapreduce tests 9m 1s Tests passed in hadoop-mapreduce-client-app.
          +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common.
          +1 mapreduce tests 1m 43s Tests passed in hadoop-mapreduce-client-core.
          -1 mapreduce tests 108m 40s Tests failed in hadoop-mapreduce-client-jobclient.
              165m 34s  



          Reason Tests
          Failed unit tests hadoop.mapred.TestLocalJobSubmission



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12743839/MAPREDUCE-5951-trunk-v13.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 81f3644
          Pre-patch Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/trunkFindbugsWarningshadoop-mapreduce-client-app.html
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/whitespace.txt
          hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt
          hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt
          hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt
          hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/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-MAPREDUCE-Build/5871/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment -1 pre-patch 18m 54s Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 4 new or modified test files. +1 javac 7m 37s There were no new javac warning messages. +1 javadoc 9m 30s 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 1m 58s The applied patch generated 13 new checkstyle issues (total was 578, now 589). -1 whitespace 0m 21s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 38s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 4m 23s The patch does not introduce any new Findbugs (version 3.0.0) warnings. +1 mapreduce tests 9m 1s Tests passed in hadoop-mapreduce-client-app. +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common. +1 mapreduce tests 1m 43s Tests passed in hadoop-mapreduce-client-core. -1 mapreduce tests 108m 40s Tests failed in hadoop-mapreduce-client-jobclient.     165m 34s   Reason Tests Failed unit tests hadoop.mapred.TestLocalJobSubmission Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12743839/MAPREDUCE-5951-trunk-v13.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 81f3644 Pre-patch Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/trunkFindbugsWarningshadoop-mapreduce-client-app.html checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/whitespace.txt hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5871/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-MAPREDUCE-Build/5871/console This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          -1 pre-patch 19m 16s Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 tests included 0m 0s The patch appears to include 4 new or modified test files.
          +1 javac 7m 29s There were no new javac warning messages.
          +1 javadoc 9m 37s 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 1m 59s The applied patch generated 13 new checkstyle issues (total was 578, now 589).
          -1 whitespace 0m 31s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 36s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 4m 29s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
          +1 mapreduce tests 9m 4s Tests passed in hadoop-mapreduce-client-app.
          +1 mapreduce tests 0m 46s Tests passed in hadoop-mapreduce-client-common.
          +1 mapreduce tests 1m 44s Tests passed in hadoop-mapreduce-client-core.
          -1 mapreduce tests 108m 34s Tests failed in hadoop-mapreduce-client-jobclient.
              166m 12s  



          Reason Tests
          Failed unit tests hadoop.mapred.TestLocalJobSubmission



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12743839/MAPREDUCE-5951-trunk-v13.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 81f3644
          Pre-patch Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/trunkFindbugsWarningshadoop-mapreduce-client-app.html
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/whitespace.txt
          hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt
          hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt
          hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt
          hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/testReport/
          Java 1.7.0_55
          uname Linux asf906.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-MAPREDUCE-Build/5870/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment -1 pre-patch 19m 16s Pre-patch trunk has 1 extant Findbugs (version 3.0.0) warnings. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 4 new or modified test files. +1 javac 7m 29s There were no new javac warning messages. +1 javadoc 9m 37s 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 1m 59s The applied patch generated 13 new checkstyle issues (total was 578, now 589). -1 whitespace 0m 31s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 36s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 4m 29s The patch does not introduce any new Findbugs (version 3.0.0) warnings. +1 mapreduce tests 9m 4s Tests passed in hadoop-mapreduce-client-app. +1 mapreduce tests 0m 46s Tests passed in hadoop-mapreduce-client-common. +1 mapreduce tests 1m 44s Tests passed in hadoop-mapreduce-client-core. -1 mapreduce tests 108m 34s Tests failed in hadoop-mapreduce-client-jobclient.     166m 12s   Reason Tests Failed unit tests hadoop.mapred.TestLocalJobSubmission Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12743839/MAPREDUCE-5951-trunk-v13.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 81f3644 Pre-patch Findbugs warnings https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/trunkFindbugsWarningshadoop-mapreduce-client-app.html checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/whitespace.txt hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5870/testReport/ Java 1.7.0_55 uname Linux asf906.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-MAPREDUCE-Build/5870/console This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Karthik Kambatla apologies for the long response time. Attached is v13.

          1. Rebased.
          2. Cleaned up imports.
          3. Fixed POM indentation.
          4. Added TODO with MAPREDUCE-6365 jira for refactor of uploadFilesInternal and fixing the use of arrays for upload policy tracking.

          After a hadoopQA run I will re-look at the unit tests. If I remember correctly, they did not seem related last run. I will double check.

          Show
          ctrezzo Chris Trezzo added a comment - Karthik Kambatla apologies for the long response time. Attached is v13. Rebased. Cleaned up imports. Fixed POM indentation. Added TODO with MAPREDUCE-6365 jira for refactor of uploadFilesInternal and fixing the use of arrays for upload policy tracking. After a hadoopQA run I will re-look at the unit tests. If I remember correctly, they did not seem related last run. I will double check.
          Hide
          kasha Karthik Kambatla added a comment -

          Nice to see the patch size go down every iteration

          Looks mostly good. As we discussed offline, Minor comments outside of that:

          1. Let us fix the use of arrays to capture per-resource upload policies in a follow-up JIRA. Can we add a TODO in the source with a JIRA number please.
          2. Are the test failures related?
          3. pom indentation is broken
          Show
          kasha Karthik Kambatla added a comment - Nice to see the patch size go down every iteration Looks mostly good. As we discussed offline, Minor comments outside of that: Let us fix the use of arrays to capture per-resource upload policies in a follow-up JIRA. Can we add a TODO in the source with a JIRA number please. Are the test failures related? pom indentation is broken
          Hide
          ctrezzo Chris Trezzo added a comment -

          1. All checkstyle errors (except for the unused import) pertain to lines that were not modified by this patch.
          2. All of the whitespace errors pertain to lines that were not modified by this patch.
          3. It seems like the unit test run was in a bad state as well. There were a bunch of address already in use exceptions on unrelated tests and ClassDefNotFoundExceptions on shared cache tests that pass locally.

          Karthik Kambatla I can fix the unused import issue, but will wait to post new version until I hear from you (just in case you have other comments about the patch). Thanks!

          Show
          ctrezzo Chris Trezzo added a comment - 1. All checkstyle errors (except for the unused import) pertain to lines that were not modified by this patch. 2. All of the whitespace errors pertain to lines that were not modified by this patch. 3. It seems like the unit test run was in a bad state as well. There were a bunch of address already in use exceptions on unrelated tests and ClassDefNotFoundExceptions on shared cache tests that pass locally. Karthik Kambatla I can fix the unused import issue, but will wait to post new version until I hear from you (just in case you have other comments about the patch). Thanks!
          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 4 new or modified test files.
          +1 javac 7m 37s There were no new javac warning messages.
          +1 javadoc 9m 42s 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 1m 58s The applied patch generated 13 new checkstyle issues (total was 568, now 581).
          -1 whitespace 0m 21s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 37s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 3m 52s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          -1 mapreduce tests 9m 6s Tests failed in hadoop-mapreduce-client-app.
          +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common.
          +1 mapreduce tests 1m 37s Tests passed in hadoop-mapreduce-client-core.
          -1 mapreduce tests 101m 6s Tests failed in hadoop-mapreduce-client-jobclient.
              153m 31s  



          Reason Tests
          Failed unit tests hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf
            hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs
            hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts
            hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempt
            hadoop.mapred.TestMiniMRChildTask
            hadoop.mapred.TestTextOutputFormat
            hadoop.mapred.TestLocalJobSubmission
            hadoop.mapred.TestFileOutputFormat
            hadoop.mapred.lib.TestKeyFieldBasedComparator
            hadoop.mapreduce.TestJobResourceUploader



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12733228/MAPREDUCE-5951-trunk-v12.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 03a293a
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/whitespace.txt
          hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt
          hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt
          hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt
          hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/testReport/
          Java 1.7.0_55
          uname Linux asf903.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-MAPREDUCE-Build/5739/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 4 new or modified test files. +1 javac 7m 37s There were no new javac warning messages. +1 javadoc 9m 42s 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 1m 58s The applied patch generated 13 new checkstyle issues (total was 568, now 581). -1 whitespace 0m 21s The patch has 8 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 37s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 3m 52s The patch does not introduce any new Findbugs (version 2.0.3) warnings. -1 mapreduce tests 9m 6s Tests failed in hadoop-mapreduce-client-app. +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common. +1 mapreduce tests 1m 37s Tests passed in hadoop-mapreduce-client-core. -1 mapreduce tests 101m 6s Tests failed in hadoop-mapreduce-client-jobclient.     153m 31s   Reason Tests Failed unit tests hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobConf   hadoop.mapreduce.v2.app.webapp.TestAMWebServicesJobs   hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempts   hadoop.mapreduce.v2.app.webapp.TestAMWebServicesAttempt   hadoop.mapred.TestMiniMRChildTask   hadoop.mapred.TestTextOutputFormat   hadoop.mapred.TestLocalJobSubmission   hadoop.mapred.TestFileOutputFormat   hadoop.mapred.lib.TestKeyFieldBasedComparator   hadoop.mapreduce.TestJobResourceUploader Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12733228/MAPREDUCE-5951-trunk-v12.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 03a293a checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/whitespace.txt hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5739/testReport/ Java 1.7.0_55 uname Linux asf903.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-MAPREDUCE-Build/5739/console This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          V12 attached.

          1. Removed calls to the shared cache release API.
          2. Fixed whitespace/style-check issues, except for the "Redundant 'public' modifier." errors in MRJobConfig.
          3. TestLocalJobSubmission test failure seems unrelated to this patch.

          Show
          ctrezzo Chris Trezzo added a comment - V12 attached. 1. Removed calls to the shared cache release API. 2. Fixed whitespace/style-check issues, except for the "Redundant 'public' modifier." errors in MRJobConfig. 3. TestLocalJobSubmission test failure seems unrelated to this patch.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Another note about removal of the release api: Without the release api, long running applications will not be able to release resources they are no longer using. Their appId will be claiming that resource until the application is finished.

          Show
          ctrezzo Chris Trezzo added a comment - Another note about removal of the release api: Without the release api, long running applications will not be able to release resources they are no longer using. Their appId will be claiming that resource until the application is finished.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Note for the above calculation: We can probably get rid of shortUserName from the resource reference if we are not doing release calls. If that is the case, then the additional memory should be even smaller.

          Show
          ctrezzo Chris Trezzo added a comment - Note for the above calculation: We can probably get rid of shortUserName from the resource reference if we are not doing release calls. If that is the case, then the additional memory should be even smaller.
          Hide
          ctrezzo Chris Trezzo added a comment -

          1. Karthik Kambatla I have thought about removing the release api more and also discussed with Sangjin Lee. I think it makes sense from a code simplicity standpoint to remove the release api. This will eliminate the need for multiple arrays and keeping track of the resources you use on the client side. If we feel that it is needed later on, we can always add it back in. The major consequence of not releasing is that the SCM store will have more resource references to keep track of during the cleaner period time (currently defaulting to 1 day). For the InMemorySCMStore, this means that there will be more SharedCacheResourceReference objects in-memory.

          Rough hand-wavy calculation for heapsize over a 24 hour period on a large cluster:

          • 42k jobs per day x 600 resources per job = 25.2 million resource references
          • A resource reference is made up of an ApplicationId and a ShortUserName.
            • Let's say the ApplicationId is two longs, so 16 bytes, and the shortUserName is 10 characters, so 20 bytes.
            • Let's also multiply this number by 3 to account for Object overhead. So (16 + 20) * 3 = 108 bytes for a single resource reference.
          • 25.2 million * 108 bytes = 2.7 GB of total heap space

          2.7 GB of extra memory does not strike me of being too crazy. We can also trade off RM load for memory size and run the cleaner at a higher frequency. Thoughts from others? If that sounds reasonable, I will file a YARN jira to make the change.

          2. Jason Lowe I will add a comment that explains why we are now using '*' instead of MRJobConfig.JOB_JAR.

          Show
          ctrezzo Chris Trezzo added a comment - 1. Karthik Kambatla I have thought about removing the release api more and also discussed with Sangjin Lee . I think it makes sense from a code simplicity standpoint to remove the release api. This will eliminate the need for multiple arrays and keeping track of the resources you use on the client side. If we feel that it is needed later on, we can always add it back in. The major consequence of not releasing is that the SCM store will have more resource references to keep track of during the cleaner period time (currently defaulting to 1 day). For the InMemorySCMStore, this means that there will be more SharedCacheResourceReference objects in-memory. Rough hand-wavy calculation for heapsize over a 24 hour period on a large cluster: 42k jobs per day x 600 resources per job = 25.2 million resource references A resource reference is made up of an ApplicationId and a ShortUserName. Let's say the ApplicationId is two longs, so 16 bytes, and the shortUserName is 10 characters, so 20 bytes. Let's also multiply this number by 3 to account for Object overhead. So (16 + 20) * 3 = 108 bytes for a single resource reference. 25.2 million * 108 bytes = 2.7 GB of total heap space 2.7 GB of extra memory does not strike me of being too crazy. We can also trade off RM load for memory size and run the cleaner at a higher frequency. Thoughts from others? If that sounds reasonable, I will file a YARN jira to make the change. 2. Jason Lowe I will add a comment that explains why we are now using '*' instead of MRJobConfig.JOB_JAR.
          Hide
          jlowe Jason Lowe added a comment -

          MRApps - we are using "*" instead of "job.jar" which should work, but I wonder if that will be an incompatible behavior change. Jason Lowe, Vinod Kumar Vavilapalli - what do you think?

          I get why the '*' is desirable since the name of the jar being shared and replacing our job jar might not match the job jar name we expect, although I think a comment stating why would help explain why we're not using the more specific job jar name that normally would be expected. That would also help prevent someone else coming along much later and "fixing" that.

          I think we're probably OK with using * specifically for the job jar because we explicitly use mapreduce.job.jar.unpack.pattern when we unarchive it. That means we'll only unarchive the classes and lib portions of the archive and not any other files from it, by default. So by default we shouldn't be picking up any other jars within the job jar. In theory someone might have packed other jars into the job jar, modified the unpack pattern property to pick up those jars, then explicitly set their classpath to pick up only a portion of those jars or all of them in a specific order. '*' does not guarantee order in any way, so that could break that scenario. I'm not sure that scenario is likely, however.

          Show
          jlowe Jason Lowe added a comment - MRApps - we are using "*" instead of "job.jar" which should work, but I wonder if that will be an incompatible behavior change. Jason Lowe, Vinod Kumar Vavilapalli - what do you think? I get why the '*' is desirable since the name of the jar being shared and replacing our job jar might not match the job jar name we expect, although I think a comment stating why would help explain why we're not using the more specific job jar name that normally would be expected. That would also help prevent someone else coming along much later and "fixing" that. I think we're probably OK with using * specifically for the job jar because we explicitly use mapreduce.job.jar.unpack.pattern when we unarchive it. That means we'll only unarchive the classes and lib portions of the archive and not any other files from it, by default. So by default we shouldn't be picking up any other jars within the job jar. In theory someone might have packed other jars into the job jar, modified the unpack pattern property to pick up those jars, then explicitly set their classpath to pick up only a portion of those jars or all of them in a specific order. '*' does not guarantee order in any way, so that could break that scenario. I'm not sure that scenario is likely, however.
          Hide
          kasha Karthik Kambatla added a comment -

          The latest patch is definitely easier to understand.

          A couple of high-level comments (sorry for missing these earlier):

          1. We seem to be using arrays to capture upload policies for individual resources in JobResourceUploader. I feel that is error-prone, and it would be nice to avoid using arrays.
          2. Do we need to release the resources? As per our offline discussion, an application calling release doesn't really affect when the resources are actually cleaned up. May be getting rid of it altogether will help us simplify both this patch and the SCM source as well?
          Show
          kasha Karthik Kambatla added a comment - The latest patch is definitely easier to understand. A couple of high-level comments (sorry for missing these earlier): We seem to be using arrays to capture upload policies for individual resources in JobResourceUploader. I feel that is error-prone, and it would be nice to avoid using arrays. Do we need to release the resources? As per our offline discussion, an application calling release doesn't really affect when the resources are actually cleaned up. May be getting rid of it altogether will help us simplify both this patch and the SCM source as well?
          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 4 new or modified test files.
          +1 javac 7m 30s 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 1m 59s The applied patch generated 27 new checkstyle issues (total was 568, now 595).
          -1 whitespace 0m 25s The patch has 18 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 37s mvn install still works.
          +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
          +1 findbugs 3m 53s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 mapreduce tests 9m 36s Tests passed in hadoop-mapreduce-client-app.
          +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common.
          +1 mapreduce tests 1m 35s Tests passed in hadoop-mapreduce-client-core.
          -1 mapreduce tests 103m 52s Tests failed in hadoop-mapreduce-client-jobclient.
              156m 34s  



          Reason Tests
          Failed unit tests hadoop.mapred.TestLocalJobSubmission



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12732450/MAPREDUCE-5951-trunk-v11.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / 2463666
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/whitespace.txt
          hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt
          hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt
          hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt
          hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/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-MAPREDUCE-Build/5720/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 4 new or modified test files. +1 javac 7m 30s 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 1m 59s The applied patch generated 27 new checkstyle issues (total was 568, now 595). -1 whitespace 0m 25s The patch has 18 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 37s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 3m 53s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 mapreduce tests 9m 36s Tests passed in hadoop-mapreduce-client-app. +1 mapreduce tests 0m 45s Tests passed in hadoop-mapreduce-client-common. +1 mapreduce tests 1m 35s Tests passed in hadoop-mapreduce-client-core. -1 mapreduce tests 103m 52s Tests failed in hadoop-mapreduce-client-jobclient.     156m 34s   Reason Tests Failed unit tests hadoop.mapred.TestLocalJobSubmission Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12732450/MAPREDUCE-5951-trunk-v11.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 2463666 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/whitespace.txt hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5720/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-MAPREDUCE-Build/5720/console This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 35s 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 4 new or modified test files.
          +1 javac 7m 28s There were no new javac warning messages.
          +1 javadoc 9m 34s 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 1m 59s The applied patch generated 30 new checkstyle issues (total was 583, now 612).
          -1 whitespace 0m 54s The patch has 23 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 38s mvn install still works.
          +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
          +1 findbugs 3m 52s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 mapreduce tests 9m 21s Tests passed in hadoop-mapreduce-client-app.
          +1 mapreduce tests 0m 46s Tests passed in hadoop-mapreduce-client-common.
          +1 mapreduce tests 1m 36s Tests passed in hadoop-mapreduce-client-core.
          -1 mapreduce tests 104m 37s Tests failed in hadoop-mapreduce-client-jobclient.
              157m 25s  



          Reason Tests
          Failed unit tests hadoop.mapred.TestLocalJobSubmission



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12732403/MAPREDUCE-5951-trunk-v10.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / f24452d
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt
          whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/whitespace.txt
          hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt
          hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt
          hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt
          hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/testReport/
          Java 1.7.0_55
          uname Linux asf906.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-MAPREDUCE-Build/5718/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 35s 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 4 new or modified test files. +1 javac 7m 28s There were no new javac warning messages. +1 javadoc 9m 34s 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 1m 59s The applied patch generated 30 new checkstyle issues (total was 583, now 612). -1 whitespace 0m 54s The patch has 23 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 38s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 3m 52s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 mapreduce tests 9m 21s Tests passed in hadoop-mapreduce-client-app. +1 mapreduce tests 0m 46s Tests passed in hadoop-mapreduce-client-common. +1 mapreduce tests 1m 36s Tests passed in hadoop-mapreduce-client-core. -1 mapreduce tests 104m 37s Tests failed in hadoop-mapreduce-client-jobclient.     157m 25s   Reason Tests Failed unit tests hadoop.mapred.TestLocalJobSubmission Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12732403/MAPREDUCE-5951-trunk-v10.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / f24452d checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/diffcheckstylehadoop-mapreduce-client-core.txt whitespace https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/whitespace.txt hadoop-mapreduce-client-app test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-app.txt hadoop-mapreduce-client-common test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-common.txt hadoop-mapreduce-client-core test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-core.txt hadoop-mapreduce-client-jobclient test log https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/artifact/patchprocess/testrun_hadoop-mapreduce-client-jobclient.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5718/testReport/ Java 1.7.0_55 uname Linux asf906.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-MAPREDUCE-Build/5718/console This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          V11 attached. Addressed all comments from Karthik Kambatla. Filed MAPREDUCE-6365 to refactor JobResourceUploader#uploadFilesInternal.

          Show
          ctrezzo Chris Trezzo added a comment - V11 attached. Addressed all comments from Karthik Kambatla . Filed MAPREDUCE-6365 to refactor JobResourceUploader#uploadFilesInternal.
          Hide
          kasha Karthik Kambatla added a comment -

          Thanks Chris.

          Comments on the latest patch:

          1. DistributedCache changes are all spurious. Omit it from the diff.
          2. Remove Job#addArchiveToSharedCacheAndClasspath altogether?
          3. JobImpl#cleanupSharedCacheResources should be always called when the job finishes, irrespective of success.
          4. JobResourceUploader
            1. stopSharedCache should set scClient to null
            2. We don't need a separate boolean to check if SCM is available. We should be able to just use (sClient != null). If we run into any issues talking to SCM, we should just abort and call stopSharedCache to avoid using it for the rest of the dependencies.
            3. uploadFiles seems to be trying to handle the case where shared-cache goes down after we set all the files that need to be uploaded. We should leave this check to the NM. The SCM might come back up by the time the NM tries uploading files. Or, it could be available at this point and go down later.
                    // if scm fails in the middle, we will set shared cache upload policies
                    // for all resources
                    // to be false. The resources that are shared successfully via
                    // SharedCacheClient.use will
                    // continued to be shared.
                    if (scClient != null && !isScmAvailable()) {
              
            4. uploadFiles checks if shared-cache is available before setting the upload policies. I think we should set the upload policies irrespective of SCM's availability when shared-cache is not disabled. Also, we should modify the below code to have the second if inside the first if.
                        if (isSharedCacheFilesEnabled()) {
                          newPath = useSharedCache(tmp, conf);
                        }
              
                        // need to inform NM to upload the file to shared cache.
                        if (newPath == null && isSharedCacheFilesEnabled()) {
                          filesSCUploadPolicies[indexOfFilesSCUploadPolicies] = true;
                        }
              
            5. useSharedCache should check for (scClient != null)
            6. uploadFiles has a lot of duplication. Can we file a follow-up JIRA to simplify it?
            7. Also, due to the try-finally in uploadFiles, it is kind of hard to see what lines have been changed. Do you think it makes any sense to wrap this in another method call, so the indentations don't show spurious changes? I am assuming most of this code will be touched by the follow-up JIRA.
            8. useSharedCache javadoc issue
          5. MRApps - we are using "*" instead of "job.jar" which should work, but I wonder if that will be an incompatible behavior change. Jason Lowe, Vinod Kumar Vavilapalli - what do you think?
          6. YarnRunner - indentation issue on line 360
          Show
          kasha Karthik Kambatla added a comment - Thanks Chris. Comments on the latest patch: DistributedCache changes are all spurious. Omit it from the diff. Remove Job#addArchiveToSharedCacheAndClasspath altogether? JobImpl#cleanupSharedCacheResources should be always called when the job finishes, irrespective of success. JobResourceUploader stopSharedCache should set scClient to null We don't need a separate boolean to check if SCM is available. We should be able to just use (sClient != null). If we run into any issues talking to SCM, we should just abort and call stopSharedCache to avoid using it for the rest of the dependencies. uploadFiles seems to be trying to handle the case where shared-cache goes down after we set all the files that need to be uploaded. We should leave this check to the NM. The SCM might come back up by the time the NM tries uploading files. Or, it could be available at this point and go down later. // if scm fails in the middle, we will set shared cache upload policies // for all resources // to be false . The resources that are shared successfully via // SharedCacheClient.use will // continued to be shared. if (scClient != null && !isScmAvailable()) { uploadFiles checks if shared-cache is available before setting the upload policies. I think we should set the upload policies irrespective of SCM's availability when shared-cache is not disabled. Also, we should modify the below code to have the second if inside the first if. if (isSharedCacheFilesEnabled()) { newPath = useSharedCache(tmp, conf); } // need to inform NM to upload the file to shared cache. if (newPath == null && isSharedCacheFilesEnabled()) { filesSCUploadPolicies[indexOfFilesSCUploadPolicies] = true ; } useSharedCache should check for (scClient != null) uploadFiles has a lot of duplication. Can we file a follow-up JIRA to simplify it? Also, due to the try-finally in uploadFiles, it is kind of hard to see what lines have been changed. Do you think it makes any sense to wrap this in another method call, so the indentations don't show spurious changes? I am assuming most of this code will be touched by the follow-up JIRA. useSharedCache javadoc issue MRApps - we are using "*" instead of "job.jar" which should work, but I wonder if that will be an incompatible behavior change. Jason Lowe , Vinod Kumar Vavilapalli - what do you think? YarnRunner - indentation issue on line 360
          Hide
          ctrezzo Chris Trezzo added a comment -

          Submit patch for qa run.

          Show
          ctrezzo Chris Trezzo added a comment - Submit patch for qa run.
          Hide
          ctrezzo Chris Trezzo added a comment -

          V10 Attached.

          1. This patch takes out support for symlinking of two cached resources with the same name. Karthik Kambatla and I chatted offline and this seems like something complex enough that it should be handled for all shared cache clients at the YARN layer. Note: Because of this change, this patch will currently not handle resources correctly in certain shared cache scenarios. Please see YARN-3637 for more context.

          JobImpl - is the cleanup of upload-policies intended to be in init-transition? Is that because we don't need the policies once we are done uploading the resources?

          2. The cleanup of upload-policies happens in the Application Master init-transition to prevent all node managers running tasks for this job from attempting to upload resources to the shared cache. Since all containers in the MapReduce application localize the same resources, we decided that the Application Master is the only container that needs to upload resources. Maybe this needs more fool proofing to prevent redundant resource upload attempts?

          3. The order of isScmAvailable() check in isSharedCacheFilesEnabled and co. was changed.

          4. Renamed getFiles to mergeLocalAndCacheResources and made it static.

          Show
          ctrezzo Chris Trezzo added a comment - V10 Attached. 1. This patch takes out support for symlinking of two cached resources with the same name. Karthik Kambatla and I chatted offline and this seems like something complex enough that it should be handled for all shared cache clients at the YARN layer. Note: Because of this change, this patch will currently not handle resources correctly in certain shared cache scenarios. Please see YARN-3637 for more context. JobImpl - is the cleanup of upload-policies intended to be in init-transition? Is that because we don't need the policies once we are done uploading the resources? 2. The cleanup of upload-policies happens in the Application Master init-transition to prevent all node managers running tasks for this job from attempting to upload resources to the shared cache. Since all containers in the MapReduce application localize the same resources, we decided that the Application Master is the only container that needs to upload resources. Maybe this needs more fool proofing to prevent redundant resource upload attempts? 3. The order of isScmAvailable() check in isSharedCacheFilesEnabled and co. was changed. 4. Renamed getFiles to mergeLocalAndCacheResources and made it static.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Added YARN-3637 to address sym-linking of resources at the YARN layer. Will remove sym-linking from this patch.

          Show
          ctrezzo Chris Trezzo added a comment - Added YARN-3637 to address sym-linking of resources at the YARN layer. Will remove sym-linking from this patch.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Rebase.

          Show
          ctrezzo Chris Trezzo added a comment - Rebase.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          -1 patch 0m 0s The patch command could not apply the patch during dryrun.



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12705266/MAPREDUCE-5951-trunk-v8.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / fe0df59
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5716/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment -1 patch 0m 0s The patch command could not apply the patch during dryrun. Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12705266/MAPREDUCE-5951-trunk-v8.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / fe0df59 Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5716/console This message was automatically generated.
          Hide
          kasha Karthik Kambatla added a comment -

          Sorry again for the delay here. Haven't been able to get a large enough chunk of time to review this.

          Few comments based on the partial review I have been able to get to so far. Will post comments for the remaining review as soon as I can:

          1. DistributedCache#getPathStringWithoutFragment: Is this to make sure we get the path to the file and not its fields? Can we add comments to describe what the method is intended to do? And, may be a different name? Can't think of a simpler one myself.
          2. Job - have some javadoc suggestions, but may be it is simpler to post an updated patch once the patch is ready.
          3. JobImpl - is the cleanup of upload-policies intended to be in init-transition? Is that because we don't need the policies once we are done uploading the resources?
          4. JobResourceUploader
            1. In isSharedCacheFilesEnabled and co., we should reverse the order of checks. isScmAvailable() should come first as it is cheaper. Also, don't need the outer parentheses there.
            2. Rename getFiles to mergeLocalAndCacheResources and make it static?
          Show
          kasha Karthik Kambatla added a comment - Sorry again for the delay here. Haven't been able to get a large enough chunk of time to review this. Few comments based on the partial review I have been able to get to so far. Will post comments for the remaining review as soon as I can: DistributedCache#getPathStringWithoutFragment: Is this to make sure we get the path to the file and not its fields? Can we add comments to describe what the method is intended to do? And, may be a different name? Can't think of a simpler one myself. Job - have some javadoc suggestions, but may be it is simpler to post an updated patch once the patch is ready. JobImpl - is the cleanup of upload-policies intended to be in init-transition? Is that because we don't need the policies once we are done uploading the resources? JobResourceUploader In isSharedCacheFilesEnabled and co., we should reverse the order of checks. isScmAvailable() should come first as it is cheaper. Also, don't need the outer parentheses there. Rename getFiles to mergeLocalAndCacheResources and make it static?
          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/12705266/MAPREDUCE-5951-trunk-v8.patch
          against trunk revision 3bc72cc.

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

          +1 tests included. The patch appears to include 4 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient.

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5304//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5304//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/12705266/MAPREDUCE-5951-trunk-v8.patch against trunk revision 3bc72cc. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5304//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5304//console This message is automatically generated.
          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/12705251/MAPREDUCE-5951-trunk-v7.patch
          against trunk revision fc90bf7.

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

          +1 tests included. The patch appears to include 4 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient:

          org.apache.hadoop.mapreduce.TestJobResourceUploader

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5303//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5303//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/12705251/MAPREDUCE-5951-trunk-v7.patch against trunk revision fc90bf7. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient: org.apache.hadoop.mapreduce.TestJobResourceUploader Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5303//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5303//console This message is automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Karthik Kambatla
          Attached is v8 of the patch. This update did two things:
          1. Added the DistributedCache changes back in. On second thought, they are necessary for this patch. The key is the DistributedCache#getPathStringWithoutFragment method. This method is added and used during the construction of the classpath, so that the symlinking during localization of resources works correctly in the case when there are two different resources with the same name. For example, checksum1/job.jar, checksum2/job.jar. If both of these resources are used as libjars in a single job, there will be a naming conflict and one of the resource's container symlink (on the node manager) will get overridden by the other. We use the fragment portion of the URI to control the symlink used by yarn localization. With this, we ensure to avoid naming conflicts at the container level (users can name their jars however they want) and we leverage shared jars coming from the cache.
          2. Fixed TestJobResourceUploader unit tests to accommodate using ConverterUtils#toApplicationId.

          Show
          ctrezzo Chris Trezzo added a comment - Karthik Kambatla Attached is v8 of the patch. This update did two things: 1. Added the DistributedCache changes back in. On second thought, they are necessary for this patch. The key is the DistributedCache#getPathStringWithoutFragment method. This method is added and used during the construction of the classpath, so that the symlinking during localization of resources works correctly in the case when there are two different resources with the same name. For example, checksum1/job.jar, checksum2/job.jar. If both of these resources are used as libjars in a single job, there will be a naming conflict and one of the resource's container symlink (on the node manager) will get overridden by the other. We use the fragment portion of the URI to control the symlink used by yarn localization. With this, we ensure to avoid naming conflicts at the container level (users can name their jars however they want) and we leverage shared jars coming from the cache. 2. Fixed TestJobResourceUploader unit tests to accommodate using ConverterUtils#toApplicationId.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Karthik Kambatla Thanks again for the comments!

          Attached is v7 of the patch. This version is rebased and addresses your comments above. I removed the DistributedCache changes, addressed comments about Job, JobID, JobImpl. With respect to comment 5.2, the patch is not hard coding MR job submission to always use SharedCache. See if the new patch improves clarity around that and let me know if you have more questions. There are two changes that will happen even if the shared cache is disabled:
          1. The SharedCacheConfig class will be used to parse configuration in JobResourceUploader. If the shared cache config parameters do not exist, then it is a no-op.
          2. The MR classpath around job jars has be changed slightly (that is the reason for the MRApps and TestMRApps changes), but should present no behavioral changes to the user. This is to handle the case where the job jar used by a job comes from the shared cache and it is named anything other than job.jar. Note that the current code assumes that whatever is localized in the job.jar directory is a single file named job.jar (i.e. job.jar/job.jar in the classpath). In the case where the job.jar is named something else, it will not get put on the classpath. This change simply puts everything in the job.jar directory (currently only the job jar) on the classpath (i.e. job.jar/*).

          With respect to the comment about fool-proof config: did you have anything specific in mind? Currently the config should only recognize disabled, enabled, jobjar, libjars, files, archives. I could split each into a separate boolean config parameter if that seems more safe? Let me know. I was trying to come up with a concise single parameter for all the modes, but maybe splitting them up into separate boolean parameters is better. I can also see the JOBJAR_VISIBILITY parameter being slightly confusing and will think if there is a better way to do that. Again, let me know if you have suggestions.

          Also, let me know if you want me to split this patch further. I could see splitting it into the following (although the splits won't be fully functional):
          1. JobResourceUploader changes. The diff is still a little wonky with the code restructure from adding shared cache checks.
          2. TaskImpl changes.
          3. JobImpl changes.
          4. job.jar classpath changes

          Show
          ctrezzo Chris Trezzo added a comment - Karthik Kambatla Thanks again for the comments! Attached is v7 of the patch. This version is rebased and addresses your comments above. I removed the DistributedCache changes, addressed comments about Job, JobID, JobImpl. With respect to comment 5.2, the patch is not hard coding MR job submission to always use SharedCache. See if the new patch improves clarity around that and let me know if you have more questions. There are two changes that will happen even if the shared cache is disabled: 1. The SharedCacheConfig class will be used to parse configuration in JobResourceUploader. If the shared cache config parameters do not exist, then it is a no-op. 2. The MR classpath around job jars has be changed slightly (that is the reason for the MRApps and TestMRApps changes), but should present no behavioral changes to the user. This is to handle the case where the job jar used by a job comes from the shared cache and it is named anything other than job.jar. Note that the current code assumes that whatever is localized in the job.jar directory is a single file named job.jar (i.e. job.jar/job.jar in the classpath). In the case where the job.jar is named something else, it will not get put on the classpath. This change simply puts everything in the job.jar directory (currently only the job jar) on the classpath (i.e. job.jar/*). With respect to the comment about fool-proof config: did you have anything specific in mind? Currently the config should only recognize disabled, enabled, jobjar, libjars, files, archives. I could split each into a separate boolean config parameter if that seems more safe? Let me know. I was trying to come up with a concise single parameter for all the modes, but maybe splitting them up into separate boolean parameters is better. I can also see the JOBJAR_VISIBILITY parameter being slightly confusing and will think if there is a better way to do that. Again, let me know if you have suggestions. Also, let me know if you want me to split this patch further. I could see splitting it into the following (although the splits won't be fully functional): 1. JobResourceUploader changes. The diff is still a little wonky with the code restructure from adding shared cache checks. 2. TaskImpl changes. 3. JobImpl changes. 4. job.jar classpath changes
          Hide
          ctrezzo Chris Trezzo added a comment -

          Created MAPREDUCE-6267 to address comment in 5.1.

          Show
          ctrezzo Chris Trezzo added a comment - Created MAPREDUCE-6267 to address comment in 5.1.
          Hide
          kasha Karthik Kambatla added a comment -

          Sorry for the delay in getting to this. Getting a continuous chunk of time to look at this somewhat large patch was hard.

          Here are my first round of comments - a combination of high-level and detailed comments. Let us see if we can get some of this in through other JIRAs first, to allow for a more thorough review.

          1. DistributedCache changes aren’t central to what this JIRA is trying to address. Could we leave them out and address in another JIRA?
            1. This has nothing to do with this patch, but it would be nice to make the code around setting CLASSPATH_FILES a little more readable. Could we define another String prefix to hold “” or classpath, based on whether classpath is null.
          2. Job
            1. The new APIs should all be @Unstable
            2. Let us make the javadoc for the new APIs a little more formal - we don’t need to mention SCMClientProtocol.use, or that the APIs are intended for user use. Even for the return value, I would go with something like “If shared cache is enabled and the resource added successfully, return true. Otherwise, return false.”
            3. How about renaming the methods to addFileToSharedCache, addArchiveToSharedCache, addFileToSharedCacheAndClasspath?
            4. Make both new methods private static instead of static private.
          3. JobID changes might not be required. Use ConverterUtils#toApplicationId?
          4. JobImpl
            1. cleanupSharedCacheResources - nit: I would check for (checksums == null || checksums.length == 0) and return to save on indentations. 80 chars is already too small.
            2. cleanupSharedCacheUploadPolicies - javadoc should use block comments. Well, may be a nit.
          5. JobSubmitter
            1. Can we do the code moving from JobSumitter to FileUploader (may be, we need a more descriptive name) to another JIRA and look at that first if needed. Otherwise, it is hard to review the changes.
            2. May be, I am misreading the patch. Is this patch hardcoding MR job submission to always use SharedCache? If yes, we should definitely avoid that.
          6. mapred-default.xml: We need a little more fool-proof config. The way the patch currently is, a typo will lead to unexpected behavior without any warnings.
          Show
          kasha Karthik Kambatla added a comment - Sorry for the delay in getting to this. Getting a continuous chunk of time to look at this somewhat large patch was hard. Here are my first round of comments - a combination of high-level and detailed comments. Let us see if we can get some of this in through other JIRAs first, to allow for a more thorough review. DistributedCache changes aren’t central to what this JIRA is trying to address. Could we leave them out and address in another JIRA? This has nothing to do with this patch, but it would be nice to make the code around setting CLASSPATH_FILES a little more readable. Could we define another String prefix to hold “” or classpath, based on whether classpath is null. Job The new APIs should all be @Unstable Let us make the javadoc for the new APIs a little more formal - we don’t need to mention SCMClientProtocol.use, or that the APIs are intended for user use. Even for the return value, I would go with something like “If shared cache is enabled and the resource added successfully, return true. Otherwise, return false.” How about renaming the methods to addFileToSharedCache, addArchiveToSharedCache, addFileToSharedCacheAndClasspath? Make both new methods private static instead of static private. JobID changes might not be required. Use ConverterUtils#toApplicationId? JobImpl cleanupSharedCacheResources - nit: I would check for (checksums == null || checksums.length == 0) and return to save on indentations. 80 chars is already too small. cleanupSharedCacheUploadPolicies - javadoc should use block comments. Well, may be a nit. JobSubmitter Can we do the code moving from JobSumitter to FileUploader (may be, we need a more descriptive name) to another JIRA and look at that first if needed. Otherwise, it is hard to review the changes. May be, I am misreading the patch. Is this patch hardcoding MR job submission to always use SharedCache? If yes, we should definitely avoid that. mapred-default.xml: We need a little more fool-proof config. The way the patch currently is, a typo will lead to unexpected behavior without any warnings.
          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/12693397/MAPREDUCE-5951-trunk-v6.patch
          against trunk revision dd0228b.

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

          +1 tests included. The patch appears to include 4 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 appears to introduce 13 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient.

          Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5112//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5112//artifact/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5112//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/12693397/MAPREDUCE-5951-trunk-v6.patch against trunk revision dd0228b. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 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 appears to introduce 13 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-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5112//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5112//artifact/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/5112//console This message is automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Karthik Kambatla V6 attached. This patch should be ready for review!

          Show
          ctrezzo Chris Trezzo added a comment - Karthik Kambatla V6 attached. This patch should be ready for review!
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attached v5. Updated to match patch in YARN-1492.

          Show
          ctrezzo Chris Trezzo added a comment - Attached v5. Updated to match patch in YARN-1492 .
          Hide
          ctrezzo Chris Trezzo added a comment -

          Rebase again.

          Show
          ctrezzo Chris Trezzo added a comment - Rebase again.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Note: #2 is only in the case where shared cache is enabled. There is still no behavior change when the shared cache is disabled.

          Show
          ctrezzo Chris Trezzo added a comment - Note: #2 is only in the case where shared cache is enabled. There is still no behavior change when the shared cache is disabled.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Rebase. Also added two fixes done by Ming Ma:
          1. Fix bug in the way the shared cache upload policies are set.
          2. Handle local resource symlinks correctly when two distinct resources have the same filename.

          Show
          ctrezzo Chris Trezzo added a comment - Rebase. Also added two fixes done by Ming Ma : 1. Fix bug in the way the shared cache upload policies are set. 2. Handle local resource symlinks correctly when two distinct resources have the same filename.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attached is a v2 patch. This fixes a bug around uploading the jobjar.

          Show
          ctrezzo Chris Trezzo added a comment - Attached is a v2 patch. This fixes a bug around uploading the jobjar.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Attached is a v1 patch based on trunk+YARN-1492 and all of it's subtasks.

          Show
          ctrezzo Chris Trezzo added a comment - Attached is a v1 patch based on trunk+ YARN-1492 and all of it's subtasks.

            People

            • Assignee:
              ctrezzo Chris Trezzo
              Reporter:
              ctrezzo Chris Trezzo
            • Votes:
              0 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development