Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-877

Client-driven block verification not functioning

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20.1, 0.21.0, 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: hdfs-client, test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This is actually the reason for HDFS-734 (TestDatanodeBlockScanner timing out). The issue is that DFSInputStream relies on readChunk being called one last time at the end of the file in order to receive the lastPacketInBlock=true packet from the DN. However, DFSInputStream.read checks pos < getFileLength() before issuing the read. Thus gotEOS never shifts to true and checksumOk() is never called.

      1. hdfs-877.txt
        14 kB
        Todd Lipcon
      2. hdfs-877-branch20.txt
        10 kB
        Todd Lipcon
      3. hdfs-877.txt
        14 kB
        Todd Lipcon
      4. hdfs-877.txt
        15 kB
        Todd Lipcon
      5. hdfs-877.txt
        5 kB
        Todd Lipcon
      6. hdfs-877.txt
        5 kB
        Todd Lipcon
      7. hdfs-877.txt
        4 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          This may turn out to be reasonably tricky to solve. The issue is that the packet with lastPacketInBlock=true comes in an empty packet after the data has been read. Consider the following scenario:

          1. Block is exactly N bytes
          2. Client determines (or knows) the file length and thus reads exactly up to byte N, but not past. This is the case for MapReduce jobs when an inputsplit doesn't cross block boundaries (eg any input file <1block)
          3. In this case, the server will still send the empty "lastPacketInBlock" packet, but the client will never read it (since it doesn't read ahead in any way)

          Point 2 above is currently being enforced by DFSInputStream, since it calls getFileLength() before passing a read() call down into the BlockReader.

          A couple things to investigate:

          1. Is the check currently done by DFSInputStream important for limiting the length visible to a reader for an in-progress block? Or can that limit be satisfied by passing only the visible length to the OP_READ_BLOCK call? If the length limitation can be ignored in the DFSInputStream layer, I think that would solve the issue fairly trivially.
          2. Alternatively, can we invert BlockReader.readChunk so that it reads ahead a packet? That is to say, if after a read, the internal buffer is emptied, can we read the next packet at this point? I don't really like this solution...
          Show
          Todd Lipcon added a comment - This may turn out to be reasonably tricky to solve. The issue is that the packet with lastPacketInBlock=true comes in an empty packet after the data has been read. Consider the following scenario: Block is exactly N bytes Client determines (or knows) the file length and thus reads exactly up to byte N, but not past. This is the case for MapReduce jobs when an inputsplit doesn't cross block boundaries (eg any input file <1block) In this case, the server will still send the empty "lastPacketInBlock" packet, but the client will never read it (since it doesn't read ahead in any way) Point 2 above is currently being enforced by DFSInputStream, since it calls getFileLength() before passing a read() call down into the BlockReader. A couple things to investigate: Is the check currently done by DFSInputStream important for limiting the length visible to a reader for an in-progress block? Or can that limit be satisfied by passing only the visible length to the OP_READ_BLOCK call? If the length limitation can be ignored in the DFSInputStream layer, I think that would solve the issue fairly trivially. Alternatively, can we invert BlockReader.readChunk so that it reads ahead a packet? That is to say, if after a read, the internal buffer is emptied, can we read the next packet at this point? I don't really like this solution...
          Hide
          Todd Lipcon added a comment -

          Upgrading this to blocker since it's a cause for a test failure.

          Worth noting that this bug only affects the proactive "checksum OK" marking that the client does after reading an entire block (thus avoiding the periodic scan on the DN). If the checksum is found to be invalid on the client, it still reports the bad block to the NN just fine. So, this isn't a dataloss bug, it's just a broken optimization and a failing test.

          Show
          Todd Lipcon added a comment - Upgrading this to blocker since it's a cause for a test failure. Worth noting that this bug only affects the proactive "checksum OK" marking that the client does after reading an entire block (thus avoiding the periodic scan on the DN). If the checksum is found to be invalid on the client, it still reports the bad block to the NN just fine. So, this isn't a dataloss bug, it's just a broken optimization and a failing test.
          Hide
          Todd Lipcon added a comment -

          I took the following route to fixing this:

          • BlockReader now knows how many bytes it's expected to read.
          • After it has read this many bytes, it reads ahead one packet, expecting an empty "end of stream" packet from the DN. In the case that this doesn't come (or it's not the special end of stream packet) it throws IOE. If it does get it, it sets gotEOS.

          This current patch is a little ugly since it duplicates the packet-header-reading logic. I have another version that depends on HDFS-881 to clean that up, if 881 gets committed in the meantime.

          Show
          Todd Lipcon added a comment - I took the following route to fixing this: BlockReader now knows how many bytes it's expected to read. After it has read this many bytes, it reads ahead one packet, expecting an empty "end of stream" packet from the DN. In the case that this doesn't come (or it's not the special end of stream packet) it throws IOE. If it does get it, it sets gotEOS. This current patch is a little ugly since it duplicates the packet-header-reading logic. I have another version that depends on HDFS-881 to clean that up, if 881 gets committed in the meantime.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429705/hdfs-877.txt
          against trunk revision 897068.

          +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 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 appears to introduce 1 new Findbugs warnings.

          +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 passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/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/12429705/hdfs-877.txt against trunk revision 897068. +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 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 appears to introduce 1 new Findbugs warnings. +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 passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/174/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          New patch addresses findbugs warning (isLastPacket member is now unused)

          Show
          Todd Lipcon added a comment - New patch addresses findbugs warning (isLastPacket member is now unused)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429731/hdfs-877.txt
          against trunk revision 897068.

          +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 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 passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/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/12429731/hdfs-877.txt against trunk revision 897068. +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 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 passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/175/console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          TestFsck also times out waiting for client to report a block as corrupt. This patch may also fix that issue.

          Show
          Suresh Srinivas added a comment - TestFsck also times out waiting for client to report a block as corrupt. This patch may also fix that issue.
          Hide
          Suresh Srinivas added a comment -

          Ignore my previous comments. The problem in TestFsck seems to be different. Will address it in a separate jira.

          Show
          Suresh Srinivas added a comment - Ignore my previous comments. The problem in TestFsck seems to be different. Will address it in a separate jira.
          Hide
          Hairong Kuang added a comment -

          Good catch, Todd! Is it possible that pos + bytesToRead > totalBytesToRead in case that the last byte to read does not align at the chunk boundary?

          Show
          Hairong Kuang added a comment - Good catch, Todd! Is it possible that pos + bytesToRead > totalBytesToRead in case that the last byte to read does not align at the chunk boundary?
          Hide
          Todd Lipcon added a comment -

          Is it possible that pos + bytesToRead > totalBytesToRead in case that the last byte to read does not align at the chunk boundary?

          I was originally thinking that the DN sent a partial chunk (with computed checksum) in that case but you're absolutely right. This new patch changes the condition to >=. I also realized that I was neglecting to account for the initial padding when computing this value. I fixed both problems and renamed totalBytesToRead to bytesNeededToFinish.

          Given that this behavior doesn't seem to be well covered by tests I think we should also open another JIRA to directly test BlockReader and ensure that CHECKSUM_OK gets sent in the various cases.

          Show
          Todd Lipcon added a comment - Is it possible that pos + bytesToRead > totalBytesToRead in case that the last byte to read does not align at the chunk boundary? I was originally thinking that the DN sent a partial chunk (with computed checksum) in that case but you're absolutely right. This new patch changes the condition to >=. I also realized that I was neglecting to account for the initial padding when computing this value. I fixed both problems and renamed totalBytesToRead to bytesNeededToFinish. Given that this behavior doesn't seem to be well covered by tests I think we should also open another JIRA to directly test BlockReader and ensure that CHECKSUM_OK gets sent in the various cases.
          Hide
          Hairong Kuang added a comment -

          The change looks good to me.
          > I think we should also open another JIRA to directly test BlockReader and ensure that CHECKSUM_OK gets sent in the various cases.
          Why don't we just add a few test cases to TestDatanodeblockScanner#testDatanodeBlocksacnner in the patch to this jira? Anyway we need unit tests to get this fix committed.

          Show
          Hairong Kuang added a comment - The change looks good to me. > I think we should also open another JIRA to directly test BlockReader and ensure that CHECKSUM_OK gets sent in the various cases. Why don't we just add a few test cases to TestDatanodeblockScanner#testDatanodeBlocksacnner in the patch to this jira? Anyway we need unit tests to get this fix committed.
          Hide
          Todd Lipcon added a comment -

          Anyway we need unit tests to get this fix committed

          I figured the fact that TestDatanodeBlockScanner was already failing meant that this is considered "tested". Just not "tested well"

          I'll try to add some tests early next week for this.

          Show
          Todd Lipcon added a comment - Anyway we need unit tests to get this fix committed I figured the fact that TestDatanodeBlockScanner was already failing meant that this is considered "tested". Just not "tested well" I'll try to add some tests early next week for this.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429786/hdfs-877.txt
          against trunk revision 897068.

          +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 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 appears to introduce 1 new Findbugs warnings.

          +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 passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/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/12429786/hdfs-877.txt against trunk revision 897068. +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 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 appears to introduce 1 new Findbugs warnings. +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 passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/92/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          New patch includes a unit test and also has two bugs fixed:

          • gotEOSBefore needs to be set before the skip() call - reasons are included in a comment
          • I took out the IOException when read past end-of-stream happens twice, since this should not actually be an error, if you follow other InputStream implementations

          I verified that the new unit test catches the original bug from this issue (by reverting that fix) and also that it shows the problem that Hairong identified (by trying the test against the first patch)

          Show
          Todd Lipcon added a comment - New patch includes a unit test and also has two bugs fixed: gotEOSBefore needs to be set before the skip() call - reasons are included in a comment I took out the IOException when read past end-of-stream happens twice, since this should not actually be an error, if you follow other InputStream implementations I verified that the new unit test catches the original bug from this issue (by reverting that fix) and also that it shows the problem that Hairong identified (by trying the test against the first patch)
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12430214/hdfs-877.txt
          against trunk revision 898881.

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

          +1 tests included. The patch appears to include 3 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 passed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/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/12430214/hdfs-877.txt against trunk revision 898881. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/96/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          There's an accidental extra debug INFO line in readChunk here - sorry about that. Waiting for review before bothering to upload a new patch.

          Show
          Todd Lipcon added a comment - There's an accidental extra debug INFO line in readChunk here - sorry about that. Waiting for review before bothering to upload a new patch.
          Hide
          Hairong Kuang added a comment -

          +1. The unit tests are great! I will commit it after you upload a new patch fixing the log issue. Please also fix the unnecessary changes to blank lines.

          Show
          Hairong Kuang added a comment - +1. The unit tests are great! I will commit it after you upload a new patch fixing the log issue. Please also fix the unnecessary changes to blank lines.
          Hide
          Hairong Kuang added a comment -

          Could you please upload a patch for 0.20 as well? Do you mind if I mark HDFS-734 as a duplicate of this so we have only one jira to track a change?

          Show
          Hairong Kuang added a comment - Could you please upload a patch for 0.20 as well? Do you mind if I mark HDFS-734 as a duplicate of this so we have only one jira to track a change?
          Hide
          Todd Lipcon added a comment -

          It actually seems like HDFS-734 is different. For me, TestDatanodeBlockScanner is passing on a fresh branch-0.20 checkout now that HDFS-127 has been reverted. Can you confirm? If so, we can mark that one as fixed.

          However, I backported the new unit test from this JIRA, and discovered that the gotEOSBefore bug is present in branch-0.20. However, this bug doesn't really cause a problem, since it only occurs when doing unaligned reads. If your read is unaligned, then reporting CHECKSUM_OK is a no-op anyway, and it doesn't really matter. Agree?

          Attaching new patch against trunk with fixed whitespace and logging.

          Show
          Todd Lipcon added a comment - It actually seems like HDFS-734 is different. For me, TestDatanodeBlockScanner is passing on a fresh branch-0.20 checkout now that HDFS-127 has been reverted. Can you confirm? If so, we can mark that one as fixed. However, I backported the new unit test from this JIRA, and discovered that the gotEOSBefore bug is present in branch-0.20. However, this bug doesn't really cause a problem, since it only occurs when doing unaligned reads. If your read is unaligned, then reporting CHECKSUM_OK is a no-op anyway, and it doesn't really matter. Agree? Attaching new patch against trunk with fixed whitespace and logging.
          Hide
          Todd Lipcon added a comment -

          Here's a patch for branch-20 that shows that checksum verification works after fixing the gotEOSBefore bug. It does pull in mockito and junit 4 (which should be entirely compatible). I don't think it's pressing to commit it if there's any concern, but the tests might be nice to have to make sure that no other patches break this behavior on the branch.

          Show
          Todd Lipcon added a comment - Here's a patch for branch-20 that shows that checksum verification works after fixing the gotEOSBefore bug. It does pull in mockito and junit 4 (which should be entirely compatible). I don't think it's pressing to commit it if there's any concern, but the tests might be nice to have to make sure that no other patches break this behavior on the branch.
          Hide
          Todd Lipcon added a comment -

          reuploading the same patch as before to put it top on the list, so I can toggle Hudson

          Show
          Todd Lipcon added a comment - reuploading the same patch as before to put it top on the list, so I can toggle Hudson
          Hide
          Hairong Kuang added a comment -

          If this is not the cause of HDFS-734, does the fix need to get into 0.20? This seems to me not so critical.

          Show
          Hairong Kuang added a comment - If this is not the cause of HDFS-734 , does the fix need to get into 0.20? This seems to me not so critical.
          Hide
          Todd Lipcon added a comment -

          Hairong: I agree, probably not worth putting it in 20. I'll probably apply it in our distro just for the sake of the unit test, since it does add yet an extra layer of verification on the tricky DFSClient code. Wanted to upload it here in case anyone else wanted to do the same.

          Show
          Todd Lipcon added a comment - Hairong: I agree, probably not worth putting it in 20. I'll probably apply it in our distro just for the sake of the unit test, since it does add yet an extra layer of verification on the tricky DFSClient code. Wanted to upload it here in case anyone else wanted to do the same.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12430826/hdfs-877.txt
          against trunk revision 899747.

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

          +1 tests included. The patch appears to include 3 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 passed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/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/12430826/hdfs-877.txt against trunk revision 899747. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/100/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Is this good to be committed?

          Show
          Todd Lipcon added a comment - Is this good to be committed?
          Hide
          Hairong Kuang added a comment -

          I've just committed this. Thanks Todd!

          Show
          Hairong Kuang added a comment - I've just committed this. Thanks Todd!
          Hide
          Todd Lipcon added a comment -

          This bug fix should go in 0.21 as well, I think. The current patch doesn't apply, but I'll try to get to one tomorrow or early next week. I ran the new unit test against unpatched 21 and three out of the four cases failed (which is a regression from unpatched 0.20 where only 1/4 fail).

          Show
          Todd Lipcon added a comment - This bug fix should go in 0.21 as well, I think. The current patch doesn't apply, but I'll try to get to one tomorrow or early next week. I ran the new unit test against unpatched 21 and three out of the four cases failed (which is a regression from unpatched 0.20 where only 1/4 fail).
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #175 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/175/)
          . Client-driven block verification not functioning. Contributed by Todd Lipcon.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #175 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/175/ ) . Client-driven block verification not functioning. Contributed by Todd Lipcon.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #208 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/208/)
          . Client-driven block verification not functioning. Contributed by Todd Lipcon.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #208 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/208/ ) . Client-driven block verification not functioning. Contributed by Todd Lipcon.
          Hide
          Hairong Kuang added a comment -

          Could you please explain why you need the fix to be in 0.21? Since TestDataBlockScanner passes, I would not worry that this is a critical bug because checksumOK is for block verification purpose. Since this is not a regression introduced by 0.21, by policy it should not be committed into 0.21.

          Show
          Hairong Kuang added a comment - Could you please explain why you need the fix to be in 0.21? Since TestDataBlockScanner passes, I would not worry that this is a critical bug because checksumOK is for block verification purpose. Since this is not a regression introduced by 0.21, by policy it should not be committed into 0.21.
          Hide
          Todd Lipcon added a comment -

          I think it's a regression in that there now exists a unit test which fails 1 case in 0.20 and 3 cases in 0.21. The fact that TestDataBlockScanner passes doesn't necessarily mean there's no bug, just that the bug isn't exposed by a test.

          I don't think it's absolutely critical, but it seems worth fixing. If you disagree, I'm happy to have this issue closed again.

          Show
          Todd Lipcon added a comment - I think it's a regression in that there now exists a unit test which fails 1 case in 0.20 and 3 cases in 0.21. The fact that TestDataBlockScanner passes doesn't necessarily mean there's no bug, just that the bug isn't exposed by a test. I don't think it's absolutely critical, but it seems worth fixing. If you disagree, I'm happy to have this issue closed again.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #101 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/101/)
          . Client-driven block verification not functioning. Contributed by Todd Lipcon.

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #101 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/101/ ) . Client-driven block verification not functioning. Contributed by Todd Lipcon.
          Hide
          Hudson added a comment -

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

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

          Integrated in Hadoop-Hdfs-trunk-Commit #179 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/179/)
          HDFS-922. Remove unnecessary semicolon added by that causes
          problems for Eclipse compilation.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #179 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/179/ ) HDFS-922 . Remove unnecessary semicolon added by that causes problems for Eclipse compilation.
          Hide
          Hudson added a comment -

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

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

          So, any verdict on this for 0.21? We should make a decision and close the JIRA either way. I'm +0 on commit but not critical.

          Show
          Todd Lipcon added a comment - So, any verdict on this for 0.21? We should make a decision and close the JIRA either way. I'm +0 on commit but not critical.
          Hide
          stack added a comment -

          I'm +1 on commit though non-critical. It has comprehensive test and it seems silly not picking up fixes (yeah, there is a freeze but its going on too long).

          Show
          stack added a comment - I'm +1 on commit though non-critical. It has comprehensive test and it seems silly not picking up fixes (yeah, there is a freeze but its going on too long).
          Hide
          Todd Lipcon added a comment -

          Re-resolving this since the 21 branch is going to die anyway (this was already put in trunk months back)

          Show
          Todd Lipcon added a comment - Re-resolving this since the 21 branch is going to die anyway (this was already put in trunk months back)

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development