HDFS-16757. Add a new method copyBlockCrossNamespace to DataNode #6926
+566
−63
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -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. |
Hexiaoqiao
reviewed
Aug 25, 2024
There was a problem hiding this comment.
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.
Be careful to throw RuntimeException directly here.
There was a problem hiding this comment.
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); |
| BlockLocalPathInfo blpi = getBlockLocalPathInfo(srcBlock); | ||
| FsVolumeImpl v = getVolume(srcBlock); | ||
|
|
||
| try (AutoCloseableLock lock = lockManager.writeLock(LockLevel.VOLUME, dstBlock.getBlockPoolId(), |
There was a problem hiding this comment.
Not sure if it is enough to obtain only dest block pool lock here.
There was a problem hiding this comment.
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.
Sorry I don't get the purpose to add userAssignmentLastBlock here.
There was a problem hiding this comment.
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.
Is this one config item same as CopyOp time out and is it necessary?
There was a problem hiding this comment.
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.
- What this attribution describe here?
There was a problem hiding this comment.
This is used for distinguishing that a DataTransfer task is form NN command or from fastcopy task.
Thanks for reveiw @Hexiaoqiao |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
As described below HDFS-16757
This is the base PR of FastCopy HDFS-2139