Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-511

Redundant block searches in BlockManager.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Some searches for a block in blocksMap are redundant and should be eliminated.
      BlockManager.addStoredBlock() contain unnecessary explicit and hidden (inside markBlockAsCorrupt()) calls of blocksMap.getStoredBlock().

      1. BlockManager.patch
        14 kB
        Konstantin Shvachko
      2. BlockManager.patch
        14 kB
        Konstantin Shvachko
      3. BlockManager.patch
        14 kB
        Konstantin Shvachko

        Activity

        Hide
        Konstantin Shvachko added a comment -

        This has been committed.

        Show
        Konstantin Shvachko added a comment - This has been committed.
        Hide
        Hadoop QA added a comment -

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

        +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-vesta.apache.org/41/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/12415060/BlockManager.patch against trunk revision 799769. +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-vesta.apache.org/41/console This message is automatically generated.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #35 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/35/)
        . Remove redundant block searches in BlockManager. Contributed by Konstantin Shvachko.

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #35 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/35/ ) . Remove redundant block searches in BlockManager. Contributed by Konstantin Shvachko.
        Hide
        Hairong Kuang added a comment -

        +1 This looks good to me.

        Show
        Hairong Kuang added a comment - +1 This looks good to me.
        Hide
        Konstantin Shvachko added a comment -

        Don't see any rational reasons to keep the old method in BlockManager, but here it is.
        I had to rename it to findAndMarkBlockAsCorrupt() because if there are two methods with the same name it would be unclear which method is actually called for a stored block.

        Show
        Konstantin Shvachko added a comment - Don't see any rational reasons to keep the old method in BlockManager, but here it is. I had to rename it to findAndMarkBlockAsCorrupt() because if there are two methods with the same name it would be unclear which method is actually called for a stored block.
        Hide
        Hairong Kuang added a comment -

        > markBlockAsCorrupt() takes BlockInfo instead of Block as a parameter, thus there is no need to search for the block in the BlocksMap.
        I would prefer to keep BlockManager#markBlockAsCorrupt(Block, ...) to make BlockManager APIs consistently to take Block as a parameter. But to have a private method BlockManager#markBlockAsCorrupt(BlockInfo...) to avoid redundant lookup.

        Show
        Hairong Kuang added a comment - > markBlockAsCorrupt() takes BlockInfo instead of Block as a parameter, thus there is no need to search for the block in the BlocksMap. I would prefer to keep BlockManager#markBlockAsCorrupt(Block, ...) to make BlockManager APIs consistently to take Block as a parameter. But to have a private method BlockManager#markBlockAsCorrupt(BlockInfo...) to avoid redundant lookup.
        Hide
        Konstantin Shvachko added a comment -

        This fixes one findbug in the previous version of the patch.
        The test-patch targets complains there are no new tests, which is correct since this does not introduce any new behavior.

        [exec] There appear to be 154 release audit warnings before the patch and 154 release audit warnings after applying the patch.
        [exec] -1 overall.  
        [exec]     +1 @author.  The patch does not contain any @author tags.
        [exec]     -1 tests included.  The patch doesn't appear to include any new or modified tests.
        [exec]                         Please justify why no new tests are needed for this patch.
        [exec]                         Also please list what manual steps were performed to verify this patch.
        [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
        [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
        [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
        [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
        [exec] ======================================================================
        [exec] ======================================================================
        [exec]     Finished build.
        [exec] ======================================================================
        [exec] ======================================================================
        
        Show
        Konstantin Shvachko added a comment - This fixes one findbug in the previous version of the patch. The test-patch targets complains there are no new tests, which is correct since this does not introduce any new behavior. [exec] There appear to be 154 release audit warnings before the patch and 154 release audit warnings after applying the patch. [exec] -1 overall. [exec] +1 @author. The patch does not contain any @author tags. [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no new tests are needed for this patch. [exec] Also please list what manual steps were performed to verify this patch. [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] ====================================================================== [exec] ====================================================================== [exec] Finished build. [exec] ====================================================================== [exec] ======================================================================
        Hide
        Konstantin Shvachko added a comment -
        1. markBlockAsCorrupt() takes BlockInfo instead of Block as a parameter, thus there is no need to search for the block in the BlocksMap.
        2. Usage of block variable in addStoredBlock() is replaced by storedBlock wherever appropriate, so there is no need to explicitly search for the block once more.
        3. FSDirectory.addBlock() should not call getStoredBlock(block) because addINode() just before that already returns the block from the block map.
        4. BlockManager.addBlock(Block) is renamed to getValidLocations() because it does not add anything anywhere it just returns the locations of the block which are not in recentInvalidateSets.
        Show
        Konstantin Shvachko added a comment - markBlockAsCorrupt() takes BlockInfo instead of Block as a parameter, thus there is no need to search for the block in the BlocksMap . Usage of block variable in addStoredBlock() is replaced by storedBlock wherever appropriate, so there is no need to explicitly search for the block once more. FSDirectory.addBlock() should not call getStoredBlock(block) because addINode() just before that already returns the block from the block map. BlockManager.addBlock(Block) is renamed to getValidLocations() because it does not add anything anywhere it just returns the locations of the block which are not in recentInvalidateSets .

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development