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

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

@@ -574,6 +574,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 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean,
       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 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean,
             "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);
   }
 
@@ -3550,21 +3549,17 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean,
    * 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) {
-        return blockManager.checkBlocksProperlyReplicated(src, v
-            .getBlocks());
-      } else {
-        // check the penultimate block of this file
-        BlockInfoContiguous b = v.getPenultimateBlock();
-        return b == null ||
-            blockManager.checkBlocksProperlyReplicated(
-                src, new BlockInfoContiguous[] { b });
-      }
-    } finally {
-      readUnlock();
+  boolean checkFileProgress(String src, INodeFile v, boolean checkall) {
+    assert hasReadLock();
+    if (checkall) {
+      return blockManager.checkBlocksProperlyReplicated(src, v
+          .getBlocks());
+    } else {
+      // check the penultimate block of this file
+      BlockInfoContiguous b = v.getPenultimateBlock();
+      return b == null ||
+          blockManager.checkBlocksProperlyReplicated(
+              src, new BlockInfoContiguous[] { b });
     }
   }
 

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

@@ -24,6 +24,7 @@ import static org.junit.Assert.assertTrue;
 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 class TestAddBlockRetry {
   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 class TestAddBlockRetry {
         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 class TestAddBlockRetry {
     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