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

Create framework for configurable disk checkers

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-alpha1
    • Component/s: util
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. HADOOP-13254.001.patch
      12 kB
      Yufei Gu
    2. HADOOP-13254.002.patch
      13 kB
      Yufei Gu
    3. HADOOP-13254.003.patch
      13 kB
      Yufei Gu
    4. HADOOP-13254.004.patch
      13 kB
      Yufei Gu
    5. HADOOP-13254.005.patch
      13 kB
      Yufei Gu
    6. HADOOP-13254.006.patch
      13 kB
      Yufei Gu
    7. HADOOP-13254.007.patch
      13 kB
      Yufei Gu
    8. HADOOP-13254.008.patch
      13 kB
      Yufei Gu

      Issue Links

        Activity

        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 22s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
        +1 mvninstall 8m 49s trunk passed
        +1 compile 9m 3s trunk passed
        +1 checkstyle 0m 29s trunk passed
        +1 mvnsite 1m 14s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 1m 56s trunk passed
        +1 javadoc 0m 54s trunk passed
        -1 mvninstall 0m 30s hadoop-common in the patch failed.
        -1 compile 0m 44s root in the patch failed.
        -1 javac 0m 44s root in the patch failed.
        -1 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 8 new + 26 unchanged - 1 fixed = 34 total (was 27)
        -1 mvnsite 0m 31s hadoop-common in the patch failed.
        +1 mvneclipse 0m 11s the patch passed
        -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix.
        -1 findbugs 0m 30s hadoop-common in the patch failed.
        +1 javadoc 0m 51s the patch passed
        -1 unit 0m 30s hadoop-common in the patch failed.
        +1 asflicense 0m 18s The patch does not generate ASF License warnings.
        28m 14s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:2c91fd8
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809533/HADOOP-13254.001.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 259b9999da5d 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 / 8a1dcce
        Default Java 1.8.0_91
        findbugs v3.0.0
        mvninstall https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt
        compile https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-compile-root.txt
        javac https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-compile-root.txt
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
        mvnsite https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt
        whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/whitespace-eol.txt
        findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/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 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 8m 49s trunk passed +1 compile 9m 3s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 14s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 56s trunk passed +1 javadoc 0m 54s trunk passed -1 mvninstall 0m 30s hadoop-common in the patch failed. -1 compile 0m 44s root in the patch failed. -1 javac 0m 44s root in the patch failed. -1 checkstyle 0m 25s hadoop-common-project/hadoop-common: The patch generated 8 new + 26 unchanged - 1 fixed = 34 total (was 27) -1 mvnsite 0m 31s hadoop-common in the patch failed. +1 mvneclipse 0m 11s the patch passed -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix. -1 findbugs 0m 30s hadoop-common in the patch failed. +1 javadoc 0m 51s the patch passed -1 unit 0m 30s hadoop-common in the patch failed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 28m 14s Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809533/HADOOP-13254.001.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 259b9999da5d 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 / 8a1dcce Default Java 1.8.0_91 findbugs v3.0.0 mvninstall https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-mvninstall-hadoop-common-project_hadoop-common.txt compile https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-compile-root.txt javac https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-compile-root.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt mvnsite https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-mvnsite-hadoop-common-project_hadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/whitespace-eol.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-findbugs-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9738/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 21s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
        +1 mvninstall 6m 49s trunk passed
        +1 compile 7m 0s trunk passed
        +1 checkstyle 0m 23s trunk passed
        +1 mvnsite 0m 57s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 1m 27s trunk passed
        +1 javadoc 0m 45s trunk passed
        +1 mvninstall 0m 40s the patch passed
        +1 compile 7m 33s the patch passed
        +1 javac 7m 33s the patch passed
        -1 checkstyle 0m 23s hadoop-common-project/hadoop-common: The patch generated 1 new + 26 unchanged - 1 fixed = 27 total (was 27)
        +1 mvnsite 0m 58s the patch passed
        +1 mvneclipse 0m 13s the patch passed
        -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 findbugs 1m 44s the patch passed
        +1 javadoc 0m 55s the patch passed
        -1 unit 8m 22s hadoop-common in the patch failed.
        +1 asflicense 0m 19s The patch does not generate ASF License warnings.
        39m 46s



        Reason Tests
        Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:2c91fd8
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809552/HADOOP-13254.002.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 3943ecc1ae21 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 / 8a1dcce
        Default Java 1.8.0_91
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
        whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/artifact/patchprocess/whitespace-eol.txt
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/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 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 6m 49s trunk passed +1 compile 7m 0s trunk passed +1 checkstyle 0m 23s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 27s trunk passed +1 javadoc 0m 45s trunk passed +1 mvninstall 0m 40s the patch passed +1 compile 7m 33s the patch passed +1 javac 7m 33s the patch passed -1 checkstyle 0m 23s hadoop-common-project/hadoop-common: The patch generated 1 new + 26 unchanged - 1 fixed = 27 total (was 27) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 13s the patch passed -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 1m 44s the patch passed +1 javadoc 0m 55s the patch passed -1 unit 8m 22s hadoop-common in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 39m 46s Reason Tests Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809552/HADOOP-13254.002.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 3943ecc1ae21 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 / 8a1dcce Default Java 1.8.0_91 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/artifact/patchprocess/whitespace-eol.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9747/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yufeigu Yufei Gu added a comment -

        Uploaded patch 003 for the style issue.
        The "white space" issue and the unit test failure are unrelated.

        Show
        yufeigu Yufei Gu added a comment - Uploaded patch 003 for the style issue. The "white space" issue and the unit test failure are unrelated.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 22s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
        +1 mvninstall 6m 47s trunk passed
        +1 compile 6m 59s trunk passed
        +1 checkstyle 0m 23s trunk passed
        +1 mvnsite 0m 56s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 32s trunk passed
        +1 javadoc 0m 46s trunk passed
        +1 mvninstall 0m 40s the patch passed
        +1 compile 6m 55s the patch passed
        +1 javac 6m 55s the patch passed
        +1 checkstyle 0m 29s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
        +1 mvnsite 0m 55s the patch passed
        +1 mvneclipse 0m 15s the patch passed
        -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 findbugs 1m 37s the patch passed
        +1 javadoc 0m 48s the patch passed
        -1 unit 8m 18s hadoop-common in the patch failed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        38m 58s



        Reason Tests
        Failed junit tests hadoop.net.TestDNS



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:2c91fd8
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809570/HADOOP-13254.003.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux d0cc4c8975e8 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 / 8a1dcce
        Default Java 1.8.0_91
        findbugs v3.0.0
        whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/artifact/patchprocess/whitespace-eol.txt
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/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 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 6m 47s trunk passed +1 compile 6m 59s trunk passed +1 checkstyle 0m 23s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 32s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 40s the patch passed +1 compile 6m 55s the patch passed +1 javac 6m 55s the patch passed +1 checkstyle 0m 29s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27) +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 15s the patch passed -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 1m 37s the patch passed +1 javadoc 0m 48s the patch passed -1 unit 8m 18s hadoop-common in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 38m 58s Reason Tests Failed junit tests hadoop.net.TestDNS Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809570/HADOOP-13254.003.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d0cc4c8975e8 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 / 8a1dcce Default Java 1.8.0_91 findbugs v3.0.0 whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/artifact/patchprocess/whitespace-eol.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9748/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yufeigu Yufei Gu added a comment -

        whitespace issue and test failure are unrelated.

        Show
        yufeigu Yufei Gu added a comment - whitespace issue and test failure are unrelated.
        Hide
        templedf Daniel Templeton added a comment -

        Thanks, Yufei Gu. A few comments:

        • Can we add @Override to BasicDiskValidator.checkStatus()?
        • In DiskValidatorFactory. getInstance(Class), shouldn't the put() call be putIfAbsent()?
        • In DiskValidatorFactory. getInstance(Class), can you use a local variable to return at the end instead of having three exits points from the method?
        • In DiskValidatorFactory. getInstance(String), the toLowerCase() is superfluous.
        • In DiskValidatorFactory. getInstance(String), when you create the exception, please include the source exception as the cause.
        • In TestBasicDiskValidator, I'm pretty sure that all of the tests inherited from TestDiskValidator will be run anyway; there's no need to wrap them. I think you're making the tests run twice.
        • In TestBasicDiskValidator.checkDirs(), you should make sure the temp file gets deleted. Either try-finally or use deleteOnExit().
        • In TestBasicDiskValidator.checkDirs(), please remove the System.out.println().
        • In TestBasicDiskValidator.checkDirs(), please make the assert messages more useful. What exactly failed and most likely why?
        • In TestDiskValidatorFactory.testGetInstance(), you should use assertNotNull() instead of assertTrue().
        • In TestDiskValidatorFactory.testGetInstance(), it would be good to check that the instance type that you get back in the right one. It would also be nice to test that you're actually caching correctly. You might have to make INSTANCES @VisibleForTesting to make that work.
        Show
        templedf Daniel Templeton added a comment - Thanks, Yufei Gu . A few comments: Can we add @Override to BasicDiskValidator.checkStatus() ? In DiskValidatorFactory. getInstance(Class) , shouldn't the put() call be putIfAbsent() ? In DiskValidatorFactory. getInstance(Class) , can you use a local variable to return at the end instead of having three exits points from the method? In DiskValidatorFactory. getInstance(String) , the toLowerCase() is superfluous. In DiskValidatorFactory. getInstance(String) , when you create the exception, please include the source exception as the cause. In TestBasicDiskValidator , I'm pretty sure that all of the tests inherited from TestDiskValidator will be run anyway; there's no need to wrap them. I think you're making the tests run twice. In TestBasicDiskValidator.checkDirs() , you should make sure the temp file gets deleted. Either try-finally or use deleteOnExit() . In TestBasicDiskValidator.checkDirs() , please remove the System.out.println() . In TestBasicDiskValidator.checkDirs() , please make the assert messages more useful. What exactly failed and most likely why? In TestDiskValidatorFactory.testGetInstance() , you should use assertNotNull() instead of assertTrue() . In TestDiskValidatorFactory.testGetInstance() , it would be good to check that the instance type that you get back in the right one. It would also be nice to test that you're actually caching correctly. You might have to make INSTANCES @VisibleForTesting to make that work.
        Hide
        yufeigu Yufei Gu added a comment -

        Daniel Templeton, thanks a lot for the detailed review. I uploaded patch 004 for all the comments.

        Show
        yufeigu Yufei Gu added a comment - Daniel Templeton , thanks a lot for the detailed review. I uploaded patch 004 for all the comments.
        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 appears to include 3 new or modified test files.
        +1 mvninstall 7m 5s trunk passed
        +1 compile 6m 43s trunk passed
        +1 checkstyle 0m 25s trunk passed
        +1 mvnsite 1m 2s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 29s trunk passed
        +1 javadoc 0m 47s trunk passed
        +1 mvninstall 0m 40s the patch passed
        +1 compile 7m 4s the patch passed
        +1 javac 7m 4s the patch passed
        +1 checkstyle 0m 23s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
        +1 mvnsite 0m 53s the patch passed
        +1 mvneclipse 0m 13s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 33s the patch passed
        +1 javadoc 0m 47s the patch passed
        +1 unit 7m 49s hadoop-common in the patch passed.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        38m 24s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:e2f6409
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12810924/HADOOP-13254.004.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 87b5b26630d1 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 / 6f0aa75
        Default Java 1.8.0_91
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9788/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9788/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 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 7m 5s trunk passed +1 compile 6m 43s trunk passed +1 checkstyle 0m 25s trunk passed +1 mvnsite 1m 2s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 29s trunk passed +1 javadoc 0m 47s trunk passed +1 mvninstall 0m 40s the patch passed +1 compile 7m 4s the patch passed +1 javac 7m 4s the patch passed +1 checkstyle 0m 23s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27) +1 mvnsite 0m 53s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 33s the patch passed +1 javadoc 0m 47s the patch passed +1 unit 7m 49s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 38m 24s Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12810924/HADOOP-13254.004.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 87b5b26630d1 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 / 6f0aa75 Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9788/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9788/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        templedf Daniel Templeton added a comment -

        Thanks, yufei. A few more comments:

        • Would you mind adding some comments to DiskValidatorFactory.getInstance(Class) that explains why you're checking the result of the put and using it if it's not null? I don't think it's obvious that it's because it could be used by multiple threads.
        • This assert message:
              assertTrue("checkDir success", success);
        

        still needs to be clearer. Something like, "call to checkDir() succeeded even though it was expected to fail"

        • In TestBasicDiskValidator.checkDirs(), I think the try for the try-finally should start a bit earlier so that it encompasses all possible unexpected exit points.
        • In TestBasicDiskValidator.checkDirs(), this code:
              File localDir = File.createTempFile("test", "tmp");
              if (isDir) {
                localDir.delete();
                localDir.mkdir();
              }
          

          doesn't make any sense to me. Shouldn't you test if it should be a dir first instead of deleting the file and creating a dir if that's what's needed?

        • In TestDiskValidatorFactory.testGetInstance(), at the end you try to get a bad instance, but you don't do anything with the result. If that's on purpose, you should at least document it.
        Show
        templedf Daniel Templeton added a comment - Thanks, yufei . A few more comments: Would you mind adding some comments to DiskValidatorFactory.getInstance(Class) that explains why you're checking the result of the put and using it if it's not null? I don't think it's obvious that it's because it could be used by multiple threads. This assert message: assertTrue( "checkDir success" , success); still needs to be clearer. Something like, "call to checkDir() succeeded even though it was expected to fail" In TestBasicDiskValidator.checkDirs() , I think the try for the try-finally should start a bit earlier so that it encompasses all possible unexpected exit points. In TestBasicDiskValidator.checkDirs() , this code: File localDir = File.createTempFile( "test" , "tmp" ); if (isDir) { localDir.delete(); localDir.mkdir(); } doesn't make any sense to me. Shouldn't you test if it should be a dir first instead of deleting the file and creating a dir if that's what's needed? In TestDiskValidatorFactory.testGetInstance() , at the end you try to get a bad instance, but you don't do anything with the result. If that's on purpose, you should at least document it.
        Hide
        yufeigu Yufei Gu added a comment -

        Thanks Daniel Templeton for the detailed review. I uploaded the patch 005 for all the comments.

        Show
        yufeigu Yufei Gu added a comment - Thanks Daniel Templeton for the detailed review. I uploaded the patch 005 for all the comments.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 23s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
        +1 mvninstall 6m 57s trunk passed
        +1 compile 7m 21s trunk passed
        +1 checkstyle 0m 23s trunk passed
        +1 mvnsite 0m 56s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 1m 27s trunk passed
        +1 javadoc 0m 49s trunk passed
        +1 mvninstall 0m 41s the patch passed
        +1 compile 7m 33s the patch passed
        +1 javac 7m 33s the patch passed
        +1 checkstyle 0m 22s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
        +1 mvnsite 0m 54s the patch passed
        +1 mvneclipse 0m 15s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 37s the patch passed
        +1 javadoc 0m 47s the patch passed
        -1 unit 8m 8s hadoop-common in the patch failed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        39m 52s



        Reason Tests
        Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:e2f6409
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811142/HADOOP-13254.005.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux a290235827a1 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 / e14ee0d
        Default Java 1.8.0_91
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9800/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9800/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9800/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 23s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 6m 57s trunk passed +1 compile 7m 21s trunk passed +1 checkstyle 0m 23s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 27s trunk passed +1 javadoc 0m 49s trunk passed +1 mvninstall 0m 41s the patch passed +1 compile 7m 33s the patch passed +1 javac 7m 33s the patch passed +1 checkstyle 0m 22s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27) +1 mvnsite 0m 54s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 37s the patch passed +1 javadoc 0m 47s the patch passed -1 unit 8m 8s hadoop-common in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 39m 52s Reason Tests Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811142/HADOOP-13254.005.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a290235827a1 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 / e14ee0d Default Java 1.8.0_91 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9800/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9800/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9800/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yufeigu Yufei Gu added a comment -

        Patch 006 is uploaded to separate test in TestDiskValidatorFactory to avoid a potential error.

        Show
        yufeigu Yufei Gu added a comment - Patch 006 is uploaded to separate test in TestDiskValidatorFactory to avoid a potential error.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 24s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
        +1 mvninstall 7m 12s trunk passed
        +1 compile 7m 59s trunk passed
        +1 checkstyle 0m 29s trunk passed
        +1 mvnsite 1m 9s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 1m 50s trunk passed
        +1 javadoc 0m 56s trunk passed
        +1 mvninstall 0m 54s the patch passed
        +1 compile 9m 2s the patch passed
        +1 javac 9m 2s the patch passed
        +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
        +1 mvnsite 1m 8s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 2m 1s the patch passed
        +1 javadoc 0m 52s the patch passed
        -1 unit 9m 4s hadoop-common in the patch failed.
        +1 asflicense 0m 23s The patch does not generate ASF License warnings.
        45m 11s



        Reason Tests
        Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:e2f6409
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811159/HADOOP-13254.006.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux ddb35e0304ef 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 / e983eaf
        Default Java 1.8.0_91
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9801/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9801/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9801/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 24s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 7m 12s trunk passed +1 compile 7m 59s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 9s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 50s trunk passed +1 javadoc 0m 56s trunk passed +1 mvninstall 0m 54s the patch passed +1 compile 9m 2s the patch passed +1 javac 9m 2s the patch passed +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27) +1 mvnsite 1m 8s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 1s the patch passed +1 javadoc 0m 52s the patch passed -1 unit 9m 4s hadoop-common in the patch failed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 45m 11s Reason Tests Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811159/HADOOP-13254.006.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ddb35e0304ef 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 / e983eaf Default Java 1.8.0_91 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9801/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9801/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9801/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yufeigu Yufei Gu added a comment -

        The test failure is unrelated.

        Show
        yufeigu Yufei Gu added a comment - The test failure is unrelated.
        Hide
        templedf Daniel Templeton added a comment -

        Thanks, Yufei Gu. Almost there. We're now down to quibbles:

        In TestBasicDiskValidator.checkDirs(), the try-finally needs to start immediately after you create the file.

           * Returns {@link DiskValidator} instance corresponding to its name.
           * DiskValidator can be "basic" for BasicDiskValidator;
           * @param diskValidator canonical class name, e.g. "basic"
        

        has a couple of issues. In the second line, it would be clearer if "DiskValidator" were "The parameter" or "The diskValidator parameter". Also in the second line, "BasicDiskValidator" should be a link. In the @param tag, "e.g." should be "for example" per the javadoc guidelines.

         * A {@link DiskValidator} is the interface of a disk validator.
        

        should not use a link for DiskValidator since the comment is in the DiskValidator class.

        /**
         * The basic DiskValidator do the same thing as existing DiskChecker do.
         */
        

        This comment would be clearer if it said that BasicDiskValidator is a wrapper around DiskChecker.

        Show
        templedf Daniel Templeton added a comment - Thanks, Yufei Gu . Almost there. We're now down to quibbles: In TestBasicDiskValidator.checkDirs() , the try-finally needs to start immediately after you create the file. * Returns {@link DiskValidator} instance corresponding to its name. * DiskValidator can be "basic" for BasicDiskValidator; * @param diskValidator canonical class name, e.g. "basic" has a couple of issues. In the second line, it would be clearer if "DiskValidator" were "The parameter" or "The diskValidator parameter". Also in the second line, "BasicDiskValidator" should be a link. In the @param tag, "e.g." should be "for example" per the javadoc guidelines. * A {@link DiskValidator} is the interface of a disk validator. should not use a link for DiskValidator since the comment is in the DiskValidator class. /** * The basic DiskValidator do the same thing as existing DiskChecker do . */ This comment would be clearer if it said that BasicDiskValidator is a wrapper around DiskChecker .
        Hide
        yufeigu Yufei Gu added a comment -

        Daniel Templeton, thanks a lot for the nice suggestions. I've uploaded the patch 007 for all comments.

        Show
        yufeigu Yufei Gu added a comment - Daniel Templeton , thanks a lot for the nice suggestions. I've uploaded the patch 007 for all comments.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 23s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
        +1 mvninstall 7m 58s trunk passed
        +1 compile 8m 29s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 1m 4s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 1m 34s trunk passed
        +1 javadoc 0m 50s trunk passed
        +1 mvninstall 0m 50s the patch passed
        +1 compile 8m 42s the patch passed
        +1 javac 8m 42s the patch passed
        +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
        +1 mvnsite 0m 59s the patch passed
        +1 mvneclipse 0m 13s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 44s the patch passed
        +1 javadoc 0m 58s the patch passed
        +1 unit 8m 51s hadoop-common in the patch passed.
        +1 asflicense 0m 23s The patch does not generate ASF License warnings.
        44m 56s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:e2f6409
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12812618/HADOOP-13254.007.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux da3f56808697 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 / 17eae9e
        Default Java 1.8.0_91
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9855/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9855/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 23s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 7m 58s trunk passed +1 compile 8m 29s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 4s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 34s trunk passed +1 javadoc 0m 50s trunk passed +1 mvninstall 0m 50s the patch passed +1 compile 8m 42s the patch passed +1 javac 8m 42s the patch passed +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27) +1 mvnsite 0m 59s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 44s the patch passed +1 javadoc 0m 58s the patch passed +1 unit 8m 51s hadoop-common in the patch passed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 44m 56s Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12812618/HADOOP-13254.007.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux da3f56808697 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 / 17eae9e Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9855/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9855/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        templedf Daniel Templeton added a comment -

        LGTM. +1 (non-binding)

        Show
        templedf Daniel Templeton added a comment - LGTM. +1 (non-binding)
        Hide
        yufeigu Yufei Gu added a comment -

        Thanks a lot, Daniel Templeton.

        Show
        yufeigu Yufei Gu added a comment - Thanks a lot, Daniel Templeton .
        Hide
        rchiang Ray Chiang added a comment - - edited

        Changed summary to match code changes. I'm assuming the configuration settings will go into YARN-5137.

        More minor nits:

        • I see that DiskValidatorFactory#getInstance(String) has @param and @throws, but DiskValidatorFactory#getInstance(Class) does not. I know it's a bit redundant, but I see similar classes have the Javadoc for both methods.
        • Make the InterfaceAudience settings more conservative for now (since DiskChecker is @InterfaceAudience.Private):
          • Keep DiskValidator as @InterfaceAudience.Private (i.e. Hadoop only) for now.
          • For DiskValidatorFactory, either remove @InterfaceStability.Evolving or change it to @InterfaceAudience.Unstable.
        Show
        rchiang Ray Chiang added a comment - - edited Changed summary to match code changes. I'm assuming the configuration settings will go into YARN-5137 . More minor nits: I see that DiskValidatorFactory#getInstance(String) has @param and @throws, but DiskValidatorFactory#getInstance(Class) does not. I know it's a bit redundant, but I see similar classes have the Javadoc for both methods. Make the InterfaceAudience settings more conservative for now (since DiskChecker is @InterfaceAudience.Private): Keep DiskValidator as @InterfaceAudience.Private (i.e. Hadoop only) for now. For DiskValidatorFactory, either remove @InterfaceStability.Evolving or change it to @InterfaceAudience.Unstable.
        Hide
        yufeigu Yufei Gu added a comment -

        Ray Chiang, thanks for the review. I uploaded patch 008 for all comments.

        Show
        yufeigu Yufei Gu added a comment - Ray Chiang , thanks for the review. I uploaded patch 008 for all comments.
        Hide
        rkanter Robert Kanter added a comment -

        I'm concerned about this code in DiskValidatorFactory:

        	      diskValidator = ReflectionUtils.newInstance(clazz, null);
        	      // check the return of putIfAbsent() to see if any other thread have put
        	      // the instance with the same key into INSTANCES
        	      DiskValidator diskValidatorRet =
        	          INSTANCES.putIfAbsent(clazz, diskValidator);
        	      if (diskValidatorRet != null) {
        	        diskValidator = diskValidatorRet;
        	      }
        

        If the implementation of DiskValidator that is being created does something like starting some threads, and putIfAbsent returns a previous instantiation of the class, then we might have a problem. For example, if you have a DiskValidator that starts off a background thread to poll SMART status, you might end up with multiple background threads doing this.

        Show
        rkanter Robert Kanter added a comment - I'm concerned about this code in DiskValidatorFactory : diskValidator = ReflectionUtils.newInstance(clazz, null ); // check the return of putIfAbsent() to see if any other thread have put // the instance with the same key into INSTANCES DiskValidator diskValidatorRet = INSTANCES.putIfAbsent(clazz, diskValidator); if (diskValidatorRet != null ) { diskValidator = diskValidatorRet; } If the implementation of DiskValidator that is being created does something like starting some threads, and putIfAbsent returns a previous instantiation of the class, then we might have a problem. For example, if you have a DiskValidator that starts off a background thread to poll SMART status, you might end up with multiple background threads doing this.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 17s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
        +1 mvninstall 8m 0s trunk passed
        +1 compile 6m 50s trunk passed
        +1 checkstyle 0m 24s trunk passed
        +1 mvnsite 0m 58s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 1m 43s trunk passed
        +1 javadoc 0m 49s trunk passed
        +1 mvninstall 0m 53s the patch passed
        +1 compile 8m 9s the patch passed
        +1 javac 8m 9s the patch passed
        +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
        +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 28s the patch passed
        +1 javadoc 0m 46s the patch passed
        +1 unit 7m 44s hadoop-common in the patch passed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        41m 30s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12816321/HADOOP-13254.008.patch
        JIRA Issue HADOOP-13254
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux d13d944af132 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 / 9560f25
        Default Java 1.8.0_91
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9928/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9928/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 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files. +1 mvninstall 8m 0s trunk passed +1 compile 6m 50s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 58s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 43s trunk passed +1 javadoc 0m 49s trunk passed +1 mvninstall 0m 53s the patch passed +1 compile 8m 9s the patch passed +1 javac 8m 9s the patch passed +1 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27) +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 28s the patch passed +1 javadoc 0m 46s the patch passed +1 unit 7m 44s hadoop-common in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 41m 30s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12816321/HADOOP-13254.008.patch JIRA Issue HADOOP-13254 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d13d944af132 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 / 9560f25 Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9928/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9928/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yufeigu Yufei Gu added a comment -

        Robert Kanter, thanks for the review. Yes, good point. If a DiskValidator creates background threads, it is more like a daemon, and we'd better to have separate functions to start it other than start it while creating, and may add status(running, stop) to check as well. And we can add these once we'd like it to be this way.

        Show
        yufeigu Yufei Gu added a comment - Robert Kanter , thanks for the review. Yes, good point. If a DiskValidator creates background threads, it is more like a daemon, and we'd better to have separate functions to start it other than start it while creating, and may add status(running, stop) to check as well. And we can add these once we'd like it to be this way.
        Hide
        templedf Daniel Templeton added a comment -

        Robert Kanter, if an instance is creating threads in the constructor, that would make it a very naughty instance indeed. Without an API that allows for a clean shutdown, creating threads is a risky proposition. I agree with Yufei Gu that when it comes to adding threads, whoever wants to add threads should change the API to include start and stop methods. Since we can't make any guarantees about what future contributors will do, I also agree that being more defensive isn't a bad idea. The cheapest solution I see is to use locks.

        Show
        templedf Daniel Templeton added a comment - Robert Kanter , if an instance is creating threads in the constructor, that would make it a very naughty instance indeed. Without an API that allows for a clean shutdown, creating threads is a risky proposition. I agree with Yufei Gu that when it comes to adding threads, whoever wants to add threads should change the API to include start and stop methods. Since we can't make any guarantees about what future contributors will do, I also agree that being more defensive isn't a bad idea. The cheapest solution I see is to use locks.
        Hide
        rkanter Robert Kanter added a comment -

        Not just future contributors, but users might make their own disk validator plugins that they don't contribute back, which can do really anything. While we don't need to protect against everything (e.g. I think it's reasonable to assume they shouldn't call System.exit()), we should expect some sort of initialization that might be done in the Constructor.

        The cheapest solution I see is to use locks.

        Another option is to add init() and close() methods, similar to what we have in the Service classes. This way, I think it's okay to assume that users should do things like starting threads or similar things in the init() method, instead of the constructor; and we give them a nice place to clean things up in the close() method.

        Show
        rkanter Robert Kanter added a comment - Not just future contributors, but users might make their own disk validator plugins that they don't contribute back, which can do really anything. While we don't need to protect against everything (e.g. I think it's reasonable to assume they shouldn't call System.exit() ), we should expect some sort of initialization that might be done in the Constructor. The cheapest solution I see is to use locks. Another option is to add init() and close() methods, similar to what we have in the Service classes. This way, I think it's okay to assume that users should do things like starting threads or similar things in the init() method, instead of the constructor; and we give them a nice place to clean things up in the close() method.
        Hide
        rchiang Ray Chiang added a comment -

        I'm good either way, as long as the Javadoc API is clearly documented for how it should be used. I'm not sure it's always good to have a threaded version (otherwise, we'll have a thread, creating a thread, creating a thread, ad nauseum). So, I see two options:

        • Create a single API that supports threaded disk checking and a separate API that supports non-threaded, direct call disk checking.
        • Create a single API that supports both.
        Show
        rchiang Ray Chiang added a comment - I'm good either way, as long as the Javadoc API is clearly documented for how it should be used. I'm not sure it's always good to have a threaded version (otherwise, we'll have a thread, creating a thread, creating a thread, ad nauseum). So, I see two options: Create a single API that supports threaded disk checking and a separate API that supports non-threaded, direct call disk checking. Create a single API that supports both.
        Hide
        yufeigu Yufei Gu added a comment - - edited

        This is another option:
        If any implementation of DiskValidator need threads, it should implement interface org.apache.hadoop.service.Service. The Servcie interface already provides well designed functions for a service like class. Why not reuse it and keep the DiskValidator interface simple. Or, make DiskValidator extend interface Service.

        Show
        yufeigu Yufei Gu added a comment - - edited This is another option: If any implementation of DiskValidator need threads, it should implement interface org.apache.hadoop.service.Service . The Servcie interface already provides well designed functions for a service like class. Why not reuse it and keep the DiskValidator interface simple. Or, make DiskValidator extend interface Service .
        Hide
        rkanter Robert Kanter added a comment -

        Reusing the Service interface is fine. However, DiskValidatorFactory would need to support this by calling the init, start, and stop methods correctly.

        Show
        rkanter Robert Kanter added a comment - Reusing the Service interface is fine. However, DiskValidatorFactory would need to support this by calling the init, start, and stop methods correctly.
        Hide
        rchiang Ray Chiang added a comment -

        We're getting dangerously close to wanting a design document for all of this.

        Initially, the DiskValidator functionality was designed for the following:

        1. Being pluggable
        2. Gathering one or more write/read style checks under one class
        3. Saving metrics based on the class

        As it was initially designed, it was meant to be a blocking call in the sense that "return bad if it's not okay to write to the disk".

        How defensive should the design be? I'd be okay with separate DiskValidator and DiskValidatorService interfaces.

        Show
        rchiang Ray Chiang added a comment - We're getting dangerously close to wanting a design document for all of this. Initially, the DiskValidator functionality was designed for the following: Being pluggable Gathering one or more write/read style checks under one class Saving metrics based on the class As it was initially designed, it was meant to be a blocking call in the sense that "return bad if it's not okay to write to the disk". How defensive should the design be? I'd be okay with separate DiskValidator and DiskValidatorService interfaces.
        Hide
        yufeigu Yufei Gu added a comment - - edited

        Thanks Ray Chiang for reiterating the design goal of DiskValidator. Agree to separate DiskValidator and DiskValidatorService interfaces. BTW, do we want to support users' own disk validator plugins in current design as Robert Kanter mentioned? I don't think it is a good idea to support that since they can do anything.

        I prefer to keep DiskValidator simple first. If we want a Service, we can create a Service to invoke DiskValidator just like what LocalDirsHandlerService does.

        Show
        yufeigu Yufei Gu added a comment - - edited Thanks Ray Chiang for reiterating the design goal of DiskValidator . Agree to separate DiskValidator and DiskValidatorService interfaces. BTW, do we want to support users' own disk validator plugins in current design as Robert Kanter mentioned? I don't think it is a good idea to support that since they can do anything. I prefer to keep DiskValidator simple first. If we want a Service , we can create a Service to invoke DiskValidator just like what LocalDirsHandlerService does.
        Hide
        rkanter Robert Kanter added a comment -

        I wasn't meaning to overly complicate things here. My concern was that if someone went and made their own plugin, that we don't want them to cause problems in the NM. If we're going to restrict the plugins to only Hadoop-supplied ones, which it looks like we are (re-looking at the code, I see that we have Private audience and Unstable on the DiskValidator interface), then I think it's fine to not worry about Threads, being super defensive, and those sorts of things for now.

        Given that, I'm +1 on the 008 patch. I'll commit this tomorrow if nobody has any other comments.

        Show
        rkanter Robert Kanter added a comment - I wasn't meaning to overly complicate things here. My concern was that if someone went and made their own plugin, that we don't want them to cause problems in the NM. If we're going to restrict the plugins to only Hadoop-supplied ones, which it looks like we are (re-looking at the code, I see that we have Private audience and Unstable on the DiskValidator interface), then I think it's fine to not worry about Threads, being super defensive, and those sorts of things for now. Given that, I'm +1 on the 008 patch. I'll commit this tomorrow if nobody has any other comments.
        Hide
        rchiang Ray Chiang added a comment -

        Given that the DiskValidator+Service option can be added without too much issue or even created without creating a new child interface, I'm fine with keeping the 008 patch as-is. +1

        Show
        rchiang Ray Chiang added a comment - Given that the DiskValidator + Service option can be added without too much issue or even created without creating a new child interface, I'm fine with keeping the 008 patch as-is. +1
        Hide
        yufeigu Yufei Gu added a comment - - edited

        Thanks a lot for the review, Robert Kanter and Ray Chiang

        Show
        yufeigu Yufei Gu added a comment - - edited Thanks a lot for the review, Robert Kanter and Ray Chiang
        Hide
        rkanter Robert Kanter added a comment -

        Sorry, I meant to commit this a few days ago. I'll do it shortly.

        Show
        rkanter Robert Kanter added a comment - Sorry, I meant to commit this a few days ago. I'll do it shortly.
        Hide
        rkanter Robert Kanter added a comment -

        Thanks Yufei Gu. And everyone else for their reviews/input.

        Committed to trunk and branch-2!

        Show
        rkanter Robert Kanter added a comment - Thanks Yufei Gu . And everyone else for their reviews/input. Committed to trunk and branch-2!
        Hide
        yufeigu Yufei Gu added a comment - - edited

        Thanks Robert Kanter for the committing.

        Show
        yufeigu Yufei Gu added a comment - - edited Thanks Robert Kanter for the committing.

          People

          • Assignee:
            yufeigu Yufei Gu
            Reporter:
            yufeigu Yufei Gu
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development