-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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.
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
HDFS-16428. Source path with storagePolicy cause wrong typeConsumed while rename #3898
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
dfs.rename(bs2, bs1); | ||
assertTrue("rename with storage policy, typeConsumed should not be 0.", | ||
dfs.getQuotaUsage(bs1).getTypeConsumed(StorageType.SSD) != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for the storage policy of bs1 after the rename.
dfs.getStoragePolicy(bs1)
It will be HOT not ONE_SSD. Once you have renamed the file another location, which uses another policy. It will show that only.
After sometime the Namenode would move the block from SSD to DISK. The behaviour looks like the default behaviour. (By Design)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayushtkn thank you very much for your comment. Yes, directory bs1 is hot but its subdirectory bs2 is setted one_ssd. In this situation, dir bs2 will use itself block storage policy rather than his parent storage policy, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, Thanx @ThinkerLei for confirming.
Rechecking, seems correct. The issue and fix makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some checkstyle warnings reported by Jenkins:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3898/2/artifact/out/results-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
Other than that Changes LGTM
🎊 +1 overall
This message was automatically generated. |
@@ -339,6 +339,13 @@ public boolean isFile() { | |||
return false; | |||
} | |||
|
|||
public boolean isSetStoragePolicy() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor comment. It will be good if we add a comment to this method. People may confuse about "inheriting storage policy from its parent" and "containing storage policy itself". Add a comment like getStoragePolicyID() and getLocalStoragePolicyID().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojiaodoubao thank you very much for your comment. I had fixed it and repush it.
final boolean isSrcSetSp = src.getLastINode().isSetStoragePolicy(); | ||
final byte storagePolicyID = isSrcSetSp ? | ||
src.getLastINode().getLocalStoragePolicyID() : | ||
dstParent.getStoragePolicyID(); | ||
final QuotaCounts delta = src.getLastINode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a test case to verify the quota check part.
…sumed in rename operation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. +1 from my side.
The checkstyle error seems not related to this changes. @ThinkerLei would you mind to give another double check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
🎊 +1 overall
This message was automatically generated. |
…hile rename (#3898). Contributed by lei w. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
…hile rename (apache#3898). Contributed by lei w. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
…hile rename (#3898). Contributed by lei w. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
…hile rename (#3898). Contributed by lei w. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
…hile rename (apache#3898). Contributed by lei w. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
No description provided.