HDFS-17839. Rename skips path without valid 'DirectoryWithQuotaFeature' in FSDirRenameOp #7989
+276
−37
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e' in FSDirRenameOp
|
💔 -1 overall
This message was automatically generated. |
|
@Hexiaoqiao @slfan1989 Hello sir, Can you help me review the PR when you have time? Thanks~ |
|
💔 -1 overall
This message was automatically generated. |
huangzhaobo99
force-pushed
the
HDFS-17839
branch
from
7a2162c to
3c60bf0
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes quota calculation in HDFS rename operations by skipping quota updates when paths don't contain valid DirectoryWithQuotaFeature entries. The optimization specifically targets rename1 scenarios where neither source nor destination paths (excluding root) have quota features, while preserving quota calculation for overwrite scenarios (rename2) that affect root directory quotas.
Key Changes:
- Added path validation to skip quota calculation when no valid DirectoryWithQuotaFeature exists
- Modified rename operations to conditionally update quotas based on path features and operation type
- Updated method signatures to support the new updateQuota parameter throughout the codebase
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| FSDirRenameOp.java | Core logic for path validation and conditional quota updates in rename operations |
| FSDirectory.java | Added path verification method and updated addLastINode to support conditional quota updates |
| FSDirMkdirOp.java | Updated method call to maintain existing quota behavior for mkdir operations |
| TestCorrectnessOfQuotaAfterRenameOp.java | Added comprehensive test for rename operations without valid quota features |
| TestRenameWithSnapshots.java | Updated mock method call to match new signature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+75
to
+77
| private static Triple<Boolean, Optional<QuotaCounts>, Optional<QuotaCounts>> verifyQuotaForRename( | ||
| FSDirectory fsd, INodesInPath src, INodesInPath dst, boolean overwrite) | ||
| throws QuotaExceededException { |
There was a problem hiding this 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.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
/**
* Verify quota for rename operation where srcInodes[srcInodes.length-1] moves
* dstInodes[dstInodes.length-1]
*
* @param fsd FSDirectory instance
* @param src Source path inodes
* @param dst Destination path inodes
* @param overwrite Whether destination will be overwritten
* @return Triple containing:
* - Boolean: true if quota was updated, false if skipped
* - Optional<QuotaCounts>: Source path quotaCounts delta
* - Optional<QuotaCounts>: Destination path quotaCounts delta
* @throws QuotaExceededException if quota limit is exceeded
*/
Comment on lines
726
to
731
| this.updateQuota = quotaPair.getLeft() || isSrcInSnapshot || srcChildIsReference; | ||
| this.srcSubTreeCount = withCount == null ? | ||
| quotaPair.getLeft() : Optional.empty(); | ||
| quotaPair.getMiddle() : Optional.empty(); | ||
| this.dstSubTreeCount = quotaPair.getRight(); | ||
| } | ||
|
|
There was a problem hiding this 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.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Ignore src is in Snapshot or is Reference
Comment on lines
1412
to
1416
| if (!added && updateQuota) { | ||
| QuotaCounts counts = quotaCount.orElseGet(() -> inode. | ||
| computeQuotaUsage(getBlockStoragePolicySuite(), | ||
| parent.getStoragePolicyID(), false, | ||
| Snapshot.CURRENT_STATE_ID)); |
There was a problem hiding this 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.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
When the add failed, it is to restore the quota usage.
If there was no update before, there is no need to update when it failed.
updateCountNoQuotaCheck(existing, pos, counts.negation());
|
🎊 +1 overall
This message was automatically generated. |
|
@huangzhaobo99 Thanks for your contribution, I will take a look at this PR tomorrow. @zhtttylz Could you please take a look at this PR? Thank you very much! |
|
Thanks! |
zhtttylz
reviewed
Sep 28, 2025
| final boolean added = parent.addChild(inode, true, | ||
| existing.getLatestSnapshotId()); | ||
| if (!added) { | ||
| if (!added && updateQuota) { |
There was a problem hiding this comment.
Sorry for the late reply. Do you think we should keep the original logic of always returning null when added == false? Regardless of whether the quota is updated, the decision to roll back the quota update should probably depend only on updateQuota. I might be missing some context here—happy to hear your thoughts.
There was a problem hiding this comment.
@zhtttylz Sorry, the original rollback quotaUsage logic has been changed, fixed.
There was a problem hiding this comment.
Thank you for the review. I will add an rename undo integration test here.
There was a problem hiding this comment.
@zhtttylz Hi sir, I have added the RenameUndo UT. The original logic had some issues, but it now functions as intended after being rectified.
huangzhaobo99
force-pushed
the
HDFS-17839
branch
from
5756bc3 to
f96079b
Compare
|
🎊 +1 overall
This message was automatically generated. |
huangzhaobo99
force-pushed
the
HDFS-17839
branch
from
f96079b to
62f0927
Compare
huangzhaobo99
force-pushed
the
HDFS-17839
branch
from
62f0927 to
1e9321b
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@zhtttylz Hello sir, if you have time, please review it again. Thank you very much. I will fix checkstyle issues later. |
|
🎊 +1 overall
This message was automatically generated. |
huangzhaobo99
force-pushed
the
HDFS-17839
branch
from
546d988 to
35d1318
Compare
|
💔 -1 overall
This message was automatically generated. |
huangzhaobo99
force-pushed
the
HDFS-17839
branch
from
35d1318 to
7d03024
Compare
|
💔 -1 overall
This message was automatically generated. |
huangzhaobo99
force-pushed
the
HDFS-17839
branch
from
7d03024 to
4f46b2a
Compare
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA: https://issues.apache.org/jira/browse/HDFS-17839
Calculating quotaUsage 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:
rename1does not change the quotaUsage of the root directory, so neither src nor dst contains "DirectoryWithQuotaFeature" label (excluding the root directory). The operation of calculating quotaUsage should be skipped.rename2still needs to consider calculating quota, otherwise it will affect the quotaUsage value of the root directory.By default, the root directory has the "DirectoryWithQuotaFeature" label.
How was this patch tested?
Add UT