From 8ae033d1a3d65281d3ad1f07f430e74d3a01b1b9 Mon Sep 17 00:00:00 2001 From: Stephen O'Donnell Date: Sun, 17 Apr 2022 13:05:11 +0100 Subject: [PATCH] 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 dbeeee03639f41a022dd07d5fc04e3aa65a94b5f) --- .../hdfs/server/namenode/FSDirAttrOp.java | 31 +++++++++++++------ .../hdfs/server/namenode/FSNamesystem.java | 11 ++++--- .../hadoop/hdfs/TestSetrepIncreasing.java | 6 ++++ 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirAttrOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirAttrOp.java index 5c347655fc..9654f599ad 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirAttrOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirAttrOp.java @@ -49,6 +49,11 @@ import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_QUOTA_BY_STORAGETYPE_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 { @@ -129,11 +134,11 @@ static FileStatus setTimes( 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); @@ -141,16 +146,14 @@ static boolean setReplication( 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, @@ -374,7 +377,7 @@ static INodeDirectory unprotectedSetQuota( return dirNode; } - static BlockInfo[] unprotectedSetReplication( + static SetRepStatus unprotectedSetReplication( FSDirectory fsd, INodesInPath iip, short replication) throws QuotaExceededException, UnresolvedLinkException, SnapshotAccessControlException, UnsupportedActionException { @@ -384,12 +387,20 @@ static BlockInfo[] unprotectedSetReplication( 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 @@ -420,7 +431,7 @@ static BlockInfo[] unprotectedSetReplication( oldBR, iip.getPath()); } } - return file.getBlocks(); + return SetRepStatus.SUCCESS; } static void unprotectedSetStoragePolicy(FSDirectory fsd, BlockManager bm, diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index 3ce8a80be6..c74286ece0 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -2370,7 +2370,7 @@ void createSymlink(String target, String link, 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(); FSPermissionChecker.setOperationType(operationName); @@ -2379,7 +2379,7 @@ boolean setReplication(final String src, final short replication) 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); } finally { writeUnlock(operationName); @@ -2388,11 +2388,12 @@ boolean setReplication(final String src, final short replication) logAuditEvent(false, operationName, src); throw e; } - 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; } /** diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java index 497d450de2..d89b077763 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java @@ -82,6 +82,12 @@ static void setrep(int fromREP, int toREP, boolean simulatedStorage) throws IOEx 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);