HDFS-16757. Add a new method copyBlockCrossNamespace to DataNode #6926
+578
−55
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? |
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. |
|
💔 -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.
There was a problem hiding this comment.
I have the same question. If the src block is not protected here, will the results be inconsistent?
...op-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
Show resolved
Hide resolved
| @@ -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 if without this , a task will block until success or failure. It is best left as a separate setting for fastcopy.
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
Show resolved
Hide resolved
Thanks for reveiw @Hexiaoqiao |
KeeProMise
reviewed
Sep 9, 2024
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
Outdated
Show resolved
Hide resolved
KeeProMise
reviewed
Sep 9, 2024
Comment on lines
+4441
to
+4446
| try { | ||
| result.get(getDnConf().getCopyBlockCrossNamespaceSocketTimeout(), TimeUnit.MILLISECONDS); | ||
| } catch (Exception e) { | ||
| LOG.error(e.getMessage()); | ||
| throw new IOException(e); | ||
| } |
There was a problem hiding this comment.
Should distinguish between InterruptedException and ExecutionException, TimeoutException?
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
Outdated
Show resolved
Hide resolved
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
Outdated
Show resolved
Hide resolved
| dstBlockFile = dstBPS.addFinalizedBlock(dstBlock.getLocalBlock(), replicaInfo); | ||
| replicaInfo = new FinalizedReplica(dstBlock.getLocalBlock(), getVolume(srcBlock), | ||
| dstBlockFile.getParentFile()); | ||
| volumeMap.add(dstBlock.getBlockPoolId(), replicaInfo); |
There was a problem hiding this comment.
Since volumeMap.add() needs to acquire the block pool's read lock, can we move the line volumeMap.add() outside the try block, and execute volumeMap.add() after releasing the lock? I'm not sure if there will be any concurrency issues with this approach.
There was a problem hiding this comment.
dst volume write lock protects hardlink file atomicity and volumeMap.add(). The best case scenario we should use src volume read lock and dst volume write lock to protect src and dst file operation.
And in DataXceiver, writeBlock() readBlock() transferBlock() , is as same as this only require lock with block meta operation.
So, I think this is enough for now. @Hexiaoqiao he @KeeProMise
tomscut
reviewed
Dec 16, 2024
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
Show resolved
Hide resolved
...-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
Outdated
Show resolved
Hide resolved
...hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/BlockPoolSlice.java
Show resolved
Hide resolved
|
💔 -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. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
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