Copilot commented on code in PR #7989:
URL: https://github.com/apache/hadoop/pull/7989#discussion_r2371230540
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java:
##########
@@ -72,14 +72,24 @@ static RenameResult renameToInt(
* Verify quota for rename operation where srcInodes[srcInodes.length-1]
moves
* dstInodes[dstInodes.length-1]
*/
- private static Pair<Optional<QuotaCounts>, Optional<QuotaCounts>>
verifyQuotaForRename(
- FSDirectory fsd, INodesInPath src, INodesInPath dst) throws
QuotaExceededException {
+ private static Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>>
verifyQuotaForRename(
+ FSDirectory fsd, INodesInPath src, INodesInPath dst, boolean overwrite)
+ throws QuotaExceededException {
Review Comment:
The method signature change from Pair to Triple with a Boolean parameter
lacks clear documentation. The Boolean return value's meaning is not obvious
from the method name or parameters. Consider renaming the method or adding
clear JavaDoc to explain what the Boolean represents.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java:
##########
@@ -712,8 +723,9 @@ private static class RenameOperation {
withCount = null;
}
// Set quota for src and dst, ignore src is in Snapshot or is Reference
+ this.updateQuota = quotaPair.getLeft() || isSrcInSnapshot ||
srcChildIsReference;
this.srcSubTreeCount = withCount == null ?
- quotaPair.getLeft() : Optional.empty();
+ quotaPair.getMiddle() : Optional.empty();
this.dstSubTreeCount = quotaPair.getRight();
}
Review Comment:
The logic for determining updateQuota combines three different conditions
without clear explanation. This complex boolean expression makes the code
harder to understand and maintain. Consider extracting this logic into a
separate method with descriptive naming.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java:
##########
@@ -1379,16 +1398,22 @@ public INodesInPath addLastINode(INodesInPath existing,
INode inode,
// always verify inode name
verifyINodeName(inode.getLocalNameBytes());
- final QuotaCounts counts = quotaCount.orElseGet(() -> inode.
- computeQuotaUsage(getBlockStoragePolicySuite(),
- parent.getStoragePolicyID(), false,
- Snapshot.CURRENT_STATE_ID));
- updateCount(existing, pos, counts, checkQuota);
+ if (updateQuota) {
+ QuotaCounts counts = quotaCount.orElseGet(() -> inode.
+ computeQuotaUsage(getBlockStoragePolicySuite(),
+ parent.getStoragePolicyID(), false,
+ Snapshot.CURRENT_STATE_ID));
+ updateCount(existing, pos, counts, checkQuota);
+ }
boolean isRename = (inode.getParent() != null);
final boolean added = parent.addChild(inode, true,
existing.getLatestSnapshotId());
- if (!added) {
+ if (!added && updateQuota) {
+ QuotaCounts counts = quotaCount.orElseGet(() -> inode.
+ computeQuotaUsage(getBlockStoragePolicySuite(),
+ parent.getStoragePolicyID(), false,
+ Snapshot.CURRENT_STATE_ID));
Review Comment:
The quota computation is duplicated - it's computed before line 1402 and
again here. This results in unnecessary computation when updateQuota is true
and the node fails to be added. Consider computing the counts once and reusing
the result.
--
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]
