Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2126 Improve Namenode startup time [umbrella task]
  3. HDFS-1295

Improve namenode restart times by short-circuiting the first block reports from datanodes

    Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: Federation Branch, 0.23.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The namenode restart is dominated by the performance of processing block reports. On a 2000 node cluster with 90 million blocks, block report processing takes 30 to 40 minutes. The namenode "diffs" the contents of the incoming block report with the contents of the blocks map, and then applies these diffs to the blocksMap, but in reality there is no need to compute the "diff" because this is the first block report from the datanode.

      This code change improves block report processing time by 300%.

      1. HDFS-1295_for_ymerge_v2.patch
        33 kB
        Matt Foley
      2. HDFS-1295_for_ymerge.patch
        32 kB
        Matt Foley
      3. HDFS-1295_delta_for_trunk.patch
        2 kB
        Matt Foley
      4. IBR_shortcut_v7atrunk.patch
        33 kB
        Matt Foley
      5. IBR_shortcut_v6atrunk.patch
        49 kB
        Matt Foley
      6. IBR_shortcut_v4atrunk.patch
        49 kB
        Matt Foley
      7. IBR_shortcut_v4atrunk.patch
        49 kB
        Matt Foley
      8. IBR_shortcut_v4atrunk.patch
        49 kB
        Matt Foley
      9. IBR_shortcut_v3atrunk.patch
        28 kB
        Matt Foley
      10. IBR_shortcut_v2a.patch
        29 kB
        Matt Foley
      11. shortCircuitBlockReport_1.txt
        10 kB
        dhruba borthakur

        Issue Links

          Activity

          Hide
          dhruba borthakur added a comment -

          This patch applied only to 0.20 and does two things:

          1. The first block report from a datanode is directly inserted into the blocksMap
          2. Optimize the countLiveNodes methods to be lightweight

          I extended the attached unit test to gather the average time to process the first block report from a datanode:

          Configuration without patch with patch
          40 nodes, 200K blocks each 232 sec 80 sec
          80 nodes, 200K blocks each 240 sec 82 sec
          Show
          dhruba borthakur added a comment - This patch applied only to 0.20 and does two things: 1. The first block report from a datanode is directly inserted into the blocksMap 2. Optimize the countLiveNodes methods to be lightweight I extended the attached unit test to gather the average time to process the first block report from a datanode: Configuration without patch with patch 40 nodes, 200K blocks each 232 sec 80 sec 80 nodes, 200K blocks each 240 sec 82 sec
          Hide
          Konstantin Shvachko added a comment -

          Could you please explain what happens with blocks in the first block report from a DN that do not belong to any file. Are they also inserted to the blocksMap?

          Show
          Konstantin Shvachko added a comment - Could you please explain what happens with blocks in the first block report from a DN that do not belong to any file. Are they also inserted to the blocksMap?
          Hide
          dhruba borthakur added a comment -

          > what happens with blocks in the first block report from a DN that do not belong to any file

          They are discarded as usual inside FSnamesystem.addStoredBlock(). This method checks if the block belongs to any inode, and if so, only then insert it into the blocksmap (this is existing code and is not modified by this patch).

          Show
          dhruba borthakur added a comment - > what happens with blocks in the first block report from a DN that do not belong to any file They are discarded as usual inside FSnamesystem.addStoredBlock(). This method checks if the block belongs to any inode, and if so, only then insert it into the blocksmap (this is existing code and is not modified by this patch).
          Hide
          Konstantin Shvachko added a comment -

          Not discarded, but rather ignored by addStoredBlock(). Therefore, with your patch the list of toInvalidate blocks will not be calculated and processed. And so deletion of replicas on data-nodes that don't belong to any file will be delayed until the next block report.
          My point is that there is trade-off, which you are not mentioning here, unless I missed something.
          The trade-off is: you will start faster, but space cleanup will be delayed.
          And the only way to fix it that I can see, is to send the second block report right after the first one, which will double the load on NN during startup.

          What is interesting though, the numbers you present show that construction of LinkedList in reportDiff() is time consuming, because the actual speedup is achieved because you reuse the same Block object, rather than creating them for each processed block as reportDiff() does.
          So, may be if we address this, we can optimize overall block processing, including the startup time.

          Btw, if it helps, you can use NNThroughputBenchmark to measure block report processing on a single node.

          Show
          Konstantin Shvachko added a comment - Not discarded, but rather ignored by addStoredBlock() . Therefore, with your patch the list of toInvalidate blocks will not be calculated and processed. And so deletion of replicas on data-nodes that don't belong to any file will be delayed until the next block report. My point is that there is trade-off, which you are not mentioning here, unless I missed something. The trade-off is: you will start faster, but space cleanup will be delayed. And the only way to fix it that I can see, is to send the second block report right after the first one, which will double the load on NN during startup. What is interesting though, the numbers you present show that construction of LinkedList in reportDiff() is time consuming, because the actual speedup is achieved because you reuse the same Block object, rather than creating them for each processed block as reportDiff() does. So, may be if we address this, we can optimize overall block processing, including the startup time. Btw, if it helps, you can use NNThroughputBenchmark to measure block report processing on a single node.
          Hide
          dhruba borthakur added a comment -

          > Not discarded, but rather ignored by addStoredBlock().

          Completely agree.

          > The trade-off is: you will start faster, but space cleanup will be delayed.

          That sounds like a fine tradeoff, don't you agree? If you like I can make it a configurable, but fewer the configuration parameters, better it is!

          Show
          dhruba borthakur added a comment - > Not discarded, but rather ignored by addStoredBlock(). Completely agree. > The trade-off is: you will start faster, but space cleanup will be delayed. That sounds like a fine tradeoff, don't you agree? If you like I can make it a configurable, but fewer the configuration parameters, better it is!
          Hide
          luoli added a comment -

          I think the trade-off is depends on how the hdfs been used. In our case, the High Availability problem is much more important than the cleanup delay because much applications depend on the hdfs to perform service 7 * 24. So I agree with Dhruba that this is a fine tradeoff.

          Show
          luoli added a comment - I think the trade-off is depends on how the hdfs been used. In our case, the High Availability problem is much more important than the cleanup delay because much applications depend on the hdfs to perform service 7 * 24. So I agree with Dhruba that this is a fine tradeoff.
          Hide
          dhruba borthakur added a comment -

          > because the actual speedup is achieved because you reuse the same Block object

          I did an experiment to not use the same Block object with the new shortcircuiting code, the performance gain remains the same.

          > The trade-off is: you will start faster, but space cleanup will be delayed.
          > That sounds like a fine tradeoff, don't you agree?

          Thanks luoli for your comments. Konstantin: do you agree that this tradeoff looks fine to you?

          Show
          dhruba borthakur added a comment - > because the actual speedup is achieved because you reuse the same Block object I did an experiment to not use the same Block object with the new shortcircuiting code, the performance gain remains the same. > The trade-off is: you will start faster, but space cleanup will be delayed. > That sounds like a fine tradeoff, don't you agree? Thanks luoli for your comments. Konstantin: do you agree that this tradeoff looks fine to you?
          Hide
          Matt Foley added a comment -

          Hi Dhruba, in HDFS-1147 you commented on the same topic, "If somebody can explain how this approach can be made to work in the presence of corrupt replicas, that will be great."

          This seems to be a different issue than the delay in cleaning up deleted blocks. Is it still relevant? Thanks.

          Show
          Matt Foley added a comment - Hi Dhruba, in HDFS-1147 you commented on the same topic, "If somebody can explain how this approach can be made to work in the presence of corrupt replicas, that will be great." This seems to be a different issue than the delay in cleaning up deleted blocks. Is it still relevant? Thanks.
          Hide
          dhruba borthakur added a comment -

          Hi Matt, thanks for the info about HDFS-1147. I closed it as a duplicate of this one.

          > this approach can be made to work in the presence of corrupt replicas, that will be great.

          can somebody pl elaborate on what possible issue a corrupt-replica might cause?

          Show
          dhruba borthakur added a comment - Hi Matt, thanks for the info about HDFS-1147 . I closed it as a duplicate of this one. > this approach can be made to work in the presence of corrupt replicas, that will be great. can somebody pl elaborate on what possible issue a corrupt-replica might cause?
          Hide
          Matt Foley added a comment -

          Hi Dhruba, I think this is a really important improvement to startup time, so I will try to get some contributors here to review it.
          Regarding this dangling issue, can you please describe any risks you see from corrupt replicas, with this shortcut in place? Thanks.

          Show
          Matt Foley added a comment - Hi Dhruba, I think this is a really important improvement to startup time, so I will try to get some contributors here to review it. Regarding this dangling issue, can you please describe any risks you see from corrupt replicas, with this shortcut in place? Thanks.
          Hide
          Matt Foley added a comment -

          Dhruba, I hope you don't mind if I submit an update for your excellent idea. This patch is based on Dhruba's idea, and includes the following:

          • Ported up to work on the current trunk.
          • Carefully studied the various shortcuts available based on (a) being in Startup Safe Mode, (b) being in non-startup Safe Mode, (c) being in the first Block Report from a given Datanode. Tried to do the right things to shortcut where we could, and not where we shouldn't. Do the right thing with corrupt blocks.
          • Refactored to simplify and clarify the Block Report processing code, so it will be easier to continue working on this area:
          • moved processReportedBlock() and reportDiff() methods into BlockManager with the other BR code;
          • added isReplicaCorrupt() and isBlockUnderConstruction() to replace complex code in processReportedBlock().
          • Changed the "toAdd" queue to use BlockInfo instead of Block, thereby avoiding lots of redundant blocksMap lookups. Still do a refresh lookup when needed, when the Block is actually a BlockInfoUnderConstruction that might have become completed.
          • Changed processReportedBlock() to have no side effects, by introducing a "toUC" queue so that Under-Construction blocks are treated consistently with the other types of reported block, in terms of creating to-do lists before processing the lists.

          This patch passes test-patch on my local system, except for "-1 tests included." The tests I used are in a new benchmark program for IBR (Initial Block Report) performance analysis which I've uploaded to HDFS-1732. Dhruba's attached unit test could also be used for a simpler test case.

          Show
          Matt Foley added a comment - Dhruba, I hope you don't mind if I submit an update for your excellent idea. This patch is based on Dhruba's idea, and includes the following: Ported up to work on the current trunk. Carefully studied the various shortcuts available based on (a) being in Startup Safe Mode, (b) being in non-startup Safe Mode, (c) being in the first Block Report from a given Datanode. Tried to do the right things to shortcut where we could, and not where we shouldn't. Do the right thing with corrupt blocks. Refactored to simplify and clarify the Block Report processing code, so it will be easier to continue working on this area: moved processReportedBlock() and reportDiff() methods into BlockManager with the other BR code; added isReplicaCorrupt() and isBlockUnderConstruction() to replace complex code in processReportedBlock(). Changed the "toAdd" queue to use BlockInfo instead of Block, thereby avoiding lots of redundant blocksMap lookups. Still do a refresh lookup when needed, when the Block is actually a BlockInfoUnderConstruction that might have become completed. Changed processReportedBlock() to have no side effects, by introducing a "toUC" queue so that Under-Construction blocks are treated consistently with the other types of reported block, in terms of creating to-do lists before processing the lists. This patch passes test-patch on my local system, except for "-1 tests included." The tests I used are in a new benchmark program for IBR (Initial Block Report) performance analysis which I've uploaded to HDFS-1732 . Dhruba's attached unit test could also be used for a simpler test case.
          Hide
          dhruba borthakur added a comment -

          Good stuf Matt, thanks for doing this.

          Show
          dhruba borthakur added a comment - Good stuf Matt, thanks for doing 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/12472903/IBR_shortcut_v2a.patch
          against trunk revision 1079069.

          +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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.server.balancer.TestBalancer
          org.apache.hadoop.hdfs.server.datanode.TestBlockReplacement
          org.apache.hadoop.hdfs.server.namenode.TestBlocksWithNotEnoughRacks
          org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS
          org.apache.hadoop.hdfs.server.namenode.TestNodeCount
          org.apache.hadoop.hdfs.TestDatanodeBlockScanner

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/239//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/239//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/239//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/12472903/IBR_shortcut_v2a.patch against trunk revision 1079069. +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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.server.balancer.TestBalancer org.apache.hadoop.hdfs.server.datanode.TestBlockReplacement org.apache.hadoop.hdfs.server.namenode.TestBlocksWithNotEnoughRacks org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS org.apache.hadoop.hdfs.server.namenode.TestNodeCount org.apache.hadoop.hdfs.TestDatanodeBlockScanner -1 contrib tests. The patch failed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/239//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/239//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/239//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

          +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 patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

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

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

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

          -1 system test framework. The patch failed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/310//testReport/
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/310//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/12472903/IBR_shortcut_v2a.patch against trunk revision 1087900. +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 patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: -1 contrib tests. The patch failed contrib unit tests. -1 system test framework. The patch failed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/310//testReport/ Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/310//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          Updated to current trunk.

          Show
          Matt Foley added a comment - Updated to current trunk.
          Hide
          Hadoop QA added a comment -

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

          +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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.server.balancer.TestBalancer
          org.apache.hadoop.hdfs.server.datanode.TestBlockReplacement
          org.apache.hadoop.hdfs.server.datanode.TestBlockReport
          org.apache.hadoop.hdfs.server.namenode.TestBlocksWithNotEnoughRacks
          org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS
          org.apache.hadoop.hdfs.server.namenode.TestNodeCount
          org.apache.hadoop.hdfs.TestDatanodeBlockScanner
          org.apache.hadoop.hdfs.TestFileConcurrentReader

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/311//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/311//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/311//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/12475272/IBR_shortcut_v3atrunk.patch against trunk revision 1087900. +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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.server.balancer.TestBalancer org.apache.hadoop.hdfs.server.datanode.TestBlockReplacement org.apache.hadoop.hdfs.server.datanode.TestBlockReport org.apache.hadoop.hdfs.server.namenode.TestBlocksWithNotEnoughRacks org.apache.hadoop.hdfs.server.namenode.TestBlockTokenWithDFS org.apache.hadoop.hdfs.server.namenode.TestNodeCount org.apache.hadoop.hdfs.TestDatanodeBlockScanner org.apache.hadoop.hdfs.TestFileConcurrentReader -1 contrib tests. The patch failed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/311//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/311//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/311//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          Small change in IBR_shortcut to correctly adapt to use of isPopulatingReplQueues() in current trunk.

          Also included changes to the six unit tests that were failing. Several timed out in Hudson without useful error logs. These I examined, added timeouts and throws of TimeoutException, and useful log info; in some cases I also fixed what appeared by code inspection to be the source of the problem. They are likely to need another pass, but submitting them through Hudson to see. (None fail on my local test environment.)

          Show
          Matt Foley added a comment - Small change in IBR_shortcut to correctly adapt to use of isPopulatingReplQueues() in current trunk. Also included changes to the six unit tests that were failing. Several timed out in Hudson without useful error logs. These I examined, added timeouts and throws of TimeoutException, and useful log info; in some cases I also fixed what appeared by code inspection to be the source of the problem. They are likely to need another pass, but submitting them through Hudson to see. (None fail on my local test environment.)
          Hide
          Matt Foley added a comment -

          Toggling "submit patch" state to trigger auto test.

          Show
          Matt Foley added a comment - Toggling "submit patch" state to trigger auto test.
          Hide
          Matt Foley added a comment -

          Hudson auto-test is refusing to trigger.
          Will try once more with full undo/redo.

          Show
          Matt Foley added a comment - Hudson auto-test is refusing to trigger. Will try once more with full undo/redo.
          Hide
          Matt Foley added a comment -

          Resubmit to try to trigger auto-test.

          Show
          Matt Foley added a comment - Resubmit to try to trigger auto-test.
          Hide
          Matt Foley added a comment -

          Trying again with status "Resume Workflow". Sorry for the spam.

          Show
          Matt Foley added a comment - Trying again with status "Resume Workflow". Sorry for the spam.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 15 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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.TestDatanodeBlockScanner
          org.apache.hadoop.hdfs.TestFileConcurrentReader

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/346//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/346//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/346//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/12476063/IBR_shortcut_v4atrunk.patch against trunk revision 1091131. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestDatanodeBlockScanner org.apache.hadoop.hdfs.TestFileConcurrentReader -1 contrib tests. The patch failed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/346//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/346//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HDFS-Build/346//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          These bugs all failed false positive due to timing changes or other issues not actually bugs in the implementation of HDFS-1295. Some of them, like HDFS-1806 (TestBlockReport.blockReport_08() and _09()) have been persistent false positives for many submissions over the past weeks.

          They are all rather small fixes. I've split the patches out to the individual bug reports for ease of examination, but if it is okay with everyone I'll just make a single submission under the HDFS-1295 banner.

          Show
          Matt Foley added a comment - These bugs all failed false positive due to timing changes or other issues not actually bugs in the implementation of HDFS-1295 . Some of them, like HDFS-1806 (TestBlockReport.blockReport_08() and _09()) have been persistent false positives for many submissions over the past weeks. They are all rather small fixes. I've split the patches out to the individual bug reports for ease of examination, but if it is okay with everyone I'll just make a single submission under the HDFS-1295 banner.
          Hide
          Matt Foley added a comment -

          Response to Hudson QA auto-test of 12/Apr/11 00:14 (PreCommit-HDFS-Build/346):

          Of the four failing tests, two are our old friends hdfsproxy, unrelated to this Jira.
          One (TestFileConcurrentReader.testUnfinishedBlockCRCErrorNormalTransferVerySmallWrite) is three days old, and unrelated to this Jira.

          The fourth, TestDatanodeBlockScanner.testTruncatedBlockReport, needs to be investigated, but is likely to also be a test issue rather than a bug in the patch.
          Also, I found that the patch for HDFS-1829 should be modified to use readLock() instead of synchronized(namesystem).
          These are likely to be small changes, while the main patch to BlockManager is fairly large, and likely to be unchanged by the fix to TestDatanodeBlockScanner.testTruncatedBlockReport.

          Therefore, please consider starting code review if you are so inclined, so that we can complete this submission soon. Thank you very much.

          Show
          Matt Foley added a comment - Response to Hudson QA auto-test of 12/Apr/11 00:14 (PreCommit-HDFS-Build/346): Of the four failing tests, two are our old friends hdfsproxy, unrelated to this Jira. One (TestFileConcurrentReader.testUnfinishedBlockCRCErrorNormalTransferVerySmallWrite) is three days old, and unrelated to this Jira. The fourth, TestDatanodeBlockScanner.testTruncatedBlockReport, needs to be investigated, but is likely to also be a test issue rather than a bug in the patch. Also, I found that the patch for HDFS-1829 should be modified to use readLock() instead of synchronized(namesystem). These are likely to be small changes, while the main patch to BlockManager is fairly large, and likely to be unchanged by the fix to TestDatanodeBlockScanner.testTruncatedBlockReport. Therefore, please consider starting code review if you are so inclined, so that we can complete this submission soon. Thank you very much.
          Hide
          Matt Foley added a comment -

          The unit test files cited in HDFS-1806, HDFS-1808, HDFS-1827, HDFS-1828, and HDFS-1829, were all found to be failing for reasons unrelated to the patch for this bug, HDFS-1295. I'm submitting fixes for each of them, but I've detached them from this bug. Please review them separately. Thanks.

          Show
          Matt Foley added a comment - The unit test files cited in HDFS-1806 , HDFS-1808 , HDFS-1827 , HDFS-1828 , and HDFS-1829 , were all found to be failing for reasons unrelated to the patch for this bug, HDFS-1295 . I'm submitting fixes for each of them, but I've detached them from this bug. Please review them separately. Thanks.
          Hide
          Matt Foley added a comment -

          This is a final candidate patch. Please review. It modifies three pairs of files:

          BlockManager and DatanodeDescriptor mods are the core changes implementing Dhruba's idea to shortcut Initial Block Reports.

          DataNode and FSNamesystem small mods improve the logging of Block Report processing on both NN and DN, so it is easier to see the improvement.

          TestDatanodeBlockScanner and DFSTestUtil mods solve several problems with TestDatanodeBlockScanner, including (a) an outright bug in blockCorruptionRecoveryPolicy() which was causing failure of testBlockCorruptionRecoveryPolicy2(); (b) make the test run much faster; and (c) make the test print useful information in event of a failure.

          It is noted that TestDatanodeBlockScanner.testTruncatedBlockReport() showed a bug in the v4 implementation of IBR shortcuts. As a result, I had to put corrupt block processing back into the IBR shortcut path, rather than ignoring them. This should not cause much change in the perf improvement, since there should be very small numbers of corrupt blocks.

          Show
          Matt Foley added a comment - This is a final candidate patch. Please review. It modifies three pairs of files: BlockManager and DatanodeDescriptor mods are the core changes implementing Dhruba's idea to shortcut Initial Block Reports. DataNode and FSNamesystem small mods improve the logging of Block Report processing on both NN and DN, so it is easier to see the improvement. TestDatanodeBlockScanner and DFSTestUtil mods solve several problems with TestDatanodeBlockScanner, including (a) an outright bug in blockCorruptionRecoveryPolicy() which was causing failure of testBlockCorruptionRecoveryPolicy2(); (b) make the test run much faster; and (c) make the test print useful information in event of a failure. It is noted that TestDatanodeBlockScanner.testTruncatedBlockReport() showed a bug in the v4 implementation of IBR shortcuts. As a result, I had to put corrupt block processing back into the IBR shortcut path, rather than ignoring them. This should not cause much change in the perf improvement, since there should be very small numbers of corrupt blocks.
          Hide
          Suresh Srinivas added a comment -

          Comments:

          1. Minor: TestDatanodeBlockScanner - could you LOG.info or LOG.debug instead of System.out
          2. Is it worth retaining the printDatanodeAssignments() and printDatanodeBlockReports(), which probably was added as debug code?
          3. In the test we have TIMEOUT to be 20s. Is it reasonably long enough so that tests do not fail?
          4. In block report time, why is the report creation time not included in metrics?
          Show
          Suresh Srinivas added a comment - Comments: Minor: TestDatanodeBlockScanner - could you LOG.info or LOG.debug instead of System.out Is it worth retaining the printDatanodeAssignments() and printDatanodeBlockReports(), which probably was added as debug code? In the test we have TIMEOUT to be 20s. Is it reasonably long enough so that tests do not fail? In block report time, why is the report creation time not included in metrics?
          Hide
          Matt Foley added a comment -

          The TestDatanodeBlockScanner and DFSTestUtil mods have been moved to bugs HDFS-1855, HDFS-1856, and HDFS-1854. Suresh's suggestions #1 and #2 regarding TestDatanodeBlockScanner were incorporated.

          #3: Yes, the 20sec timeout is plenty long, because the config params were modified to make the testcase run much faster.

          #4: In block report time, the report creation time is included in metrics; see line 3156 of FSNamesystem. However, it is still useful to provide the log message as given, because the metrics are bucketed and not possible to cross-correlate with a particular datanode. Also, the original log message (line 3137 of the unmodified file) stated that a report from a given datanode was processed but didn't give the processing time.

          Show
          Matt Foley added a comment - The TestDatanodeBlockScanner and DFSTestUtil mods have been moved to bugs HDFS-1855 , HDFS-1856 , and HDFS-1854 . Suresh's suggestions #1 and #2 regarding TestDatanodeBlockScanner were incorporated. #3: Yes, the 20sec timeout is plenty long, because the config params were modified to make the testcase run much faster. #4: In block report time, the report creation time is included in metrics; see line 3156 of FSNamesystem. However, it is still useful to provide the log message as given, because the metrics are bucketed and not possible to cross-correlate with a particular datanode. Also, the original log message (line 3137 of the unmodified file) stated that a report from a given datanode was processed but didn't give the processing time.
          Hide
          dhruba borthakur added a comment -

          hi suresh, the block-report creation time is included in the metric.

          Show
          dhruba borthakur added a comment - hi suresh, the block-report creation time is included in the metric.
          Hide
          Hadoop QA added a comment -

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

          +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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.TestFileConcurrentReader

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/399//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/399//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/399//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/12476974/IBR_shortcut_v7atrunk.patch against trunk revision 1095789. +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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestFileConcurrentReader +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/399//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/399//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/399//console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          > the block-report creation time is included in the metric.
          Sorry I missed this.

          +1 for the patch.

          Show
          Suresh Srinivas added a comment - > the block-report creation time is included in the metric. Sorry I missed this. +1 for the patch.
          Hide
          dhruba borthakur added a comment -

          Code looks good to me, +1

          Show
          dhruba borthakur added a comment - Code looks good to me, +1
          Hide
          Matt Foley added a comment -

          Thank you, gentlemen.

          The failed unit test TestFileConcurrentReader is unrelated to this patch.

          Ready to commit.

          Show
          Matt Foley added a comment - Thank you, gentlemen. The failed unit test TestFileConcurrentReader is unrelated to this patch. Ready to commit.
          Hide
          Suresh Srinivas added a comment -

          I committed the patch. Thank you Matt.

          Show
          Suresh Srinivas added a comment - I committed the patch. Thank you Matt.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #607 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/607/)
          HDFS-1295. Improve namenode restart times by short-circuiting the first block reports from datanodes. Contributed by Matt Foley.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #607 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/607/ ) HDFS-1295 . Improve namenode restart times by short-circuiting the first block reports from datanodes. Contributed by Matt Foley.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #650 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/650/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #650 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/650/ )
          Hide
          Matt Foley added a comment -

          This patch needs to be ported to yahoo-merge branch.

          Also, the integration of HDFS-1052 into trunk (r1097905 on Apr 29) reverted a portion of this patch in DataNode.java in trunk.

          Show
          Matt Foley added a comment - This patch needs to be ported to yahoo-merge branch. Also, the integration of HDFS-1052 into trunk (r1097905 on Apr 29) reverted a portion of this patch in DataNode.java in trunk.
          Hide
          Matt Foley added a comment -

          Delta patch to re-integrate HDFS-1295 with DataNode.java in trunk.

          Show
          Matt Foley added a comment - Delta patch to re-integrate HDFS-1295 with DataNode.java in trunk.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12481878/HDFS-1295_delta_for_trunk.patch
          against trunk revision 1133476.

          +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 (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.cli.TestHDFSCLI

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/744//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/744//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/744//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/12481878/HDFS-1295_delta_for_trunk.patch against trunk revision 1133476. +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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.cli.TestHDFSCLI +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/744//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/744//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/744//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          Attaching patch ported to yahoo-merge branch.

          Turning off "Patch Available" so Hudson doesn't try to run test-patch on non-trunk patch.

          Show
          Matt Foley added a comment - Attaching patch ported to yahoo-merge branch. Turning off "Patch Available" so Hudson doesn't try to run test-patch on non-trunk patch.
          Hide
          Suresh Srinivas added a comment -

          +1 for the trunk patch

          Show
          Suresh Srinivas added a comment - +1 for the trunk patch
          Hide
          Suresh Srinivas added a comment -

          +1 for the yahoo-merge patch also

          Show
          Suresh Srinivas added a comment - +1 for the yahoo-merge patch also
          Hide
          Matt Foley added a comment -

          Response to test-patch:
          -1 core tests: TestHDFSCL failure is unrelated.
          -1 tests included: This is simply a completion of the previously approved patch.

          Committed HDFS-1295_delta_for_trunk.patch to trunk.

          Show
          Matt Foley added a comment - Response to test-patch: -1 core tests: TestHDFSCL failure is unrelated. -1 tests included: This is simply a completion of the previously approved patch. Committed HDFS-1295 _delta_for_trunk.patch to trunk.
          Hide
          Matt Foley added a comment -

          Turns out HDFS-1295 is dependent on HDFS-900. Merged HDFS-900 to yahoo-merge, but now need a slightly modified port of HDFS-1295. Attached.

          Show
          Matt Foley added a comment - Turns out HDFS-1295 is dependent on HDFS-900 . Merged HDFS-900 to yahoo-merge, but now need a slightly modified port of HDFS-1295 . Attached.
          Hide
          Suresh Srinivas added a comment -

          +1 for the patch.

          Show
          Suresh Srinivas added a comment - +1 for the patch.
          Hide
          Matt Foley added a comment -

          Committed to yahoo-merge branch. Thanks for the review, Suresh!

          Show
          Matt Foley added a comment - Committed to yahoo-merge branch. Thanks for the review, Suresh!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #746 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/746/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #746 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/746/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #699 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/699/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #699 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/699/ )

            People

            • Assignee:
              Matt Foley
              Reporter:
              dhruba borthakur
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development