-
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-16757. Add a new method copyBlockCrossNamespace to DataNode #6926
base: HDFS-2139
Are you sure you want to change the base?
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Why not merge to trunk? |
@ZanderXu ,hello, sir. Please review this when you have time. |
At last comment in https://issues.apache.org/jira/browse/HDFS-2139 . I guess this is used for isolate. When all work is done will merge into trunk. Thanks |
💔 -1 overall
This message was automatically generated. |
@Hexiaoqiao @ZanderXu , please review this if you have time . Thanks. And the test case error is because branch apache:HDFS-2139 should merge HDFS-17566(https://issues.apache.org/jira/browse/HDFS-17566) |
💔 -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 @LiuGuH for your works. Leave some comments inline. PFYI. And this is one huge PR, we need involve more folks to revews.
Another one, now we don't consider capacity used between different BlockPool when hardlink at DataNode side, right? Will it resolve at next PR?
LOG.warn("{}:Failed to transfer {} to {} got", | ||
bpReg, b, targets[0], ie); | ||
if (copyBlockCrossNamespace) { | ||
throw new RuntimeException(ie); |
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.
Be careful to throw RuntimeException directly 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.
DataTransfer is implemented Runnable. And only support throw RuntimeException . This RuntimeException will catch by future.get() try catch .
LOG.error("Failed to transfer block {}", b, t); | ||
LOG.error("Failed to transfer block {}", source, t); | ||
if (copyBlockCrossNamespace) { | ||
throw new RuntimeException(t); |
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.
As the above comment.
BlockLocalPathInfo blpi = getBlockLocalPathInfo(srcBlock); | ||
FsVolumeImpl v = getVolume(srcBlock); | ||
|
||
try (AutoCloseableLock lock = lockManager.writeLock(LockLevel.VOLUME, dstBlock.getBlockPoolId(), |
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.
Not sure if it is enough to obtain only dest block pool lock 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.
Strictly speaking , here we should use src read lock and dst write lock. And must make sure the order of accquire locks of pools to avoid deadlock.
@@ -131,6 +131,7 @@ public class DFSOutputStream extends FSOutputSummer | |||
private FileEncryptionInfo fileEncryptionInfo; | |||
private int writePacketSize; | |||
private boolean leaseRecovered = false; | |||
private ExtendedBlock userAssignmentLastBlock; |
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.
Sorry I don't get the purpose to add userAssignmentLastBlock
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.
This is used for DFSOutputStream.completeFile() . When fastcop a file from src to dst , and dst is control by outputStream for file lease ,and use addblock() for create block with dst . When completeFile() is invoked, the last block should exits. So userAssignmentLastBlock is called when outputstrem closed.
@@ -136,6 +136,11 @@ public class DFSConfigKeys extends CommonConfigurationKeys { | |||
"dfs.datanode.ec.reconstruct.write.bandwidthPerSec"; | |||
public static final long DFS_DATANODE_EC_RECONSTRUCT_WRITE_BANDWIDTHPERSEC_DEFAULT = | |||
0; // A value of zero indicates no limit | |||
public static final String DFS_DATANODE_COPY_BLOCK_CROSS_NAMESPACE_SOCKET_TIMEOUT_MS_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.
Is this one config item same as CopyOp time out and is it necessary?
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 is used for copyBlockCrossNamespaceExecutor task timeout. And it not , a task will block until it sucess or failed. This can be removed or retain.
private DataTransferThrottler throttler; | ||
private boolean copyBlockCrossNamespace; |
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.
- What this attribution describe 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.
This is used for distinguishing that a DataTransfer task is form NN command or from fastcopy task.
Thanks for reveiw @Hexiaoqiao |
Description of PR
As described below HDFS-16757
This is the base PR of FastCopy HDFS-2139