-
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-15879. Exclude slow nodes when choose targets for blocks #2748
Conversation
Failed junit tests: This failed unit tests was unrelated to the change, and it worked fine locally. |
Hi @Hexiaoqiao , could you please help review the code? Thank you. |
Hi @dineshchitlangia @tasanuma @ayushtkn @qizhu-lucas , could you please help review the code? Thank you. |
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.
@tomscut Thank you for contributing this improvement. Requested minor changes inline.
...hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
Outdated
Show resolved
Hide resolved
...hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SlowPeerTracker.java
Outdated
Show resolved
Hide resolved
Thanks @dineshchitlangia for your review and advice. And I will fix it soon. |
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 LGTM, thank you for making the changes.
I will leave it open for a few other folks to review this.
...ava/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicyExcludeSlowNodes.java
Outdated
Show resolved
Hide resolved
Thanks @dineshchitlangia for your careful review. |
💔 -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. |
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 for updating the PR, @tomscut.
I have one more request and commented on it.
...ava/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicyExcludeSlowNodes.java
Show resolved
Hide resolved
...ava/org/apache/hadoop/hdfs/server/blockmanagement/TestReplicationPolicyExcludeSlowNodes.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
@tomscut Looks good to me, except for the checkstyle issues. Could you fix them? (I'm sorry that I should have mentioned it before.) |
...hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Hi @tasanuma , those failed unit tests are unrelated to the change, and they work fine locally. Please take a look at the new commit, thank you. |
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.
Merged it. Thanks for your contribution, @tomscut! |
Thanks @tasanuma @dineshchitlangia ! |
…#2748) Reviewed-by: Dinesh Chitlangia <dineshc@apache.org> Reviewed-by: Takanobu Asanuma <tasanuma@apache.org>
JIRA: HDFS-15879
Previously, we have monitored the slow nodes, related to HDFS-11194
We can use a thread to periodically collect these slow nodes into a set. Then use the set to filter out slow nodes when choose targets for blocks.
This feature can be configured to be turned on when needed.