[
https://issues.apache.org/jira/browse/HDFS-17839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18022047#comment-18022047
]
ASF GitHub Bot commented on HDFS-17839:
---------------------------------------
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.
> Rename skips path without valid 'DirectoryWithQuotaFeature' in FSDirRenameOp
> ----------------------------------------------------------------------------
>
> Key: HDFS-17839
> URL: https://issues.apache.org/jira/browse/HDFS-17839
> Project: Hadoop HDFS
> Issue Type: Improvement
> Reporter: Zhaobo Huang
> Assignee: Zhaobo Huang
> Priority: Major
> Labels: pull-request-available
>
> Calculating quota usage is primarily used to update directories labeled with
> the DirectoryWithQuotaFeature. When there is no valid
> DirectoryWithQuotaFeature in the paths of src and dst (excluding the root
> directory), the quota should not be calculated in rename scenarios:
> 1. rename1 does not change the quota of the root directory, so neither src
> nor dst contains DirectoryWithQuotaFeature (excluding the root directory).
> The operation of calculating quota should be skipped.
> 2. In the scenario of overwriting, rename2 still needs to consider
> calculating quota, otherwise it will affect the quotaUsage value of the root
> directory.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
