jojochuang commented on a change in pull request #2964:
URL: https://github.com/apache/hadoop/pull/2964#discussion_r715810510
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -1091,6 +1091,24 @@ static void unprotectedUpdateCount(INodesInPath
inodesInPath,
}
}
+ /**
+ * check that all parent directories have quotas set.
+ */
+ static boolean verifyIsQuota(INodesInPath iip, int pos) {
Review comment:
looks like pos is always assigned iip.length() -1. So why not simplify
the logic and remove the pos?
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
##########
@@ -417,12 +434,32 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
// Ensure dst has quota to accommodate rename
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
- verifyQuotaForRename(fsd, srcIIP, dstIIP);
+ QuotaCounts srcPolicyCounts = new QuotaCounts.Builder(true).build();
+ QuotaCounts dstPolicyCounts = new QuotaCounts.Builder(true).build();
+ boolean srcIIPIsQuota = FSDirectory.verifyIsQuota(
+ srcIIP, srcIIP.length() - 1);
+ boolean dstIIPIsQuota = FSDirectory.verifyIsQuota(
+ dstIIP, dstIIP.length() - 1);
+ if (srcIIPIsQuota || dstIIPIsQuota) {
+ if (dstParent.getStoragePolicyID() ==
+ srcIIP.getLastINode().getStoragePolicyID()) {
+ srcPolicyCounts = srcIIP.getLastINode().computeQuotaUsage(bsps);
Review comment:
line 446 and 449 are the same. You may move them to line 444.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -1091,6 +1091,24 @@ static void unprotectedUpdateCount(INodesInPath
inodesInPath,
}
}
+ /**
+ * check that all parent directories have quotas set.
+ */
+ static boolean verifyIsQuota(INodesInPath iip, int pos) {
+ for (int i = (Math.min(pos, iip.length()) - 1); i > 0; i--) {
+ INode currNode = iip.getINode(i);
+ if (currNode == null) {
+ continue;
+ }
+ if (currNode.isDirectory()) {
+ if (currNode.isQuotaSet()) {
Review comment:
if the bottom most directory has quota set, does it imply all the
ancestor directories have quota set?
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
##########
@@ -417,12 +434,32 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
// Ensure dst has quota to accommodate rename
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
- verifyQuotaForRename(fsd, srcIIP, dstIIP);
Review comment:
this entire method unprotectedRenameTo() needs a refactor. It's way too
long too complex to understand.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java
##########
@@ -1091,6 +1091,24 @@ static void unprotectedUpdateCount(INodesInPath
inodesInPath,
}
}
+ /**
+ * check that all parent directories have quotas set.
+ */
+ static boolean verifyIsQuota(INodesInPath iip, int pos) {
Review comment:
the method name verifyIsQuota() isn't intuitive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
