Skip to content

HDFS-16531. Avoid setReplication writing an edit record if old replication equals the new value #4148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Hexiaoqiao merged 1 commit into from
Apr 17, 2022
Merged

HDFS-16531. Avoid setReplication writing an edit record if old replication equals the new value #4148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions ...project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirAttrOp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,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 {
Expand Down Expand Up @@ -134,28 +139,26 @@ 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);
if (fsd.isPermissionEnabled()) {
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,
Expand Down Expand Up @@ -381,7 +384,7 @@ static INodeDirectory unprotectedSetQuota(
return dirNode;
}

static BlockInfo[] unprotectedSetReplication(
static SetRepStatus unprotectedSetReplication(
FSDirectory fsd, INodesInPath iip, short replication)
throws QuotaExceededException, UnresolvedLinkException,
SnapshotAccessControlException, UnsupportedActionException {
Expand All @@ -391,12 +394,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
Expand Down Expand Up @@ -427,7 +438,7 @@ static BlockInfo[] unprotectedSetReplication(
oldBR, iip.getPath());
}
}
return file.getBlocks();
return SetRepStatus.SUCCESS;
}

static void unprotectedSetStoragePolicy(FSDirectory fsd, BlockManager bm,
Expand Down
11 changes: 6 additions & 5 deletions ...roject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2448,7 +2448,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);
Expand All @@ -2457,7 +2457,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, getLockReportInfoSupplier(src));
Expand All @@ -2466,11 +2466,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,
Copy link
Contributor

@jojochuang jojochuang Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this change, if success, it logs a true.
Now if status == FSDirAttrOp.SetRepStatus.SUCCESS, it logs a false?

Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this does not changes the prior log except additional false log when setReplication failed.
a. if status == FSDirAttrOp.SetRepStatus.SUCCESS, it logs a true since status != FSDirAttrOp.SetRepStatus.INVALID;
b. if status == FSDirAttrOp.SetRepStatus.UNCHANGED, it also logs a true which is same as before.
c. if status == FSDirAttrOp.SetRepStatus.INVALID, it logs a false which is additional compare to before logic.

Copy link
Contributor Author

@sodonnel sodonnel Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it should only log false if INVALID != SetRepStatus.INVALID -> this would return false. SUCCESS or UNCHANGED would log true. The change here, is that failures (INVALID) were previously not logged in the audits, which was a bug IMO. Other operations seem to audit failures in general.

Copy link
Contributor

@hemanthboyina hemanthboyina Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, in most of the operations whenever there is an ACE we log audit as false and then we throw the exception, for the other exceptions like IO the exception was thrown directly without the operation getting logged

though i agree with @sodonnel and @Hexiaoqiao here, we can log the audit as false when the status == FSDirAttrOp.SetRepStatus.INVALID

operationName, src);
return status != FSDirAttrOp.SetRepStatus.INVALID;
}

/**
Expand Down
6 changes: 6 additions & 0 deletions ...p-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestSetrepIncreasing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down