Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: test
    • Labels:
      None

      Description

      This subtask takes care about HDFS part of Hadoop only. Others will be added later as needed: it will include only new aspects development and modifications of build.xml file

      1. HADOOP-6003.sh
        0.5 kB
        Konstantin Boudnik
      2. HADOOP-6003.patch
        13 kB
        Konstantin Boudnik
      3. HADOOP-6003.patch
        13 kB
        Konstantin Boudnik
      4. HADOOP-6003.patch
        13 kB
        Konstantin Boudnik
      5. HADOOP-6003.sh
        0.5 kB
        Konstantin Boudnik
      6. HADOOP-6003.patch
        15 kB
        Konstantin Boudnik
      7. HADOOP-6003.patch
        16 kB
        Konstantin Boudnik
      8. HADOOP-6003.patch
        16 kB
        Konstantin Boudnik
      9. HADOOP-6003.patch
        16 kB
        Konstantin Boudnik
      10. HADOOP-6003.patch
        16 kB
        Konstantin Boudnik
      11. HADOOP-6003.patch
        16 kB
        Konstantin Boudnik
      12. HADOOP-6003.patch
        16 kB
        Konstantin Boudnik

        Activity

        Hide
        Konstantin Boudnik added a comment -

        Thanks a bunch, Nicholas!

        I'm so sorry to cause so many remarks - it's my very first real patch in the
        Hadoop and I was kinda sluggish.

        Thanks for your patience and commitment!
        Cos

        Show
        Konstantin Boudnik added a comment - Thanks a bunch, Nicholas! I'm so sorry to cause so many remarks - it's my very first real patch in the Hadoop and I was kinda sluggish. Thanks for your patience and commitment! Cos
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Forgot to post test-patch results earlier:

             [exec] -1 overall.  
             [exec] 
             [exec]     +1 @author.  The patch does not contain any @author tags.
             [exec] 
             [exec]     +1 tests included.  The patch appears to include 9 new or modified tests.
             [exec] 
             [exec]     -1 javadoc.  The javadoc tool appears to have generated 1 warning messages.
             [exec] 
             [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
             [exec] 
             [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
             [exec] 
             [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
        

        The javadoc warnings are not related. See HDFS-440.

        Show
        Tsz Wo Nicholas Sze added a comment - Forgot to post test-patch results earlier: [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 9 new or modified tests. [exec] [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. The javadoc warnings are not related. See HDFS-440 .
        Hide
        Tsz Wo Nicholas Sze added a comment -

        I have committed this. Thanks, Cos!

        Show
        Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, Cos!
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1 patch looks good.

        Show
        Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
        Hide
        Konstantin Boudnik added a comment - - edited

        Patch is modified (mostly the modifications of the build.xml file) to reflect the split between projects and renaming of the core to common.

        It has been testes and verified that it works with new project structure.

        Show
        Konstantin Boudnik added a comment - - edited Patch is modified (mostly the modifications of the build.xml file) to reflect the split between projects and renaming of the core to common. It has been testes and verified that it works with new project structure.
        Hide
        Konstantin Boudnik added a comment -

        JavaDoc comments tags are fixed
        Better documentation for the FI classes is written.

        Other comments are considered and fixes are made.

        Show
        Konstantin Boudnik added a comment - JavaDoc comments tags are fixed Better documentation for the FI classes is written. Other comments are considered and fixes are made.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        After the project split, this issue belongs to hdfs.

        Show
        Tsz Wo Nicholas Sze added a comment - After the project split, this issue belongs to hdfs.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Codes look good. Two more minor comments:

        • javadoc need to be quoted by /** */. For example,
          +/*
          + * This class wraps the logic around fault injection configuration file
          + * Default file is expected to be found in src/test/fi-site.xml
          + * This default file should be copied by JUnit Ant's tasks to 
          + * build/test/extraconf folder before tests are ran
          + * An alternative location can be set through
          + *   -Dfault.property.config=<file_name>
          + */
          
        • Unnecessary space changes should be reverted. e.g.
          --- build.xml	(revision 786191)
          +++ build.xml	(working copy)
          @@ -428,7 +428,7 @@
                 <fileset dir="${mapred.src.dir}" includes="mapred-default.xml"/>
               </copy>
             </target>
          -
          +  
             <target name="compile-hdfs-classes" depends="compile-core-classes">
               <jsp-compile
                uriroot="${src.webapps}/hdfs"
          @@ -452,7 +452,7 @@
               </jsp-compile>
           
               <!-- Compile Java files (excluding JSPs) checking warnings -->
          -    <javac 
          +   <javac 
                encoding="${build.encoding}" 
                srcdir="${hdfs.src.dir};${build.src}" 
                includes="org/apache/hadoop/**/*.java"
          
        Show
        Tsz Wo Nicholas Sze added a comment - Codes look good. Two more minor comments: javadoc need to be quoted by /** */. For example, +/* + * This class wraps the logic around fault injection configuration file + * Default file is expected to be found in src/test/fi-site.xml + * This default file should be copied by JUnit Ant's tasks to + * build/test/extraconf folder before tests are ran + * An alternative location can be set through + * -Dfault.property.config=<file_name> + */ Unnecessary space changes should be reverted. e.g. --- build.xml (revision 786191) +++ build.xml (working copy) @@ -428,7 +428,7 @@ <fileset dir= "${mapred.src.dir}" includes= "mapred- default .xml" /> </copy> </target> - + <target name= "compile-hdfs-classes" depends= "compile-core-classes" > <jsp-compile uriroot= "${src.webapps}/hdfs" @@ -452,7 +452,7 @@ </jsp-compile> <!-- Compile Java files (excluding JSPs) checking warnings --> - <javac + <javac encoding= "${build.encoding}" srcdir= "${hdfs.src.dir};${build.src}" includes= "org/apache/hadoop/**/*.java"
        Hide
        Konstantin Boudnik added a comment -

        Debug output has to have log4j's debug level

        Show
        Konstantin Boudnik added a comment - Debug output has to have log4j's debug level
        Hide
        Konstantin Boudnik added a comment -

        Default probability for all faults has to be 0.00

        Show
        Konstantin Boudnik added a comment - Default probability for all faults has to be 0.00
        Hide
        Konstantin Boudnik added a comment -

        I agree with the point - it is misleading when a level of probability set as an integer.
        I have fixed the config file comment and ProbabilityModel code to work with the value as a float between 0.00 and 1.00
        I think adding a special case to handle '%' is excessive.

        Show
        Konstantin Boudnik added a comment - I agree with the point - it is misleading when a level of probability set as an integer. I have fixed the config file comment and ProbabilityModel code to work with the value as a float between 0.00 and 1.00 I think adding a special case to handle '%' is excessive.
        Hide
        Tsz Wo Nicholas Sze added a comment -
        • I thought fi.* is the probability, but it actually is the probability in percentage. It seems to me that it is not common to specify probability in percentage. I suggest that if it is a percentage, it needs a %. i.e.
          • <value>1</value> means probability = 1
          • <value>1%</value> means probability = 0.01
        • Tried the latest patch. Everything worked fine: TestFileCreation failed when setting fi.* = 100.0. ant clean succeeded. I probably messed something up last time. My faults!
        Show
        Tsz Wo Nicholas Sze added a comment - I thought fi.* is the probability, but it actually is the probability in percentage. It seems to me that it is not common to specify probability in percentage. I suggest that if it is a percentage, it needs a %. i.e. <value>1</value> means probability = 1 <value>1%</value> means probability = 0.01 Tried the latest patch. Everything worked fine: TestFileCreation failed when setting fi.* = 100.0. ant clean succeeded. I probably messed something up last time. My faults!
        Hide
        Konstantin Boudnik added a comment - - edited

        Slightly different version of the original patch:
        conf/fi-site.xml is moved to src/test. build.xml is modified to copy it for test runs.
        <iajc> task definition is moved inside of the target 'injectfaults' to guarantee that it always defined

        Couldn't reproduce the issue with

        ant clean
        

        works for me every time. Is it possible that you had some files permission issues?

        Wrt to TestFileCreation: I have ran this test many times and seen a failure only once. The problem with this test is that while I can confirm that faults methods are called they aren't called frequent enough (i.e. < 80 calls during the test execution) to reach the necessary threshold of 1% to inject a fault. In other words, the instrumented isn't being called often enough. I'd suggest to run TestDirectoryScanner instead where these instrumented functions are called pretty often so the test fails on every run.

        Show
        Konstantin Boudnik added a comment - - edited Slightly different version of the original patch: conf/fi-site.xml is moved to src/test. build.xml is modified to copy it for test runs. <iajc> task definition is moved inside of the target 'injectfaults' to guarantee that it always defined Couldn't reproduce the issue with ant clean works for me every time. Is it possible that you had some files permission issues? Wrt to TestFileCreation: I have ran this test many times and seen a failure only once. The problem with this test is that while I can confirm that faults methods are called they aren't called frequent enough (i.e. < 80 calls during the test execution) to reach the necessary threshold of 1% to inject a fault. In other words, the instrumented isn't being called often enough. I'd suggest to run TestDirectoryScanner instead where these instrumented functions are called pretty often so the test fails on every run.
        Hide
        Tsz Wo Nicholas Sze added a comment -
        • Tried the following
        1. ant injectfaults
        2. set fi.* to 1
        3. ant test-core -Dtestcase=TestFileCreation
          It still passed the test. Have I done something wrong?
        • ant clean failed.
          bash-3.2$ ant clean
          Buildfile: build.xml
          ...
          BUILD FAILED
          d:\@sze\hadoop\latest\build.xml:1604: Unable to delete file d:\@sze\hadoop\latest\build\ivy\lib\Hadoop\common\aspectjtools-1.6.4.jar
          
          Total time: 0 seconds
          
        • I think fi-site.xml should not be placed in conf directory. It may confuse cluster administrators. It is better to put everything under test.
        Show
        Tsz Wo Nicholas Sze added a comment - Tried the following ant injectfaults set fi.* to 1 ant test-core -Dtestcase=TestFileCreation It still passed the test. Have I done something wrong? ant clean failed. bash-3.2$ ant clean Buildfile: build.xml ... BUILD FAILED d:\@sze\hadoop\latest\build.xml:1604: Unable to delete file d:\@sze\hadoop\latest\build\ivy\lib\Hadoop\common\aspectjtools-1.6.4.jar Total time: 0 seconds I think fi-site.xml should not be placed in conf directory. It may confuse cluster administrators. It is better to put everything under test.
        Hide
        Konstantin Boudnik added a comment -

        Final modifications:

        • new package is renamed to org.apache.hadoop.fi
        • FIConfig class is renamed to FiConfig
        • system property prefix is made to be 'fi' instead of rather lengthy fault.probability
        • default config file for FI framework is added
        • missed Apache license boiler plate is attached to one of the files
        Show
        Konstantin Boudnik added a comment - Final modifications: new package is renamed to org.apache.hadoop.fi FIConfig class is renamed to FiConfig system property prefix is made to be 'fi' instead of rather lengthy fault.probability default config file for FI framework is added missed Apache license boiler plate is attached to one of the files
        Hide
        Konstantin Boudnik added a comment -

        This version of the patch includes a way to define probability levels through a standard Hadoop configuration file. Default location of the config file is conf/fi-site.xml
        An alternative location might be set through -Dfault.probability.config= system property

        Also, one can use a standard build.property file to specify all needed probability levels in the runtime

        Show
        Konstantin Boudnik added a comment - This version of the patch includes a way to define probability levels through a standard Hadoop configuration file. Default location of the config file is conf/fi-site.xml An alternative location might be set through -Dfault.probability.config= system property Also, one can use a standard build.property file to specify all needed probability levels in the runtime
        Hide
        Konstantin Boudnik added a comment -

        Agree, config file option seems like a good idea. I was planning to add this
        later and have this framework to be in place ASAP, but I can surely add this
        feature right now.

        One more thing: I believe it would be good to have dynamic configuration i.e.
        through system properties as well as the config file. This ability to pass
        system properties provides ad-hoc testing opportunity if one needs to quickly
        check/reproduce something.

        I'll provide config file option shortly.

        Show
        Konstantin Boudnik added a comment - Agree, config file option seems like a good idea. I was planning to add this later and have this framework to be in place ASAP, but I can surely add this feature right now. One more thing: I believe it would be good to have dynamic configuration i.e. through system properties as well as the config file. This ability to pass system properties provides ad-hoc testing opportunity if one needs to quickly check/reproduce something. I'll provide config file option shortly.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        I think the ant command may look like:

        ant test -Dfault.injection.enable -Dfault.injection.conf=CONF_FILE
        

        All the FI parameters, included probability values, are specified in CONF_FILE. Then, we only have to change conf file but not the build script for the later development.

        Show
        Tsz Wo Nicholas Sze added a comment - I think the ant command may look like: ant test -Dfault.injection.enable -Dfault.injection.conf=CONF_FILE All the FI parameters, included probability values, are specified in CONF_FILE. Then, we only have to change conf file but not the build script for the later development.
        Hide
        Konstantin Boudnik added a comment -

        Indentation inconsistency is fixed.

        As for the separate tests development: I agree on that. Different tests would have to be developed with FI in minds. However, for the Phase 1 (e.g. the framework introduction) we might use existing tests if in need to verify certain aspects of the error handling.

        On the API: I think current proposal will not stand when we'll be talking about complex injection scenarios and would have to replaced with something else. I'm agree that we need to develop some of these scenarios before a better API might be developed.

        Show
        Konstantin Boudnik added a comment - Indentation inconsistency is fixed. As for the separate tests development: I agree on that. Different tests would have to be developed with FI in minds. However, for the Phase 1 (e.g. the framework introduction) we might use existing tests if in need to verify certain aspects of the error handling. On the API: I think current proposal will not stand when we'll be talking about complex injection scenarios and would have to replaced with something else. I'm agree that we need to develop some of these scenarios before a better API might be developed.
        Hide
        Philip Zeyliger added a comment -

        It looks like some of the patches have inconsistent usage of tabs/spaces; you may want to clean that up.

        It is expected to see unit tests failing with faults in place. We might need to develop different kind of tests to utilize fault injection in a better way.

        I think you're right on here. It seems like having a global setting for injection probability will mostly lead to test results that are hard to grok. Have you thought about setting up a separate package/target for tests that are designed to interact with injected faults? Once one or two of those tests exists, it'll be clearer whether "ant run-test-hdfs -Dfault.probability.FSDataset=3" is the right API.

        Show
        Philip Zeyliger added a comment - It looks like some of the patches have inconsistent usage of tabs/spaces; you may want to clean that up. It is expected to see unit tests failing with faults in place. We might need to develop different kind of tests to utilize fault injection in a better way. I think you're right on here. It seems like having a global setting for injection probability will mostly lead to test results that are hard to grok. Have you thought about setting up a separate package/target for tests that are designed to interact with injected faults? Once one or two of those tests exists, it'll be clearer whether "ant run-test-hdfs -Dfault.probability.FSDataset=3" is the right API.
        Hide
        Konstantin Boudnik added a comment -

        As far as I can tell this failing test isn't related to the patch at all, because of its orthogonality. Test failure seems to be connected to some sort of configuration issues.

        Review of the patch will be highly appreciated!

        Show
        Konstantin Boudnik added a comment - As far as I can tell this failing test isn't related to the patch at all, because of its orthogonality. Test failure seems to be connected to some sort of configuration issues. Review of the patch will be highly appreciated!
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12410248/HADOOP-6003.patch
        against trunk revision 783672.

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

        +1 tests included. The patch appears to include 10 new or modified tests.

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

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        -1 Eclipse classpath. The patch causes the Eclipse classpath to differ from the contents of the lib directories.

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

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

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12410248/HADOOP-6003.patch against trunk revision 783672. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 10 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 Eclipse classpath. The patch causes the Eclipse classpath to differ from the contents of the lib directories. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/491/console This message is automatically generated.
        Hide
        Konstantin Boudnik added a comment - - edited

        This patch includes the following additions:

        • AspectJ framework (version 1.6.4) is added to the Ivy resolver's configuration
        • the implementation of a simple probability calculation and configuration needed by fault injection
        • two aspects for datanode's classes BlockReceiver and FSDataset are created and tested

        It is expected to see unit tests failing with faults in place. We might need to develop different kind of tests to utilize fault injection in a better way.

        The interface of the new framework is as follows:

        • ant injectfaults will weave the aspects in place after the normal compilation of HDFS classes is complete
        • ant run-test-hdfs will execute unit tests as usual, but faults will be injected according to the rules
        • ant jar will create Hadoop's jar as usual, but if 'injectfaults' has been executed before then the jar file will include instrumented classes, e.g. with fault invocations

        The rules of faults injection probability calculation are as follows:

        • default probability level is set to 0. Thus even with aspects weaved into the classes faults won't be injected/executed unless specified explicitly
        • to set certain class' faults probability level one needs to specify system property in the following format
          •   ant run-test-hdfs -Dfault.probability.FSDataset=3 
            

            which will set the probability of faults injections into FSDataset class at about 3%

        Show
        Konstantin Boudnik added a comment - - edited This patch includes the following additions: AspectJ framework (version 1.6.4) is added to the Ivy resolver's configuration the implementation of a simple probability calculation and configuration needed by fault injection two aspects for datanode's classes BlockReceiver and FSDataset are created and tested It is expected to see unit tests failing with faults in place. We might need to develop different kind of tests to utilize fault injection in a better way. The interface of the new framework is as follows: ant injectfaults will weave the aspects in place after the normal compilation of HDFS classes is complete ant run-test-hdfs will execute unit tests as usual, but faults will be injected according to the rules ant jar will create Hadoop's jar as usual, but if 'injectfaults' has been executed before then the jar file will include instrumented classes, e.g. with fault invocations The rules of faults injection probability calculation are as follows: default probability level is set to 0. Thus even with aspects weaved into the classes faults won't be injected/executed unless specified explicitly to set certain class' faults probability level one needs to specify system property in the following format ant run-test-hdfs -Dfault.probability.FSDataset=3 which will set the probability of faults injections into FSDataset class at about 3%
        Hide
        Konstantin Boudnik added a comment -

        Shell script for folders creation and the patch

        Show
        Konstantin Boudnik added a comment - Shell script for folders creation and the patch

          People

          • Assignee:
            Konstantin Boudnik
            Reporter:
            Konstantin Boudnik
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development