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
    • Hadoop Flags:
      Reviewed

      Description

      According to the test plan a number of new features are going to be implemented as a part of this umbrella (HDFS-265) JIRA.
      These new features are have to be tested properly. Block recovery is one of new functionality which require new tests to be developed.

      1. blockRecoveryPositive.patch
        23 kB
        Hairong Kuang
      2. blockRecoveryPositive1.patch
        28 kB
        Hairong Kuang
      3. blockRecoveryPositive2_0.21.patch
        16 kB
        Hairong Kuang
      4. blockRecoveryPositive2.patch
        28 kB
        Hairong Kuang
      5. TEST-org.apache.hadoop.hdfs.server.datanode.TestBlockRecovery.txt
        4 kB
        Konstantin Boudnik

        Issue Links

          Activity

          Konstantin Boudnik created issue -
          Konstantin Boudnik made changes -
          Field Original Value New Value
          Description According to the test plan a number of new features are going to be implemented as a part of this umbrella (HDFS-265) JIRA.
          These new features are have to be tested properly. Block recovery is one of new functionality which require new tests to be developed.
          Konstantin Boudnik made changes -
          Component/s test [ 12312916 ]
          Konstantin Boudnik made changes -
          Assignee Konstantin Boudnik [ cos ]
          Konstantin Boudnik made changes -
          Assignee Konstantin Boudnik [ cos ]
          Konstantin Boudnik made changes -
          Assignee Konstantin Boudnik [ cos ]
          Hide
          Konstantin Boudnik added a comment -

          I've spent quite some time doing some refactoring of the Datanode code to make it more testable. It seems to be possible to master some tests with Mockito. However, they look more like end-to-end test eventually, because of high complexity of dependencies in the code. E.g. mocking of the datanode requires to spy on NameNode (i.e. actually initialize it) because there some calls between these two; then an actual files need to be added or proper blocks mocked and inserted to cover particular executed paths; and so on.

          Which forced me to take a look at the current coverage situation of Datanode class. To my surprise I've discovered that recoverBlock() is pretty much well covered (except for some error conditions processing); syncBlock() looks even better. FsNameSysmtem.commitBlockSynchronization() seems to be Ok as well. It means that existing tests are covering this new functionality in a pretty decent fashion.

          I'd suggest to drop these tests. For real.

          Show
          Konstantin Boudnik added a comment - I've spent quite some time doing some refactoring of the Datanode code to make it more testable. It seems to be possible to master some tests with Mockito. However, they look more like end-to-end test eventually, because of high complexity of dependencies in the code. E.g. mocking of the datanode requires to spy on NameNode (i.e. actually initialize it) because there some calls between these two; then an actual files need to be added or proper blocks mocked and inserted to cover particular executed paths; and so on. Which forced me to take a look at the current coverage situation of Datanode class. To my surprise I've discovered that recoverBlock() is pretty much well covered (except for some error conditions processing); syncBlock() looks even better. FsNameSysmtem.commitBlockSynchronization() seems to be Ok as well. It means that existing tests are covering this new functionality in a pretty decent fashion. I'd suggest to drop these tests. For real.
          Hide
          Konstantin Boudnik added a comment -

          After discussion with HDFS dev. folks it has been decided to hold the effort of implementing this feature until more resources is available.

          The risk of running into some issues is relatively low because the target code is indirectly covered by other tests (according to a Clover report)

          Show
          Konstantin Boudnik added a comment - After discussion with HDFS dev. folks it has been decided to hold the effort of implementing this feature until more resources is available. The risk of running into some issues is relatively low because the target code is indirectly covered by other tests (according to a Clover report)
          Konstantin Boudnik made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Won't Fix [ 2 ]
          Hide
          Allen Wittenauer added a comment -

          It seems very wrong to close this as "won't fix" just because Yahoo! doesn't want to put resources behind it at this moment. If this is still an issue, it should be left open.

          Show
          Allen Wittenauer added a comment - It seems very wrong to close this as "won't fix" just because Yahoo! doesn't want to put resources behind it at this moment. If this is still an issue, it should be left open.
          Hide
          Konstantin Boudnik added a comment -

          'Y! doesn't want to put resources behind..." wasn't the reason to close the JIRA. It had been closed (and, btw, all the test cases in the original test plan are left intact) because it doesn't seem reasonable to develop new test cases for a code which is covered pretty well.

          Show
          Konstantin Boudnik added a comment - 'Y! doesn't want to put resources behind..." wasn't the reason to close the JIRA. It had been closed (and, btw, all the test cases in the original test plan are left intact) because it doesn't seem reasonable to develop new test cases for a code which is covered pretty well.
          Hide
          Hairong Kuang added a comment -

          I am reopening this. Block recovery leads to the truncation of data thus has the risk of losing data. Making sure that the tests cover all the cases is important.

          Show
          Hairong Kuang added a comment - I am reopening this. Block recovery leads to the truncation of data thus has the risk of losing data. Making sure that the tests cover all the cases is important.
          Hairong Kuang made changes -
          Resolution Won't Fix [ 2 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Hairong Kuang added a comment -

          This patch imlements the block recovery tests described in section 5.
          1. TestBlockRecovery covers BlockRecovery_02.8 - 13.
          2. TestLeaseRecovery2#testHardLeaseRecovery is a funcational test that covers BlockRecovery_02 (02.1-02.5,) and BlockRecovery_03, 03.1, 04.
          3. I do not think BlockRecovery_02.6 is in the scope of this test.
          4. BlockRecovery_01 is a negative test that will be covered in the negative test suite.

          Show
          Hairong Kuang added a comment - This patch imlements the block recovery tests described in section 5. 1. TestBlockRecovery covers BlockRecovery_02.8 - 13. 2. TestLeaseRecovery2#testHardLeaseRecovery is a funcational test that covers BlockRecovery_02 (02.1-02.5,) and BlockRecovery_03, 03.1, 04. 3. I do not think BlockRecovery_02.6 is in the scope of this test. 4. BlockRecovery_01 is a negative test that will be covered in the negative test suite.
          Hairong Kuang made changes -
          Attachment blockRecoveryPositive.patch [ 12436051 ]
          Hairong Kuang made changes -
          Link This issue is blocked by HADOOP-6570 [ HADOOP-6570 ]
          Hide
          Konstantin Boudnik added a comment -

          A couple of nits:

          • TestBlockRecovery has a mix of JUnit3 and JUnit4 in it. In particular
            +import static junit.framework.Assert.assertTrue;
            

            needs to be replaced with appropriate org.junit imports

          • TestLeaseRecovery2#testHardLeaseRecovery uses a mix of LOG statements and invocations of System.out.println. Is it intentional?
          • in general, would it make sense to add some test description in the form of JavaDoc?

          Looks good otherwise!

          Show
          Konstantin Boudnik added a comment - A couple of nits: TestBlockRecovery has a mix of JUnit3 and JUnit4 in it. In particular +import static junit.framework.Assert.assertTrue; needs to be replaced with appropriate org.junit imports TestLeaseRecovery2#testHardLeaseRecovery uses a mix of LOG statements and invocations of System.out.println . Is it intentional? in general, would it make sense to add some test description in the form of JavaDoc? Looks good otherwise!
          Hide
          Hairong Kuang added a comment -

          Here is a new patch that addressed all Cos's comments.

          Show
          Hairong Kuang added a comment - Here is a new patch that addressed all Cos's comments.
          Hairong Kuang made changes -
          Attachment blockRecoveryPositive.patch [ 12436355 ]
          Hairong Kuang made changes -
          Status Reopened [ 4 ] Patch Available [ 10002 ]
          Hairong Kuang made changes -
          Attachment blockRecoveryPositive1.patch [ 12436368 ]
          Hairong Kuang made changes -
          Attachment blockRecoveryPositive.patch [ 12436355 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12436355/blockRecoveryPositive.patch
          against trunk revision 911744.

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

          +1 tests included. The patch appears to include 6 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 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/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/12436355/blockRecoveryPositive.patch against trunk revision 911744. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/237/console This message is automatically generated.
          Hide
          Konstantin Boudnik added a comment -

          Hmm, please let me know if I'm doing something wrong here, but I've tried to run
          ant clean run-test-unit and all TestBlockRecovery tests have failed (see attached log)

          Show
          Konstantin Boudnik added a comment - Hmm, please let me know if I'm doing something wrong here, but I've tried to run ant clean run-test-unit and all TestBlockRecovery tests have failed (see attached log)
          Konstantin Boudnik made changes -
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12436410/TEST-org.apache.hadoop.hdfs.server.datanode.TestBlockRecovery.txt
          against trunk revision 911744.

          +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 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/238/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/12436410/TEST-org.apache.hadoop.hdfs.server.datanode.TestBlockRecovery.txt against trunk revision 911744. +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 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/238/console This message is automatically generated.
          Hide
          Hairong Kuang added a comment -

          This patch fixes the failed test problem.

          Show
          Hairong Kuang added a comment - This patch fixes the failed test problem.
          Hairong Kuang made changes -
          Attachment blockRecoveryPositive2.patch [ 12436621 ]
          Hide
          Konstantin Boudnik added a comment -

          Yup, it does the trick. I've ran the tests are both seems to be running fine now.

          +1 on the patch.

          Show
          Konstantin Boudnik added a comment - Yup, it does the trick. I've ran the tests are both seems to be running fine now. +1 on the patch.
          Hide
          Konstantin Boudnik added a comment -

          Clearly, Hairong was working on this.

          Show
          Konstantin Boudnik added a comment - Clearly, Hairong was working on this.
          Konstantin Boudnik made changes -
          Assignee Konstantin Boudnik [ cos ] Hairong Kuang [ hairong ]
          Hide
          Hairong Kuang added a comment -

          Here is a patch for 0.21.

          Show
          Hairong Kuang added a comment - Here is a patch for 0.21.
          Hairong Kuang made changes -
          Attachment blockRecoveryPositive2_0.21.patch [ 12439530 ]
          Hairong Kuang made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Fix Version/s 0.21.0 [ 12314046 ]
          Fix Version/s 0.22.0 [ 12314241 ]
          Resolution Fixed [ 1 ]
          Hide
          Hairong Kuang added a comment -

          Forgot to mention that all unit tests are passed on my local machine.

          I've just committed this!

          Show
          Hairong Kuang added a comment - Forgot to mention that all unit tests are passed on my local machine. I've just committed this!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #220 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/220/)
          . Create new tests for block recovery. Contributed by Hairong.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #220 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/220/ ) . Create new tests for block recovery. Contributed by Hairong.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #146 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/146/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #146 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/146/ )
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #302 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/302/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #302 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/302/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #275 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/275/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #275 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/275/ )
          Tom White made changes -
          Fix Version/s 0.22.0 [ 12314241 ]
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Hairong Kuang
              Reporter:
              Konstantin Boudnik
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development