HDFS-16531. Avoid setReplication writing an edit record if old replication equals the new value (#4148). Contributed by Stephen O'Donnell.

(cherry picked from commit dbeeee0363)

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

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

@@ -50,6 +50,11 @@ import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_QUOTA_BY_STORAGETYPE_ENAB
 import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_STORAGE_POLICY_ENABLED_KEY;
 
 public class FSDirAttrOp {
+
+  protected enum SetRepStatus {
+    UNCHANGED, INVALID, SUCCESS
+  }
+
   static FileStatus setPermission(
       FSDirectory fsd, FSPermissionChecker pc, final String src,
       FsPermission permission) throws IOException {
@@ -130,11 +135,11 @@ public class FSDirAttrOp {
     return fsd.getAuditFileInfo(iip);
   }
 
-  static boolean setReplication(
+  static SetRepStatus setReplication(
       FSDirectory fsd, FSPermissionChecker pc, BlockManager bm, String src,
       final short replication) throws IOException {
     bm.verifyReplication(src, replication, null);
-    final boolean isFile;
+    final SetRepStatus status;
     fsd.writeLock();
     try {
       final INodesInPath iip = fsd.resolvePath(pc, src, DirOp.WRITE);
@@ -142,16 +147,14 @@ public class FSDirAttrOp {
         fsd.checkPathAccess(pc, iip, FsAction.WRITE);
       }
 
-      final BlockInfo[] blocks = unprotectedSetReplication(fsd, iip,
-                                                           replication);
-      isFile = blocks != null;
-      if (isFile) {
+      status = unprotectedSetReplication(fsd, iip, replication);
+      if (status == SetRepStatus.SUCCESS) {
         fsd.getEditLog().logSetReplication(iip.getPath(), replication);
       }
     } finally {
       fsd.writeUnlock();
     }
-    return isFile;
+    return status;
   }
 
   static FileStatus unsetStoragePolicy(FSDirectory fsd, FSPermissionChecker pc,
@@ -376,7 +379,7 @@ public class FSDirAttrOp {
     }
   }
 
-  static BlockInfo[] unprotectedSetReplication(
+  static SetRepStatus unprotectedSetReplication(
       FSDirectory fsd, INodesInPath iip, short replication)
       throws QuotaExceededException, UnresolvedLinkException,
       SnapshotAccessControlException, UnsupportedActionException {
@@ -386,12 +389,20 @@ public class FSDirAttrOp {
     final INode inode = iip.getLastINode();
     if (inode == null || !inode.isFile() || inode.asFile().isStriped()) {
       // TODO we do not support replication on stripe layout files yet
-      return null;
+      // We return invalid here, so we skip writing an edit, but also write an
+      // unsuccessful audit message.
+      return SetRepStatus.INVALID;
     }
 
     INodeFile file = inode.asFile();
     // Make sure the directory has sufficient quotas
     short oldBR = file.getPreferredBlockReplication();
+    if (oldBR == replication) {
+      // No need to do anything as the requested rep factor is the same as
+      // existing. Returning UNCHANGED to we can skip writing edits, but still
+      // log a successful audit message.
+      return SetRepStatus.UNCHANGED;
+    }
 
     long size = file.computeFileSize(true, true);
     // Ensure the quota does not exceed
@@ -422,7 +433,7 @@ public class FSDirAttrOp {
                              oldBR, iip.getPath());
       }
     }
-    return file.getBlocks();
+    return SetRepStatus.SUCCESS;
   }
 
   static void unprotectedSetStoragePolicy(FSDirectory fsd, BlockManager bm,

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

@@ -2239,14 +2239,14 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean,
   boolean setReplication(final String src, final short replication)
       throws IOException {
     final String operationName = "setReplication";
-    boolean success = false;
+    FSDirAttrOp.SetRepStatus status;
     checkOperation(OperationCategory.WRITE);
     final FSPermissionChecker pc = getPermissionChecker();
     writeLock();
     try {
       checkOperation(OperationCategory.WRITE);
       checkNameNodeSafeMode("Cannot set replication for " + src);
-      success = FSDirAttrOp.setReplication(dir, pc, blockManager, src,
+      status = FSDirAttrOp.setReplication(dir, pc, blockManager, src,
           replication);
     } catch (AccessControlException e) {
       logAuditEvent(false, operationName, src);
@@ -2254,11 +2254,12 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean,
     } finally {
       writeUnlock(operationName);
     }
-    if (success) {
+    if (status == FSDirAttrOp.SetRepStatus.SUCCESS) {
       getEditLog().logSync();
-      logAuditEvent(true, operationName, src);
     }
-    return success;
+    logAuditEvent(status != FSDirAttrOp.SetRepStatus.INVALID,
+        operationName, src);
+    return status != FSDirAttrOp.SetRepStatus.INVALID;
   }
 
   /**

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java

@@ -82,6 +82,12 @@ public class TestSetrepIncreasing {
   public void testSetrepIncreasing() throws IOException {
     setrep(3, 7, false);
   }
+
+  @Test(timeout=120000)
+  public void testSetrepSameRepValue() throws IOException {
+    setrep(3, 3, false);
+  }
+
   @Test(timeout=120000)
   public void testSetrepIncreasingSimulatedStorage() throws IOException {
     setrep(3, 7, true);