HDFS-17509. RBF: Fix ClientProtocol.concat will throw NPE if tgr is a empty file. #6784
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
RBF: Fix ClientProtocol.concat will throw NPE if tgr is a empty file.
|
💔 -1 overall
This message was automatically generated. |
added 2 commits
May 6, 2024 10:45
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Hi,sir. Do you have time to review this PR, thanks! |
|
🎊 +1 overall
This message was automatically generated. |
ZanderXu
reviewed
May 9, 2024
| } | ||
| } | ||
| // Concat only effects when all files in same namespace. | ||
| // And in router view, a file only exists in one RemoteLocation. |
There was a problem hiding this comment.
Normally a file only exits in one Namespace. But maybe multiple namespaces contain this file and RBF returns the file in the first namespace to the client.
This first namespace is got by the OrderedResolver.
you can refer to this case:
- /path mounts to NS1, NS2 and NS3
- NS2 and NS3 contains /path
OrderedResolverreturns NS1, NS2 and NS3
for this case, we should proxy this concat to NS2 instead of NS1, right?
There was a problem hiding this comment.
For the empty file, maybe we can use getFileInfo to get the namespace that this trg belongs to.
There was a problem hiding this comment.
Thanks for review.
you can refer to this case:
- /path mounts to NS1, NS2 and NS3
- NS2 and NS3 contains /path
OrderedResolverreturns NS1, NS2 and NS3for this case, we should proxy this
concatto NS2 instead of NS1, right?
This only happens when /path is a directory. And for a file ,it must only be from one exactly nameservice via dfsrouter.
There was a problem hiding this comment.
DFSRouter with different order(HASH,LOCAL, RANDOM, HASH_ALL, SPACE) , a file cannot be written into two or more nameservices.
There was a problem hiding this comment.
Emmm, there is a scene .
-
If a file is already exist in two nameservices. And the add router mount.
NS1 /user/test/file
NS2 /user/test/file -
Add router mount.
hdfs dfsrouteradmin -add /user/test NS1,NS2 /user/test -order RANDOM -
getDestination
hdfs dfsrouteradmin -getDestination /user/test/file
Will return NS1,NS2
For a file in Router view with more than one nameservices, I think should thrown Exception for concat method.
Look forward to your guidance , thanks ! @ZanderXu
There was a problem hiding this comment.
For a file in Router view with more than one nameservices, I think should thrown Exception for concat method.
For multiple namespaces contain one same file case, RBF just return the file in the first namespace currently, such as: getBlockLocation, getFileInfo, etc.
So If you want to thrown Exception for concat, maybe you need to modify all RPCs to throw Exception for this case.
May we can use getFileInfo instead of getBlockLocation to fix this bug. BTW, getFileInfo is no needed if this path only mounts to one namespace.
There was a problem hiding this comment.
May we can use getFileInfo instead of getBlockLocation to fix this bug. BTW, getFileInfo is no needed if this path only mounts to one namespace.
At the beginning , I consider use getFileInfo . But the problem is that HdfsFileStatus does not have any information about nameservices or blockpoolid, only LocatedBlock has.
There was a problem hiding this comment.
we don't need to get NSId or BPId from the result of getFileInfo, you can refer to invokeSequential to loop all namespaces one by one.
- Get all namespaces of the input path.
- Send getFileInfo to each namespace one by one
- The first namespace that the result of getFileInfo is not null is the one we need
| @@ -1224,6 +1224,17 @@ public void testProxyConcatFile() throws Exception { | |||
| String badPath = "/unknownlocation/unknowndir"; | |||
| compareResponses(routerProtocol, nnProtocol, m, | |||
| new Object[] {badPath, new String[] {routerFile}}); | |||
|
|
|||
| // Test when concat trg is a empty file | |||
There was a problem hiding this comment.
we also need to check the empty source file.
There was a problem hiding this comment.
When there is a empty source file, it will throw exception in namenode. This behaiver is as same as with dfsrouter.
And when trg is a empty file, it is diffrent . Without dfsrouter ,it success. And with dfsrouter, it will throw Exception.
So I think there is no need to check the empty source file. Or implement it in another PR.
There was a problem hiding this comment.
Do the namenode and rbf throw the same Exception?
Maybe RBF throws NPE, but NN throws org.apache.hadoop.HadoopIllegalArgumentException.
There was a problem hiding this comment.
When srclist has a empty file ,both namenode and dfsrouter will throw the same IOException
https://github.com/apache/hadoop/blob/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirConcatOp.java#L153-L155
There was a problem hiding this comment.
Can you modify the UT to cover the case that one or more source files are empty?
There was a problem hiding this comment.
Add test for a empty src file
|
🎊 +1 overall
This message was automatically generated. |
ZanderXu
reviewed
May 11, 2024
| @@ -1224,6 +1224,17 @@ public void testProxyConcatFile() throws Exception { | |||
| String badPath = "/unknownlocation/unknowndir"; | |||
| compareResponses(routerProtocol, nnProtocol, m, | |||
| new Object[] {badPath, new String[] {routerFile}}); | |||
|
|
|||
| // Test when concat trg is a empty file | |||
There was a problem hiding this comment.
Do the namenode and rbf throw the same Exception?
Maybe RBF throws NPE, but NN throws org.apache.hadoop.HadoopIllegalArgumentException.
| RemoteMethod method = | ||
| new RemoteMethod("getFileInfo", new Class<?>[] {String.class}, new RemoteParam()); | ||
| // Check for file information sequentially | ||
| RemoteResult<RemoteLocation, HdfsFileStatus> result = |
There was a problem hiding this comment.
If locations only contains one namespace, we can returns this namespace directly instead of getting the namespace through getFileInfo, right?
There was a problem hiding this comment.
This may not be true. Even locations only contains one namespace, it still cannot decide whether the file exists or not. So getFileInfo is better to execute at least once. Or it will send to namenode and namenode throw file is not found.
There was a problem hiding this comment.
RBF is simply responsible for locating the downstream namespace and then proxying the request.
So if the input path is only mounted to one namespace, RBF only needs to proxy it directly. RBF does not need to check if the file exists in this only one downstream namespace, right?
|
🎊 +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. |
ZanderXu
reviewed
May 13, 2024
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterClientProtocol.java
Outdated
Show resolved
Hide resolved
ZanderXu
reviewed
May 13, 2024
| String targetFile = cluster.getFederatedTestDirectoryForNS(sameNameservice) + "_targetFile"; | ||
| createFile(routerFS, targetFile, existingFileSize); | ||
| // Concat in same namespaces, succeeds | ||
| testConcat(srcEmptyFile, targetFile, true, true, |
There was a problem hiding this comment.
here please check if the exception is org.apache.hadoop.ipc.RemoteException(org.apache.hadoop.HadoopIllegalArgumentException)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Merged. Thanks @LiuGuH for your contribution. |
K0K0V0K
pushed a commit
to K0K0V0K/hadoop
that referenced
this pull request
May 17, 2024
K0K0V0K
pushed a commit
to K0K0V0K/hadoop
that referenced
this pull request
May 17, 2024
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 HDFS-17509
hdfs dfs -concat /tmp/merge /tmp/t1 /tmp/t2
When /tmp/merge is a empty file, this command will throw NPE via DFSRouter.
Because a file via DFSRouter only can be exists in one namespace, so this RP can implement in this way.