Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-10048

LocalDirAllocator should avoid holding locks while accessing the filesystem

    Details

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

      Description

      As noted in MAPREDUCE-5584 and HADOOP-7016, LocalDirAllocator can be a bottleneck for multithreaded setups like the ShuffleHandler. We should consider moving to a lockless design or minimizing the critical sections to a very small amount of time that does not involve I/O operations.

      1. HADOOP-10048.003.patch
        11 kB
        Yi Li
      2. HADOOP-10048.004.patch
        12 kB
        Jason Lowe
      3. HADOOP-10048.005.patch
        12 kB
        Jason Lowe
      4. HADOOP-10048.006.patch
        13 kB
        Jason Lowe
      5. HADOOP-10048.patch
        11 kB
        Jason Lowe
      6. HADOOP-10048.trunk.patch
        11 kB
        Yi Li

        Issue Links

          Activity

          Hide
          jlowe Jason Lowe added a comment -

          Here's a patch that uses a lockless approach to the problem. The variables that were being synchronized were treated read-only, except for the dirNumLastAccessed field. Therefore the patch wraps the relevant variables in a context object and when the conf is updated the context reference is atomically swapped out for a new context object. Each method needing to access the context makes a local copy of the reference and always uses the context through that local reference, so they are always looking at a self-consistent context. The tradeoff is that the context itself may be stale, but it should always be self-consistent. The dirNumLastAccessed field is treated in a similar fashion. It is read once into a local variable then the local variable is used for iteration purposes.

          Since this is primarily a performance optimization the patch also changes the local directories to be stored as Path objects rather than Strings so we don't need to create so many Paths from scratch during methods.

          Ran some performance numbers on multithreaded accesses with this change and was a bit surprised to see it was significantly slower than the old version. However that's because the change in HADOOP-9652 causes fs.exists() to fork-and-exec the stat command, and before this change that was done serially across threads. With this change it effectively becomes a mini fork-bomb, forking stat in parallel like crazy. When I reverted the HADOOP-9652 change locally, the patched version was about 2.5x faster than the original version with 8 threads across 12 local directories.

          The speedup is nice, but I'm not sure this needs to be a Blocker for the 2.3.0 release. If the filesystem is the real bottleneck (e.g.: accessing one of the drives is always really, really slow or using fork-and-exec stat to do an fs.exists) then this change will only marginally help in most use cases. Eventually all of the serving threads are going to be hung up waiting for the filesystem which is only a bit better (or sometimes worse as in the fork-and-exec case) than waiting serially. The real throughput of a serve using the allocator may not be significantly improved.

          Show
          jlowe Jason Lowe added a comment - Here's a patch that uses a lockless approach to the problem. The variables that were being synchronized were treated read-only, except for the dirNumLastAccessed field. Therefore the patch wraps the relevant variables in a context object and when the conf is updated the context reference is atomically swapped out for a new context object. Each method needing to access the context makes a local copy of the reference and always uses the context through that local reference, so they are always looking at a self-consistent context. The tradeoff is that the context itself may be stale, but it should always be self-consistent. The dirNumLastAccessed field is treated in a similar fashion. It is read once into a local variable then the local variable is used for iteration purposes. Since this is primarily a performance optimization the patch also changes the local directories to be stored as Path objects rather than Strings so we don't need to create so many Paths from scratch during methods. Ran some performance numbers on multithreaded accesses with this change and was a bit surprised to see it was significantly slower than the old version. However that's because the change in HADOOP-9652 causes fs.exists() to fork-and-exec the stat command, and before this change that was done serially across threads. With this change it effectively becomes a mini fork-bomb, forking stat in parallel like crazy. When I reverted the HADOOP-9652 change locally, the patched version was about 2.5x faster than the original version with 8 threads across 12 local directories. The speedup is nice, but I'm not sure this needs to be a Blocker for the 2.3.0 release. If the filesystem is the real bottleneck (e.g.: accessing one of the drives is always really, really slow or using fork-and-exec stat to do an fs.exists) then this change will only marginally help in most use cases. Eventually all of the serving threads are going to be hung up waiting for the filesystem which is only a bit better (or sometimes worse as in the fork-and-exec case) than waiting serially. The real throughput of a serve using the allocator may not be significantly improved.
          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/12609909/HADOOP-10048.patch
          against trunk revision .

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

          -1 tests included. 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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3243//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3243//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/12609909/HADOOP-10048.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . 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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) 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-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3243//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3243//console This message is automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          I think this is a would-be-nice feature but not a blocker for 2.4 given it's been this way since at least 0.23.

          Show
          jlowe Jason Lowe added a comment - I think this is a would-be-nice feature but not a blocker for 2.4 given it's been this way since at least 0.23.
          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/12609909/HADOOP-10048.patch
          against trunk revision .

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

          -1 tests included. 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 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 1.3.9) 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-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3661//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3661//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/12609909/HADOOP-10048.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . 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 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 1.3.9) 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-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3661//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3661//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/12609909/HADOOP-10048.patch
          against trunk revision .

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

          -1 tests included. 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 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 1.3.9) warnings.

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

          -1 core tests. The following test timeouts occurred in hadoop-common-project/hadoop-common:

          org.apache.hadoop.http.TestHttpServer

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4202//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4202//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/12609909/HADOOP-10048.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . 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 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 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The following test timeouts occurred in hadoop-common-project/hadoop-common: org.apache.hadoop.http.TestHttpServer +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4202//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4202//console This message is automatically generated.
          Hide
          hongyu.bi hongyu bi added a comment -

          any update about this jira? thanks.
          we suffered a lot from this lock

          Show
          hongyu.bi hongyu bi added a comment - any update about this jira? thanks. we suffered a lot from this lock
          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/12609909/HADOOP-10048.patch
          against trunk revision e996a1b.

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

          -1 tests included. 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 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 3 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-common-project/hadoop-common.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5284//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/5284//artifact/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5284//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/12609909/HADOOP-10048.patch against trunk revision e996a1b. +1 @author . The patch does not contain any @author tags. -1 tests included . 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 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 3 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-common-project/hadoop-common. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5284//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/5284//artifact/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5284//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 38s Pre-patch trunk compilation is healthy.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 tests included 0m 0s The patch 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 javac 7m 31s There were no new javac warning messages.
          +1 javadoc 9m 35s 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 4s The applied patch generated 5 new checkstyle issues (total was 23, now 22).
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 33s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 1m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          +1 common tests 22m 24s Tests passed in hadoop-common.
              59m 23s  



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12609909/HADOOP-10048.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / a319771
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/artifact/patchprocess/diffcheckstylehadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/artifact/patchprocess/whitespace.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/testReport/
          Java 1.7.0_55
          uname Linux asf907.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-HADOOP-Build/6455/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 38s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch 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 javac 7m 31s There were no new javac warning messages. +1 javadoc 9m 35s 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 4s The applied patch generated 5 new checkstyle issues (total was 23, now 22). -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 33s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 common tests 22m 24s Tests passed in hadoop-common.     59m 23s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12609909/HADOOP-10048.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / a319771 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/artifact/patchprocess/diffcheckstylehadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/artifact/patchprocess/whitespace.txt hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6455/testReport/ Java 1.7.0_55 uname Linux asf907.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-HADOOP-Build/6455/console This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -



          -1 overall



          Vote Subsystem Runtime Comment
          0 pre-patch 14m 36s 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 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 javac 7m 31s There were no new javac warning messages.
          +1 javadoc 9m 35s There were no new javadoc warning messages.
          +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
          -1 checkstyle 1m 5s The applied patch generated 5 new checkstyle issues (total was 23, now 22).
          -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 install 1m 32s mvn install still works.
          +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
          +1 findbugs 1m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
          -1 common tests 21m 37s Tests failed in hadoop-common.
              58m 35s  



          Reason Tests
          Timed out tests org.apache.hadoop.security.token.delegation.TestZKDelegationTokenSecretManager



          Subsystem Report/Notes
          Patch URL http://issues.apache.org/jira/secure/attachment/12609909/HADOOP-10048.patch
          Optional Tests javadoc javac unit findbugs checkstyle
          git revision trunk / a319771
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/artifact/patchprocess/diffcheckstylehadoop-common.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/artifact/patchprocess/whitespace.txt
          hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/artifact/patchprocess/testrun_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/testReport/
          Java 1.7.0_55
          uname Linux asf907.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-HADOOP-Build/6460/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 36s 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 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 javac 7m 31s There were no new javac warning messages. +1 javadoc 9m 35s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 1m 5s The applied patch generated 5 new checkstyle issues (total was 23, now 22). -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 32s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 38s The patch does not introduce any new Findbugs (version 2.0.3) warnings. -1 common tests 21m 37s Tests failed in hadoop-common.     58m 35s   Reason Tests Timed out tests org.apache.hadoop.security.token.delegation.TestZKDelegationTokenSecretManager Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12609909/HADOOP-10048.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / a319771 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/artifact/patchprocess/diffcheckstylehadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/artifact/patchprocess/whitespace.txt hadoop-common test log https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/artifact/patchprocess/testrun_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/6460/testReport/ Java 1.7.0_55 uname Linux asf907.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-HADOOP-Build/6460/console This message was automatically generated.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Moving features/enhancements out of previously closed releases into the next minor release 2.8.0.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Moving features/enhancements out of previously closed releases into the next minor release 2.8.0.
          Hide
          drearycold Yi Li added a comment -

          Hi all, we have integrated this patch into our internal build (based on hadoop-2.5-cdh5.3.2) and deployed to two clusters (tens/hundreds of nodes each) for more than a month.So far there's no abnormal behavior, no job will be stuck in SHUFFLE phase (back then a single fetch attempt could hang for several hours under heavy load), and we have observed that the throughput of ShuffleHandler can be 10x higher than before (based on ShuffleMetrics).
          Is the patch good to go? Thanks.

          Show
          drearycold Yi Li added a comment - Hi all, we have integrated this patch into our internal build (based on hadoop-2.5-cdh5.3.2) and deployed to two clusters (tens/hundreds of nodes each) for more than a month.So far there's no abnormal behavior, no job will be stuck in SHUFFLE phase (back then a single fetch attempt could hang for several hours under heavy load), and we have observed that the throughput of ShuffleHandler can be 10x higher than before (based on ShuffleMetrics). Is the patch good to go? Thanks.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 patch 0m 4s HADOOP-10048 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



          Subsystem Report/Notes
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12609909/HADOOP-10048.patch
          JIRA Issue HADOOP-10048
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/8844/console
          Powered by Apache Yetus 0.2.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 0s Docker mode activated. -1 patch 0m 4s HADOOP-10048 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12609909/HADOOP-10048.patch JIRA Issue HADOOP-10048 Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/8844/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          drearycold Yi Li added a comment -

          rebase onto trunk

          Show
          drearycold Yi Li added a comment - rebase onto trunk
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s 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 6m 28s trunk passed
          +1 compile 5m 39s trunk passed with JDK v1.8.0_74
          +1 compile 6m 34s trunk passed with JDK v1.7.0_95
          +1 checkstyle 0m 20s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 36s trunk passed
          +1 javadoc 0m 54s trunk passed with JDK v1.8.0_74
          +1 javadoc 1m 4s trunk passed with JDK v1.7.0_95
          -1 mvninstall 0m 21s hadoop-common in the patch failed.
          -1 compile 0m 30s root in the patch failed with JDK v1.8.0_74.
          -1 javac 0m 30s root in the patch failed with JDK v1.8.0_74.
          -1 compile 0m 35s root in the patch failed with JDK v1.7.0_95.
          -1 javac 0m 35s root in the patch failed with JDK v1.7.0_95.
          -1 checkstyle 0m 18s hadoop-common-project/hadoop-common: patch generated 5 new + 17 unchanged - 6 fixed = 22 total (was 23)
          -1 mvnsite 0m 23s hadoop-common in the patch failed.
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          -1 findbugs 0m 15s hadoop-common in the patch failed.
          +1 javadoc 0m 48s the patch passed with JDK v1.8.0_74
          +1 javadoc 1m 2s the patch passed with JDK v1.7.0_95
          -1 unit 0m 21s hadoop-common in the patch failed with JDK v1.8.0_74.
          -1 unit 0m 22s hadoop-common in the patch failed with JDK v1.7.0_95.
          +1 asflicense 0m 17s Patch does not generate ASF License warnings.
          30m 8s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ca8df7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12793278/HADOOP-10048.trunk.patch
          JIRA Issue HADOOP-10048
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 37f7c3b8663f 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 / 658ee95
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          mvninstall https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt
          compile https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.8.0_74.txt
          javac https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.8.0_74.txt
          compile https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.7.0_95.txt
          javac https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.7.0_95.txt
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          mvnsite https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/console
          Powered by Apache Yetus 0.2.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 10s 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 6m 28s trunk passed +1 compile 5m 39s trunk passed with JDK v1.8.0_74 +1 compile 6m 34s trunk passed with JDK v1.7.0_95 +1 checkstyle 0m 20s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 36s trunk passed +1 javadoc 0m 54s trunk passed with JDK v1.8.0_74 +1 javadoc 1m 4s trunk passed with JDK v1.7.0_95 -1 mvninstall 0m 21s hadoop-common in the patch failed. -1 compile 0m 30s root in the patch failed with JDK v1.8.0_74. -1 javac 0m 30s root in the patch failed with JDK v1.8.0_74. -1 compile 0m 35s root in the patch failed with JDK v1.7.0_95. -1 javac 0m 35s root in the patch failed with JDK v1.7.0_95. -1 checkstyle 0m 18s hadoop-common-project/hadoop-common: patch generated 5 new + 17 unchanged - 6 fixed = 22 total (was 23) -1 mvnsite 0m 23s hadoop-common in the patch failed. +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. -1 findbugs 0m 15s hadoop-common in the patch failed. +1 javadoc 0m 48s the patch passed with JDK v1.8.0_74 +1 javadoc 1m 2s the patch passed with JDK v1.7.0_95 -1 unit 0m 21s hadoop-common in the patch failed with JDK v1.8.0_74. -1 unit 0m 22s hadoop-common in the patch failed with JDK v1.7.0_95. +1 asflicense 0m 17s Patch does not generate ASF License warnings. 30m 8s Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12793278/HADOOP-10048.trunk.patch JIRA Issue HADOOP-10048 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 37f7c3b8663f 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 / 658ee95 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 mvninstall https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt compile https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.8.0_74.txt javac https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.8.0_74.txt compile https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.7.0_95.txt javac https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-compile-root-jdk1.7.0_95.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt mvnsite https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_95.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/8845/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          drearycold Yi Li added a comment -

          fix a type, sorry for the inconvenience

          Show
          drearycold Yi Li added a comment - fix a type, sorry for the inconvenience
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s 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 6m 28s trunk passed
          +1 compile 5m 40s trunk passed with JDK v1.8.0_74
          +1 compile 6m 32s trunk passed with JDK v1.7.0_95
          +1 checkstyle 0m 20s trunk passed
          +1 mvnsite 0m 55s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 38s trunk passed
          +1 javadoc 0m 52s trunk passed with JDK v1.8.0_74
          +1 javadoc 1m 4s trunk passed with JDK v1.7.0_95
          +1 mvninstall 0m 40s the patch passed
          +1 compile 5m 54s the patch passed with JDK v1.8.0_74
          +1 javac 5m 54s the patch passed
          +1 compile 6m 59s the patch passed with JDK v1.7.0_95
          +1 javac 6m 59s the patch passed
          -1 checkstyle 0m 21s hadoop-common-project/hadoop-common: patch generated 5 new + 17 unchanged - 6 fixed = 22 total (was 23)
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 1m 56s the patch passed
          +1 javadoc 0m 52s the patch passed with JDK v1.8.0_74
          +1 javadoc 1m 4s the patch passed with JDK v1.7.0_95
          -1 unit 6m 34s hadoop-common in the patch failed with JDK v1.8.0_74.
          +1 unit 7m 17s hadoop-common in the patch passed with JDK v1.7.0_95.
          +1 asflicense 0m 22s Patch does not generate ASF License warnings.
          58m 13s



          Reason Tests
          JDK v1.8.0_74 Failed junit tests hadoop.security.ssl.TestReloadingX509TrustManager



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ca8df7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12793282/HADOOP-10048.003.patch
          JIRA Issue HADOOP-10048
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux b0b66a69f87e 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 / 658ee95
          Default Java 1.7.0_95
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt
          unit test logs https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt
          JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/console
          Powered by Apache Yetus 0.2.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 10s 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 6m 28s trunk passed +1 compile 5m 40s trunk passed with JDK v1.8.0_74 +1 compile 6m 32s trunk passed with JDK v1.7.0_95 +1 checkstyle 0m 20s trunk passed +1 mvnsite 0m 55s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 38s trunk passed +1 javadoc 0m 52s trunk passed with JDK v1.8.0_74 +1 javadoc 1m 4s trunk passed with JDK v1.7.0_95 +1 mvninstall 0m 40s the patch passed +1 compile 5m 54s the patch passed with JDK v1.8.0_74 +1 javac 5m 54s the patch passed +1 compile 6m 59s the patch passed with JDK v1.7.0_95 +1 javac 6m 59s the patch passed -1 checkstyle 0m 21s hadoop-common-project/hadoop-common: patch generated 5 new + 17 unchanged - 6 fixed = 22 total (was 23) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 1m 56s the patch passed +1 javadoc 0m 52s the patch passed with JDK v1.8.0_74 +1 javadoc 1m 4s the patch passed with JDK v1.7.0_95 -1 unit 6m 34s hadoop-common in the patch failed with JDK v1.8.0_74. +1 unit 7m 17s hadoop-common in the patch passed with JDK v1.7.0_95. +1 asflicense 0m 22s Patch does not generate ASF License warnings. 58m 13s Reason Tests JDK v1.8.0_74 Failed junit tests hadoop.security.ssl.TestReloadingX509TrustManager Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12793282/HADOOP-10048.003.patch JIRA Issue HADOOP-10048 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b0b66a69f87e 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 / 658ee95 Default Java 1.7.0_95 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_74 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_95 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt unit test logs https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.8.0_74.txt JDK v1.7.0_95 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/8846/console Powered by Apache Yetus 0.2.0 http://yetus.apache.org This message was automatically generated.
          Hide
          drearycold Yi Li added a comment -

          It seems the failed test is not related with this patch. Any suggestions?

          Show
          drearycold Yi Li added a comment - It seems the failed test is not related with this patch. Any suggestions?
          Hide
          djp Junping Du added a comment -

          The patch is a bit old. Kick off Jenkins test manually again. Will start to review it from there.
          Jason Lowe, this long existing issue is still valid now. Isn't it?

          Show
          djp Junping Du added a comment - The patch is a bit old. Kick off Jenkins test manually again. Will start to review it from there. Jason Lowe , this long existing issue is still valid now. Isn't it?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch 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 8m 31s trunk passed
          +1 compile 8m 30s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 1m 12s trunk passed
          +1 mvneclipse 0m 16s trunk passed
          +1 findbugs 1m 47s trunk passed
          +1 javadoc 1m 2s trunk passed
          +1 mvninstall 0m 52s the patch passed
          +1 compile 8m 29s the patch passed
          +1 javac 8m 29s the patch passed
          -1 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 5 new + 17 unchanged - 6 fixed = 22 total (was 23)
          +1 mvnsite 0m 53s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 30s the patch passed
          +1 javadoc 0m 54s the patch passed
          +1 unit 8m 10s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          44m 30s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12793282/HADOOP-10048.003.patch
          JIRA Issue HADOOP-10048
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 60cafec7c6c7 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 / 42c22f7
          Default Java 1.8.0_91
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9530/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9530/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9530/console
          Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch 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 8m 31s trunk passed +1 compile 8m 30s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 12s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 1m 47s trunk passed +1 javadoc 1m 2s trunk passed +1 mvninstall 0m 52s the patch passed +1 compile 8m 29s the patch passed +1 javac 8m 29s the patch passed -1 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 5 new + 17 unchanged - 6 fixed = 22 total (was 23) +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 30s the patch passed +1 javadoc 0m 54s the patch passed +1 unit 8m 10s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 44m 30s Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12793282/HADOOP-10048.003.patch JIRA Issue HADOOP-10048 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 60cafec7c6c7 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 / 42c22f7 Default Java 1.8.0_91 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9530/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9530/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9530/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          this long existing issue is still valid now. Isn't it?

          Yes, it's still applicable.

          Show
          jlowe Jason Lowe added a comment - this long existing issue is still valid now. Isn't it? Yes, it's still applicable.
          Hide
          jlowe Jason Lowe added a comment -

          Updating the patch to use AtomicReference so it's a little more clear that Context must not be accessed directly multiple times during a method but rather via a cached local reference. I also tried to make checkstyle happier.

          Show
          jlowe Jason Lowe added a comment - Updating the patch to use AtomicReference so it's a little more clear that Context must not be accessed directly multiple times during a method but rather via a cached local reference. I also tried to make checkstyle happier.
          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 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 8m 37s trunk passed
          +1 compile 6m 33s trunk passed
          +1 checkstyle 0m 25s trunk passed
          +1 mvnsite 0m 57s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 25s trunk passed
          +1 javadoc 0m 56s trunk passed
          +1 mvninstall 0m 53s the patch passed
          +1 compile 6m 26s the patch passed
          +1 javac 6m 26s the patch passed
          +1 checkstyle 0m 22s hadoop-common-project/hadoop-common: The patch generated 0 new + 17 unchanged - 6 fixed = 17 total (was 23)
          +1 mvnsite 0m 51s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 22s the patch passed
          +1 javadoc 0m 52s the patch passed
          +1 unit 7m 57s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          39m 21s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805249/HADOOP-10048.004.patch
          JIRA Issue HADOOP-10048
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 5fabcb1af9bb 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 / 757050f
          Default Java 1.8.0_91
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9538/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9538/console
          Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s 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 8m 37s trunk passed +1 compile 6m 33s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 25s trunk passed +1 javadoc 0m 56s trunk passed +1 mvninstall 0m 53s the patch passed +1 compile 6m 26s the patch passed +1 javac 6m 26s the patch passed +1 checkstyle 0m 22s hadoop-common-project/hadoop-common: The patch generated 0 new + 17 unchanged - 6 fixed = 17 total (was 23) +1 mvnsite 0m 51s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 22s the patch passed +1 javadoc 0m 52s the patch passed +1 unit 7m 57s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 39m 21s Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12805249/HADOOP-10048.004.patch JIRA Issue HADOOP-10048 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5fabcb1af9bb 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 / 757050f Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9538/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9538/console Powered by Apache Yetus 0.3.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          sjlee0 Sangjin Lee added a comment -

          Thanks Jason Lowe for updating the patch. Just curious, using the AtomicReference is just for readability and making the intention more explicit, right? In terms of actual memory model semantics, volatile and AtomicReference should be the same for the code in this patch.

          Show
          sjlee0 Sangjin Lee added a comment - Thanks Jason Lowe for updating the patch. Just curious, using the AtomicReference is just for readability and making the intention more explicit, right? In terms of actual memory model semantics, volatile and AtomicReference should be the same for the code in this patch.
          Hide
          jlowe Jason Lowe added a comment -

          Correct, AtomicReference internally just uses volatile. I went with it so future maintainers are more likely to cache the reference in a local when wielding it. When it was a naked volatile member it was easier to accidentally reference it multiple times in an expression or method which could result in surprising inconsistencies sometimes. I suspect the volatile is ever-so-slightly faster than the AtomicReference since the former requires loading an offset within the current object to get the context reference while the latter requires loading an offset within the current object to get the AtomicReference reference object then loading that to get the context reference. However the difference will be unmeasurable with this use-case, so I went with the more-likely-to-be-correct-under-maintenance approach.

          Show
          jlowe Jason Lowe added a comment - Correct, AtomicReference internally just uses volatile. I went with it so future maintainers are more likely to cache the reference in a local when wielding it. When it was a naked volatile member it was easier to accidentally reference it multiple times in an expression or method which could result in surprising inconsistencies sometimes. I suspect the volatile is ever-so-slightly faster than the AtomicReference since the former requires loading an offset within the current object to get the context reference while the latter requires loading an offset within the current object to get the AtomicReference reference object then loading that to get the context reference. However the difference will be unmeasurable with this use-case, so I went with the more-likely-to-be-correct-under-maintenance approach.
          Hide
          djp Junping Du added a comment -

          Thanks Jason Lowe for updating the patch. I am OK with lockless approach to solve the problem. However, in the existing patch (004), it seems cannot guarantee the consistency for Async call of getLocalPathForWrite(.., conf, ...) with conf in multi-thread case if conf get changed.
          Consider the case that thread A (conf1) and thread B (conf2) call this method at the same time: thread A hit confChanged() with conf1 first, then thread B hit confChanged() with conf2, afterwards thread A going forward to get path for write that is based on conf2 now - that means thread A could get a path that is not existed in conf1.
          I think we should keep the consistency between API's return result and parameter, like: we can let confChanged() return a thread local Context object and do the following thing with this local context. What do you think?

          Show
          djp Junping Du added a comment - Thanks Jason Lowe for updating the patch. I am OK with lockless approach to solve the problem. However, in the existing patch (004), it seems cannot guarantee the consistency for Async call of getLocalPathForWrite(.., conf, ...) with conf in multi-thread case if conf get changed. Consider the case that thread A (conf1) and thread B (conf2) call this method at the same time: thread A hit confChanged() with conf1 first, then thread B hit confChanged() with conf2, afterwards thread A going forward to get path for write that is based on conf2 now - that means thread A could get a path that is not existed in conf1. I think we should keep the consistency between API's return result and parameter, like: we can let confChanged() return a thread local Context object and do the following thing with this local context. What do you think?
          Hide
          jlowe Jason Lowe added a comment -

          Yep, I agree that it would be better to be consistent given they passed an explicit conf, and I agree that returning the context to use from confChanged is a straightforward fix for that. I updated the patch accordingly.

          Show
          jlowe Jason Lowe added a comment - Yep, I agree that it would be better to be consistent given they passed an explicit conf, and I agree that returning the context to use from confChanged is a straightforward fix for that. I updated the patch accordingly.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch 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 6m 41s trunk passed
          +1 compile 7m 8s trunk passed
          +1 checkstyle 0m 23s trunk passed
          +1 mvnsite 1m 0s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 22s trunk passed
          +1 javadoc 1m 5s trunk passed
          +1 mvninstall 0m 39s the patch passed
          +1 compile 6m 54s the patch passed
          +1 javac 6m 54s the patch passed
          +1 checkstyle 0m 23s hadoop-common-project/hadoop-common: The patch generated 0 new + 17 unchanged - 6 fixed = 17 total (was 23)
          +1 mvnsite 0m 56s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 36s the patch passed
          +1 javadoc 0m 53s the patch passed
          +1 unit 8m 28s hadoop-common in the patch passed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          39m 12s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12806151/HADOOP-10048.005.patch
          JIRA Issue HADOOP-10048
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c7929f7fc633 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 / 9a31e5d
          Default Java 1.8.0_91
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9580/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9580/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s 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 6m 41s trunk passed +1 compile 7m 8s trunk passed +1 checkstyle 0m 23s trunk passed +1 mvnsite 1m 0s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 1m 5s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 6m 54s the patch passed +1 javac 6m 54s the patch passed +1 checkstyle 0m 23s hadoop-common-project/hadoop-common: The patch generated 0 new + 17 unchanged - 6 fixed = 17 total (was 23) +1 mvnsite 0m 56s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 36s the patch passed +1 javadoc 0m 53s the patch passed +1 unit 8m 28s hadoop-common in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 39m 12s Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12806151/HADOOP-10048.005.patch JIRA Issue HADOOP-10048 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c7929f7fc633 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 / 9a31e5d Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9580/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9580/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          djp Junping Du added a comment -

          Thanks Jason Lowe for updating the patch!
          005 patch looks good in overall. Just two minor issues:

          +          ctx.dirNumLastAccessed = dirNum;
          

          Given the ctx could be local context, I think we want to update it to the currentContext which can accessed immediately - something like: currentContext.get().dirNumLastAccessed = dirNum. Isn't it?

          +            if(ctx.localFS.mkdirs(tmpDir)|| ctx.localFS.exists(tmpDir)) {
          

          Shouldn't we check dir exists first then mkdir if not?

          Show
          djp Junping Du added a comment - Thanks Jason Lowe for updating the patch! 005 patch looks good in overall. Just two minor issues: + ctx.dirNumLastAccessed = dirNum; Given the ctx could be local context, I think we want to update it to the currentContext which can accessed immediately - something like: currentContext.get().dirNumLastAccessed = dirNum. Isn't it? + if(ctx.localFS.mkdirs(tmpDir)|| ctx.localFS.exists(tmpDir)) { Shouldn't we check dir exists first then mkdir if not?
          Hide
          jlowe Jason Lowe added a comment -

          Given the ctx could be local context, I think we want to update it to the currentContext which can accessed immediately - something like: currentContext.get().dirNumLastAccessed = dirNum. Isn't it?

          No, that could trigger an array bounds exception if we update it to a value that is past the number of directories in the other, unrelated context. Also we don't need to worry about this particular race. When the new context is set it will compute a new random index for the next directory, so the index will still be random even if a call using the old context missed updating the new context.

          Shouldn't we check dir exists first then mkdir if not?

          I was just preserving the existing behavior since it's unrelated to this change. In practice the raw local fs mkdirs already does the exists check anyway. Also switching them could lead to a weird error later if the local path ended up being an existing file. localFS.exists will return true and we won't try the mkdirs, but mkdirs throws a useful error message explaining it's trying to create a directory where a file already exists. Since this is not related to this change and could degrade the error diagnostics in some corner cases, I'm tempted to leave it as-is. If we feel it's important to fix it then we can tackle it in a followup JIRA where it does the full file stat first, checks the corner cases, then calls mkdirs if necessary.

          Show
          jlowe Jason Lowe added a comment - Given the ctx could be local context, I think we want to update it to the currentContext which can accessed immediately - something like: currentContext.get().dirNumLastAccessed = dirNum. Isn't it? No, that could trigger an array bounds exception if we update it to a value that is past the number of directories in the other, unrelated context. Also we don't need to worry about this particular race. When the new context is set it will compute a new random index for the next directory, so the index will still be random even if a call using the old context missed updating the new context. Shouldn't we check dir exists first then mkdir if not? I was just preserving the existing behavior since it's unrelated to this change. In practice the raw local fs mkdirs already does the exists check anyway. Also switching them could lead to a weird error later if the local path ended up being an existing file. localFS.exists will return true and we won't try the mkdirs, but mkdirs throws a useful error message explaining it's trying to create a directory where a file already exists. Since this is not related to this change and could degrade the error diagnostics in some corner cases, I'm tempted to leave it as-is. If we feel it's important to fix it then we can tackle it in a followup JIRA where it does the full file stat first, checks the corner cases, then calls mkdirs if necessary.
          Hide
          djp Junping Du added a comment -

          No, that could trigger an array bounds exception if we update it to a value that is past the number of directories in the other, unrelated context. Also we don't need to worry about this particular race.

          We can simply move dirNum % numDirs ahead of ctx.dirDF[dirNum] to get rid of array out of bound issue. However, I agree that this particular race is not important given the value of dirNumLastAccessed could mean something different in different context.
          Under the same context, mark dirNumLastAccessed as volatile could still cause multiple threads end up with the same dirNumLastAccessed in case int dirNum = ctx.dirNumLastAccessed; get accessed at almost the same time. In this case, previous round-robin pickup for disks with available capacity is broken, we may use random instead. Otherwise, accessing of disks could be aggregated on particular disk. Thoughts?

          Since this is not related to this change and could degrade the error diagnostics in some corner cases, I'm tempted to leave it as-is. If we feel it's important to fix it then we can tackle it in a followup JIRA where it does the full file stat first, checks the corner cases, then calls mkdirs if necessary.

          That sounds a reasonable plan. We can discuss this later in other JIRA.

          Show
          djp Junping Du added a comment - No, that could trigger an array bounds exception if we update it to a value that is past the number of directories in the other, unrelated context. Also we don't need to worry about this particular race. We can simply move dirNum % numDirs ahead of ctx.dirDF [dirNum] to get rid of array out of bound issue. However, I agree that this particular race is not important given the value of dirNumLastAccessed could mean something different in different context. Under the same context, mark dirNumLastAccessed as volatile could still cause multiple threads end up with the same dirNumLastAccessed in case int dirNum = ctx.dirNumLastAccessed; get accessed at almost the same time. In this case, previous round-robin pickup for disks with available capacity is broken, we may use random instead. Otherwise, accessing of disks could be aggregated on particular disk. Thoughts? Since this is not related to this change and could degrade the error diagnostics in some corner cases, I'm tempted to leave it as-is. If we feel it's important to fix it then we can tackle it in a followup JIRA where it does the full file stat first, checks the corner cases, then calls mkdirs if necessary. That sounds a reasonable plan. We can discuss this later in other JIRA.
          Hide
          jlowe Jason Lowe added a comment -

          Otherwise, accessing of disks could be aggregated on particular disk. Thoughts?

          I'm not worried about the dirNumLastAccessed being somewhat random – it already is random if someone needs a write location without specifying a size. What is more concerning is the thundering herd problem where a bunch of threads all need write locations with a size at the same time. All or most of the threads could end up theoretically clustering on the same disk which is less than ideal. Attaching a new patch that uses an AtomicInteger to make sure that simultaneous threads won't get the same starting point when searching the directories.

          This approach doesn't completely solve the clustering issue when one or more disks gets full enough to not satisfy the requests. An alternative approach would be to use a random starting location like is done when the size is not specified. I went with this approach since it is closer to the original semantics without adding the undesired locking necessary to guarantee it.

          Show
          jlowe Jason Lowe added a comment - Otherwise, accessing of disks could be aggregated on particular disk. Thoughts? I'm not worried about the dirNumLastAccessed being somewhat random – it already is random if someone needs a write location without specifying a size. What is more concerning is the thundering herd problem where a bunch of threads all need write locations with a size at the same time. All or most of the threads could end up theoretically clustering on the same disk which is less than ideal. Attaching a new patch that uses an AtomicInteger to make sure that simultaneous threads won't get the same starting point when searching the directories. This approach doesn't completely solve the clustering issue when one or more disks gets full enough to not satisfy the requests. An alternative approach would be to use a random starting location like is done when the size is not specified. I went with this approach since it is closer to the original semantics without adding the undesired locking necessary to guarantee it.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 27s 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 8m 51s trunk passed
          +1 compile 9m 17s trunk passed
          +1 checkstyle 0m 27s trunk passed
          +1 mvnsite 1m 16s trunk passed
          +1 mvneclipse 0m 11s trunk passed
          +1 findbugs 1m 49s trunk passed
          +1 javadoc 1m 2s trunk passed
          +1 mvninstall 1m 0s the patch passed
          +1 compile 9m 7s the patch passed
          +1 javac 9m 7s the patch passed
          +1 checkstyle 0m 28s hadoop-common-project/hadoop-common: The patch generated 0 new + 17 unchanged - 6 fixed = 17 total (was 23)
          +1 mvnsite 1m 15s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 54s the patch passed
          +1 javadoc 1m 0s the patch passed
          -1 unit 17m 29s hadoop-common in the patch failed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          56m 52s



          Reason Tests
          Failed junit tests hadoop.ha.TestZKFailoverController
          Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:2c91fd8
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12807744/HADOOP-10048.006.patch
          JIRA Issue HADOOP-10048
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 2a6066b77148 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 / aadb77e
          Default Java 1.8.0_91
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit test logs https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 27s 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 8m 51s trunk passed +1 compile 9m 17s trunk passed +1 checkstyle 0m 27s trunk passed +1 mvnsite 1m 16s trunk passed +1 mvneclipse 0m 11s trunk passed +1 findbugs 1m 49s trunk passed +1 javadoc 1m 2s trunk passed +1 mvninstall 1m 0s the patch passed +1 compile 9m 7s the patch passed +1 javac 9m 7s the patch passed +1 checkstyle 0m 28s hadoop-common-project/hadoop-common: The patch generated 0 new + 17 unchanged - 6 fixed = 17 total (was 23) +1 mvnsite 1m 15s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 54s the patch passed +1 javadoc 1m 0s the patch passed -1 unit 17m 29s hadoop-common in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 56m 52s Reason Tests Failed junit tests hadoop.ha.TestZKFailoverController Timed out junit tests org.apache.hadoop.http.TestHttpServerLifecycle Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12807744/HADOOP-10048.006.patch JIRA Issue HADOOP-10048 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 2a6066b77148 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 / aadb77e Default Java 1.8.0_91 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit test logs https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9649/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          The unit tests failures appear to be unrelated, and the tests pass for me locally with the patch applied.

          Show
          jlowe Jason Lowe added a comment - The unit tests failures appear to be unrelated, and the tests pass for me locally with the patch applied.
          Hide
          djp Junping Du added a comment -

          Thanks Jason Lowe for updating the patch.

          All or most of the threads could end up theoretically clustering on the same disk which is less than ideal. Attaching a new patch that uses an AtomicInteger to make sure that simultaneous threads won't get the same starting point when searching the directories.

          Make sense. This approach looks better in solving this problem.

          An alternative approach would be to use a random starting location like is done when the size is not specified.

          Agree. This could be a nice improvement that we could make later. However, for size not specified case, creating a Random object per call may not be necessary. May be this is also something we can improve later?

          Latest (006) patch looks pretty good to me. I would like to get it in for next 24 hours if no further comments from others.

          Show
          djp Junping Du added a comment - Thanks Jason Lowe for updating the patch. All or most of the threads could end up theoretically clustering on the same disk which is less than ideal. Attaching a new patch that uses an AtomicInteger to make sure that simultaneous threads won't get the same starting point when searching the directories. Make sense. This approach looks better in solving this problem. An alternative approach would be to use a random starting location like is done when the size is not specified. Agree. This could be a nice improvement that we could make later. However, for size not specified case, creating a Random object per call may not be necessary. May be this is also something we can improve later? Latest (006) patch looks pretty good to me. I would like to get it in for next 24 hours if no further comments from others.
          Hide
          djp Junping Du added a comment -

          I have commit the patch to trunk, branch-2 and branch-2.8. Thanks Jason Lowe for patch contribution!

          Show
          djp Junping Du added a comment - I have commit the patch to trunk, branch-2 and branch-2.8. Thanks Jason Lowe for patch contribution!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #9921 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9921/)
          HADOOP-10048. LocalDirAllocator should avoid holding locks while (junping_du: rev c14c1b298e29e799f7c8f15ff24d7eba6e0cd39b)

          • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/LocalDirAllocator.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #9921 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9921/ ) HADOOP-10048 . LocalDirAllocator should avoid holding locks while (junping_du: rev c14c1b298e29e799f7c8f15ff24d7eba6e0cd39b) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/LocalDirAllocator.java
          Hide
          zhz Zhe Zhang added a comment -

          Thanks Jason Lowe for the fix and Junping Du for the review. Is this a valid bug fix for 2.7 and 2.6 as well? Should we consider backporting it?

          Show
          zhz Zhe Zhang added a comment - Thanks Jason Lowe for the fix and Junping Du for the review. Is this a valid bug fix for 2.7 and 2.6 as well? Should we consider backporting it?
          Hide
          jlowe Jason Lowe added a comment -

          Technically this is a performance optimization improvement rather than a bug fix. Typically we wouldn't backport since those other releases are in maintenance and should only receive fixes rather than features/improvements to reduce the risk of destabilizing those releases. If we feel this is an important enough performance improvement and the rewards outweigh the risks then I'm OK with it. It doesn't pick cleanly, but it would if we also backported HADOOP-8436, HADOOP-8437, and HADOOP-12252 which are all bug fixes.

          Show
          jlowe Jason Lowe added a comment - Technically this is a performance optimization improvement rather than a bug fix. Typically we wouldn't backport since those other releases are in maintenance and should only receive fixes rather than features/improvements to reduce the risk of destabilizing those releases. If we feel this is an important enough performance improvement and the rewards outweigh the risks then I'm OK with it. It doesn't pick cleanly, but it would if we also backported HADOOP-8436 , HADOOP-8437 , and HADOOP-12252 which are all bug fixes.
          Hide
          djp Junping Du added a comment -

          I would prefer not to backport to 2.6 and 2.7 for the same reason as Jason Lowe mentioned. This is a performance gain but not a bug fix which should be kept out of maintenance release as our previous practices.

          Show
          djp Junping Du added a comment - I would prefer not to backport to 2.6 and 2.7 for the same reason as Jason Lowe mentioned. This is a performance gain but not a bug fix which should be kept out of maintenance release as our previous practices.
          Hide
          zhz Zhe Zhang added a comment -

          Thanks for the comments Jason Lowe and Junping Du. I agree we should keep it in 2.8.

          Show
          zhz Zhe Zhang added a comment - Thanks for the comments Jason Lowe and Junping Du . I agree we should keep it in 2.8.

            People

            • Assignee:
              jlowe Jason Lowe
              Reporter:
              jlowe Jason Lowe
            • Votes:
              1 Vote for this issue
              Watchers:
              24 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development