-
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-17299. Adding rack failure tolerance when creating a new file #6513
Conversation
💔 -1 overall
This message was automatically generated. |
… Fixing test failures
… Fixing test failures
… Fixing test failures
💔 -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.
Overall the changes looks good.
@ritegarg Can you please add a comment on why you changed the signature of setupPipelineForAppendOrRecovery
method.
" already exists in state " + replicaInfo.getState() + | ||
" and thus cannot be created."); | ||
if (newGS != 0L) { | ||
cleanupReplica(b.getBlockPoolId(), replicaInfo); |
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.
Add a comment on why are we cleaning up replica 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.
Added
@ritegarg There are still 45 test faiures in the latest run. Please take a look. |
@ayushtkn @Hexiaoqiao Can you guys please provide some early feedback? Thank you. |
Updated |
Fixed locally, started a new Jenkins build |
🎊 +1 overall
This message was automatically generated. |
*/ | ||
private void setupPipelineForAppendOrRecovery() throws IOException { | ||
private boolean setupPipelineForAppendOrRecovery() throws IOException { |
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.
|
||
// During create stage, if we remove a node (nodes.length - 1) | ||
// min replication should still be satisfied. | ||
if (isCreateStage && !(dfsClient.dtpReplaceDatanodeOnFailureReplication > 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.
Reason behind adding this check here:
We are already doing this check in catch block of addDatanode2ExistingPipeline
method here.
But when isAppend
flag is set to false
and we are in PIPELINE_SETUP_CREATE
phase, we exit early from addDatanode2ExistingPipeline
method here
Irrespective of ReplaceDatanodeOnFailure policy, we will NEVER add a new datanode to the pipeline during PIPELINE_SETUP_CREATE stage and if removing one bad datanode is going to violate dfs.client.block.write.replace-datanode-on-failure.min-replication
property then we should exit early.
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.
Thinking about the case where we are in PIPELINE_SETUP_CREATE stage but isAppend is set to true, then it will not exit early from addDatanode2ExistingPipeline method here
Assuming the replication factor is 3
and dfs.client.block.write.replace-datanode-on-failure.min-replication is set to 3
and there is 1 bad node in the pipeline and there are valid nodes in the cluster, this patch will return false early.
I think we should move this check after handleDatanodeReplacement
method. @ritegarg
💔 -1 overall
This message was automatically generated. |
Closing this PR in favor of https://github.com/apache/hadoop/pull/6556/files |
Description of PR
Adding rack failure tolerance when creating a new file. During create file operation(see logs in jira), in case of bad rack, the create operation failed and hence the operation was not rack failure tolerant. This was not same as what happens during append phase where min replication factor is applicable.
Adding this update to fix this behavior to make the create phase on the lines of append phase.
How was this patch tested?
Added Test Cases for following scenarios:-
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?