-
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-16277 improve decision in AvailableSpaceBlockPlacementPolicy #3559
Conversation
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.
Jira title is wrong: HADOOP-16277 is something different:
https://issues.apache.org/jira/browse/HADOOP-16277
Please correct the title.
Regarding the code changes:
*Should make this configurable, With default value as 5,
*Need to do the same change in AvailableSpaceRackFaultTolerantBlockPlacementPolicy
*Add a test
💔 -1 overall
This message was automatically generated. |
@ayushtkn Thanks for you kindly review, will improve it later |
Just an idea, i think also
|
🎊 +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.
Dropped some comments,
Regarding the test, You need to add a separate test which can validate your changes. As of now I don't see any
...n/java/org/apache/hadoop/hdfs/server/blockmanagement/AvailableSpaceBlockPlacementPolicy.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HdfsBlockPlacementPolicies.md
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hdfs/server/blockmanagement/AvailableSpaceBlockPlacementPolicy.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@ayushtkn Thanks for your detail comments, they have been fixed and test cases passed as well. any new ideas ? |
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.
Looks almost good to me. Dropped some minor comments. Should be good to go once addressed.
...va/org/apache/hadoop/hdfs/server/blockmanagement/TestAvailableSpaceRackFaultTolerantBPP.java
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/hdfs/server/blockmanagement/TestAvailableSpaceBlockPlacementPolicy.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hdfs/server/blockmanagement/AvailableSpaceBlockPlacementPolicy.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hdfs/server/blockmanagement/AvailableSpaceBlockPlacementPolicy.java
Outdated
Show resolved
Hide resolved
.../hadoop/hdfs/server/blockmanagement/AvailableSpaceRackFaultTolerantBlockPlacementPolicy.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
Outdated
Show resolved
Hide resolved
...va/org/apache/hadoop/hdfs/server/blockmanagement/TestAvailableSpaceRackFaultTolerantBPP.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
@ayushtkn Thanks your timely review, comments have been improved and tests are ok. |
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.
Jenkins have reported some checkstyle warnings
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3559/8/artifact/out/results-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
They seems to be for line length, Can you check once, I guess the ones reported from test can be fixed.
Couple of them are because the variable name is big, I think we can ignore them, as there is no good way to get rid of them.
Apart from that. Changes LGTM
Thanks @ayushtkn ,checkstyle has been fixed.except for the long variable name ones. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
|
||
if (balancedSpaceTolerance > 20 || balancedSpaceTolerance < 0) { | ||
LOG.warn("The value of " | ||
+ DFS_NAMENODE_AVAILABLE_SPACE_BLOCK_PLACEMENT_POLICY_BALANCED_SPACE_TOLERANCE_KEY |
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.
It would be great, if you can print the use configured value here.
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.
thanks @prasad-acit ,seems more clear for the warning message
💔 -1 overall
This message was automatically generated. |
It seems github issue ? |
💔 -1 overall
This message was automatically generated. |
@@ -54,6 +57,10 @@ public void initialize(Configuration conf, FSClusterStats stats, | |||
DFS_NAMENODE_AVAILABLE_SPACE_RACK_FAULT_TOLERANT_BLOCK_PLACEMENT_POLICY_BALANCED_SPACE_PREFERENCE_FRACTION_KEY, | |||
DFS_NAMENODE_AVAILABLE_SPACE_BLOCK_RACK_FAULT_TOLERANT_PLACEMENT_POLICY_BALANCED_SPACE_PREFERENCE_FRACTION_DEFAULT); | |||
|
|||
balancedSpaceTolerance = conf.getInt( | |||
DFS_NAMENODE_AVAILABLE_SPACE_RACK_FAULT_TOLERANT_BLOCK_PLACEMENT_POLICY_BALANCED_SPACE_TOLERANCE_KEY, | |||
DFS_NAMENODE_AVAILABLE_SPACE_BLOCK_RACK_FAULT_TOLERANT_PLACEMENT_POLICY_BALANCED_SPACE_TOLERANCE_DEFAULT); |
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.
Constants exceed the length lead to length issue, can be shrinked. BLOCK_PLACEMENT_POLICY => BPP can be used. Other places already large, atleast new code we can target.
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.
Thanks @prasad-acit for your advice ,it should be used for total new code , for now, if we cut ir for short, the variable name meaning may not that clear,maybe we can igonre it, and apply the short name as far as possible in furture.
@GuoPhilipse I have retriggred the build here: |
Realy appreciate it! @ayushtkn :) |
🎊 +1 overall
This message was automatically generated. |
Cool,it shows +1 overall, but still have failed check in report ,not sure if it is normal ? @ayushtkn |
Thanx @GuoPhilipse for the contribution, Have merged the PR. |
Thanks again @ayushtkn @prasad-acit for your kindly review. :) |
…3559). Contributed by guo. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
…pache#3559). Contributed by guo. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
Hi
In product environment,we may meet two or more datanode usage reaches nealy 100%,for exampe 99.99%,98%,97%.
if we configure
AvailableSpaceBlockPlacementPolicy
, we also have the chance to choose the 99.99%(assume it is the highest usage),for we treat the two choosen datanode as the same usage if their storage usage are different within 5%.but this is not what we want, so i suggest we can improve the decision.