Uploaded image for project: 'Hadoop Map/Reduce'
  1. Hadoop Map/Reduce
  2. MAPREDUCE-6365

Refactor JobResourceUploader#uploadFilesInternal

    Details

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

      Description

      JobResourceUploader#uploadFilesInternal is a large method and there are similar pieces of code that could probably be pulled out into separate methods. This refactor would improve readability of the code.

        Issue Links

          Activity

          Hide
          nijel nijel added a comment -

          I would like to work on this issue.
          Please feel free to re assign if you started the work

          Show
          nijel nijel added a comment - I would like to work on this issue. Please feel free to re assign if you started the work
          Hide
          ctrezzo Chris Trezzo added a comment -

          Hey nijel. I have already started the work as part of MAPREDUCE-5951. Thanks!

          Show
          ctrezzo Chris Trezzo added a comment - Hey nijel . I have already started the work as part of MAPREDUCE-5951 . Thanks!
          Hide
          ctrezzo Chris Trezzo added a comment -

          v1 patch attached. /cc Sangjin Lee

          This is a simple patch that does the following:

          1. Separates files, libjars, archives, jobjar uploading logic into separate methods instead of one big method. This will make the code more readable in the future, especially when adding shared cache support.
          2. Rename the JobResourceUploader#uploadFiles method to uploadResources. This more appropriately represents what the method does and avoids a redundant method name when creating the new upload methods.
          Show
          ctrezzo Chris Trezzo added a comment - v1 patch attached. /cc Sangjin Lee This is a simple patch that does the following: Separates files, libjars, archives, jobjar uploading logic into separate methods instead of one big method. This will make the code more readable in the future, especially when adding shared cache support. Rename the JobResourceUploader#uploadFiles method to uploadResources. This more appropriately represents what the method does and avoids a redundant method name when creating the new upload methods.
          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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 7m 45s trunk passed
          +1 compile 0m 28s trunk passed
          +1 checkstyle 0m 19s trunk passed
          +1 mvnsite 0m 32s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 59s trunk passed
          +1 javadoc 0m 24s trunk passed
          +1 mvninstall 0m 25s the patch passed
          +1 compile 0m 25s the patch passed
          +1 javac 0m 25s the patch passed
          +1 checkstyle 0m 14s the patch passed
          +1 mvnsite 0m 29s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 10s the patch passed
          +1 javadoc 0m 23s the patch passed
          -1 unit 2m 18s hadoop-mapreduce-client-core in the patch failed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          17m 36s



          Reason Tests
          Failed junit tests hadoop.mapreduce.tools.TestCLI



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12816917/MAPREDUCE-6365-trunk-v1.patch
          JIRA Issue MAPREDUCE-6365
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux f1690bf8dbb0 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 932aed6
          Default Java 1.8.0_91
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/artifact/patchprocess/patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt
          unit test logs https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/artifact/patchprocess/patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core U: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 7m 45s trunk passed +1 compile 0m 28s trunk passed +1 checkstyle 0m 19s trunk passed +1 mvnsite 0m 32s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 59s trunk passed +1 javadoc 0m 24s trunk passed +1 mvninstall 0m 25s the patch passed +1 compile 0m 25s the patch passed +1 javac 0m 25s the patch passed +1 checkstyle 0m 14s the patch passed +1 mvnsite 0m 29s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 10s the patch passed +1 javadoc 0m 23s the patch passed -1 unit 2m 18s hadoop-mapreduce-client-core in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 17m 36s Reason Tests Failed junit tests hadoop.mapreduce.tools.TestCLI Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12816917/MAPREDUCE-6365-trunk-v1.patch JIRA Issue MAPREDUCE-6365 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f1690bf8dbb0 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 932aed6 Default Java 1.8.0_91 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/artifact/patchprocess/patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt unit test logs https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/artifact/patchprocess/patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core U: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6604/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          ctrezzo Chris Trezzo added a comment -

          Sangjin Lee The patch should be good to go and ready for review.

          The two minus ones are accounted for:

          1. The patch does not include unit tests because it is purely a refactor with no functional changes. The existing unit tests pass.
          2. There was one failed test (TestCLI#testGetJob), but that is a known flapping test and already has a jira accounting for it (MAPREDUCE-6625). I ran the test locally and it passed.
          Show
          ctrezzo Chris Trezzo added a comment - Sangjin Lee The patch should be good to go and ready for review. The two minus ones are accounted for: The patch does not include unit tests because it is purely a refactor with no functional changes. The existing unit tests pass. There was one failed test (TestCLI#testGetJob), but that is a known flapping test and already has a jira accounting for it ( MAPREDUCE-6625 ). I ran the test locally and it passed.
          Hide
          sjlee0 Sangjin Lee added a comment -

          +1. Will commit soon.

          Show
          sjlee0 Sangjin Lee added a comment - +1. Will commit soon.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Committed the patch to trunk. Thanks Chris Trezzo for your contribution! Should this be backported to an earlier version? Branch-2 (2.9.0)?

          Show
          sjlee0 Sangjin Lee added a comment - Committed the patch to trunk. Thanks Chris Trezzo for your contribution! Should this be backported to an earlier version? Branch-2 (2.9.0)?
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #10121 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10121/)
          MAPREDUCE-6365. Refactor JobResourceUploader#uploadFilesInternal (Chris (sjlee: rev 8f0d3d69d65a252439610e6f13d679808d768569)

          • hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/JobSubmitter.java
          • hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/JobResourceUploader.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #10121 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10121/ ) MAPREDUCE-6365 . Refactor JobResourceUploader#uploadFilesInternal (Chris (sjlee: rev 8f0d3d69d65a252439610e6f13d679808d768569) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/JobSubmitter.java hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/JobResourceUploader.java
          Hide
          ctrezzo Chris Trezzo added a comment -

          Thanks Sangjin Lee! The intention would be to get MapReduce support for Shared cache in 2.9.0, so backporting this refactor to 2.9.0 sounds great.

          Show
          ctrezzo Chris Trezzo added a comment - Thanks Sangjin Lee ! The intention would be to get MapReduce support for Shared cache in 2.9.0, so backporting this refactor to 2.9.0 sounds great.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development