-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-16964. Improve processing of excess redundancy after failover. #5510
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.
Thanks to report and patch. IIUC, this improvement try to prepose to clean excess replicas exclude ones reside at stale storage, rather than wait to all storage leave stale status, right? If so, I think it makes sense to me. Leave some review comments inline, FYI.
@@ -4020,12 +4014,22 @@ public void setReplication( | |||
} | |||
} | |||
|
|||
private void processExtraRedundancyBlock(final BlockInfo block, |
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.
Suggest to add some java doc for this new method.
/** | ||
* Find how many of the containing nodes are "extra", if any. | ||
* If there are any extras, call chooseExcessRedundancies() to | ||
* mark them in the excessRedundancyMap. | ||
* @return if all redundancy replicas are removed |
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.
@return if all redundancy replicas are removed
->
@return true if all redundancy replicas are removed.
@@ -4035,17 +4039,17 @@ private void processExtraRedundancyBlock(final BlockInfo block, | |||
Collection<DatanodeStorageInfo> nonExcess = new ArrayList<>(); | |||
Collection<DatanodeDescriptor> corruptNodes = corruptReplicas | |||
.getNodes(block); | |||
boolean hasStaleStorage = false; | |||
DatanodeStorageInfo staleStorage = null; |
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.
this should be one set about DatanodeStorageInfo
because it could be more than one stale storage here.
💔 -1 overall
This message was automatically generated. |
@Hexiaoqiao Thanks for your review, I've updated the PR based on your suggestion. |
💔 -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. Let's wait if other folks are interested about this improvement.
BTW, the failed unit test seems un-relate to this PR. @zhangshuyan0 Please take another check.
@Hexiaoqiao Thanks. I've checked it out. |
Committed to trunk. @zhangshuyan0 thanks for your contribution! |
…pache#5510). Contributed by Shuyan Zhang. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
Description of PR
After failover, the block with excess redundancy cannot be processed until all replicas are not stale, because the stale ones may have been deleted. That is to say, we need to wait for the FBRs of all datanodes on which the block resides before deleting the redundant replicas. This is unnecessary, we can bypass stale replicas when dealing with excess replicas, and delete non-stale excess replicas in a more timely manner.