HDFS-8071. Redundant checkFileProgress() in PART II of getAdditionalBlock(). Contributed by Konstantin Shvachko.

Author: shvachko

hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

@@ -465,6 +465,9 @@ Release 2.7.0 - UNRELEASED
     HDFS-7811. Avoid recursive call getStoragePolicyID in
     INodeFile#computeQuotaUsage. (Xiaoyu Yao and jing9)
 
+    HDFS-8071. Redundant checkFileProgress() in PART II of getAdditionalBlock().
+    (shv)
+
   OPTIMIZATIONS
 
     HDFS-7454. Reduce memory footprint for AclEntries in NameNode.

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

@@ -3032,6 +3032,10 @@ LocatedBlock getAdditionalBlock(String src, long fileId, String clientName,
       FileState fileState = analyzeFileState(
           src, fileId, clientName, previous, onRetryBlock);
       final INodeFile pendingFile = fileState.inode;
+      // Check if the penultimate block is minimally replicated
+      if (!checkFileProgress(src, pendingFile, false)) {
+        throw new NotReplicatedYetException("Not replicated yet: " + src);
+      }
       src = fileState.path;
 
       if (onRetryBlock[0] != null && onRetryBlock[0].getLocations().length > 0) {
@@ -3244,11 +3248,6 @@ FileState analyzeFileState(String src,
             "last block in file " + lastBlockInFile);
       }
     }
-
-    // Check if the penultimate block is minimally replicated
-    if (!checkFileProgress(src, pendingFile, false)) {
-      throw new NotReplicatedYetException("Not replicated yet: " + src);
-    }
     return new FileState(pendingFile, src, iip);
   }
 
@@ -3551,28 +3550,23 @@ Block createNewBlock() throws IOException {
    * replicated.  If not, return false. If checkall is true, then check
    * all blocks, otherwise check only penultimate block.
    */
-  private boolean checkFileProgress(String src, INodeFile v, boolean checkall) {
-    readLock();
-    try {
-      if (checkall) {
-        // check all blocks of the file.
-        for (BlockInfoContiguous block: v.getBlocks()) {
-          if (!isCompleteBlock(src, block, blockManager.minReplication)) {
-            return false;
-          }
-        }
-      } else {
-        // check the penultimate block of this file
-        BlockInfoContiguous b = v.getPenultimateBlock();
-        if (b != null
-            && !isCompleteBlock(src, b, blockManager.minReplication)) {
+  boolean checkFileProgress(String src, INodeFile v, boolean checkall) {
+    if (checkall) {
+      // check all blocks of the file.
+      for (BlockInfoContiguous block: v.getBlocks()) {
+        if (!isCompleteBlock(src, block, blockManager.minReplication)) {
           return false;
         }
       }
-      return true;
-    } finally {
-      readUnlock();
+    } else {
+      // check the penultimate block of this file
+      BlockInfoContiguous b = v.getPenultimateBlock();
+      if (b != null
+          && !isCompleteBlock(src, b, blockManager.minReplication)) {
+        return false;
+      }
     }
+    return true;
   }
 
   private static boolean isCompleteBlock(String src, BlockInfoContiguous b, int minRepl) {

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAddBlockRetry.java

@@ -24,6 +24,7 @@
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.spy;
 
+import java.io.IOException;
 import java.lang.reflect.Field;
 import java.util.EnumSet;
 import java.util.HashSet;
@@ -90,7 +91,7 @@ public void tearDown() throws Exception {
   public void testRetryAddBlockWhileInChooseTarget() throws Exception {
     final String src = "/testRetryAddBlockWhileInChooseTarget";
 
-    FSNamesystem ns = cluster.getNamesystem();
+    final FSNamesystem ns = cluster.getNamesystem();
     BlockManager spyBM = spy(ns.getBlockManager());
     final NamenodeProtocols nn = cluster.getNameNodeRpc();
 
@@ -107,11 +108,15 @@ public DatanodeStorageInfo[] answer(InvocationOnMock invocation)
         LOG.info("chooseTarget for " + src);
         DatanodeStorageInfo[] ret =
             (DatanodeStorageInfo[]) invocation.callRealMethod();
+        assertTrue("Penultimate block must be complete",
+            checkFileProgress(src, false));
         count++;
         if(count == 1) { // run second addBlock()
           LOG.info("Starting second addBlock for " + src);
           nn.addBlock(src, "clientName", null, null,
               INodeId.GRANDFATHER_INODE_ID, null);
+          assertTrue("Penultimate block must be complete",
+              checkFileProgress(src, false));
           LocatedBlocks lbs = nn.getBlockLocations(src, 0, Long.MAX_VALUE);
           assertEquals("Must be one block", 1, lbs.getLocatedBlocks().size());
           lb2 = lbs.get(0);
@@ -142,6 +147,16 @@ public DatanodeStorageInfo[] answer(InvocationOnMock invocation)
     assertEquals("Blocks are not equal", lb1.getBlock(), lb2.getBlock());
   }
 
+  boolean checkFileProgress(String src, boolean checkall) throws IOException {
+    final FSNamesystem ns = cluster.getNamesystem();
+    ns.readLock();
+    try {
+      return ns.checkFileProgress(src, ns.dir.getINode(src).asFile(), checkall);
+    } finally {
+      ns.readUnlock();
+    }
+  }
+
   /*
    * Since NameNode will not persist any locations of the block, addBlock()
    * retry call after restart NN should re-select the locations and return to