HADOOP-17742. fix distcp fail when copying to ftp filesystem #3071
Conversation
This file contains hidden or 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. |
Member
Author
|
@steveloughran hi, would you help me review the commit |
steveloughran
requested changes
Jun 3, 2021
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
I've commented on the distcp side of the changes,.
Regarding FTP
- That Test suite MUST be restored. That's where the entire semantics of rename in the FS are validated.
- Looking at the history of the code you are cutting, it went in because FTP couldn't rename files into different directories (HADOOP-9361).
What has changed since then?
AFAIK,. the FTP API used by the client has to change to the directory and rename by short name only, not the full the path.
| Path root = target.equals(targetWorkPath) ? targetWorkPath.getParent() | ||
| : targetWorkPath; | ||
| Path tempFile = new Path(root, ".distcp.tmp." + | ||
| String tempFilePrefix = DistCpUtils.getTargetTempFilePrefix(fileSystem); |
Contributor
There was a problem hiding this comment.
assuming the Path is resolved, the FS Scheme is that of Path.toURI.getScheme(); no need to pass the FS around.
| public static final String CLASS_INSTANTIATION_ERROR_MSG = | ||
| "Unable to instantiate "; | ||
|
|
||
| public static final String TARGET_TEMP_FILE_PREFIX_COMMA = ".distcp.tmp."; |
Contributor
There was a problem hiding this comment.
- use DOT instead of COMMA here.
- add javadoc to explain what it does
|
|
||
| public static final String TARGET_TEMP_FILE_PREFIX_COMMA = ".distcp.tmp."; | ||
|
|
||
| public static final String TARGET_TEMP_FILE_PREFIX = "distcp.tmp."; |
Contributor
There was a problem hiding this comment.
add _FTP and javadocs to explain why it is needed
| throws IOException { | ||
| LOG.info("Copying {} to {}", source.getPath(), target); | ||
|
|
||
| final Configuration configuration = context.getConfiguration(); |
Contributor
There was a problem hiding this comment.
If you just get the schema in getTempFile() from the path, no need to pass in FileSystem, so no need to move this up.
| } | ||
|
|
||
| /** | ||
| * Return the target temp file prefix |
Contributor
There was a problem hiding this comment.
add . on javadoc sentence to keep javadoc happy
| String tempFilePrefix = DistCpUtils.getTargetTempFilePrefix(targetFS); | ||
| FileStatus[] tempFiles = targetFS.globStatus( | ||
| new Path(targetWorkPath, ".distcp.tmp." + jobId.replaceAll("job","attempt") + "*")); | ||
| new Path(targetWorkPath, tempFilePrefix + jobId.replaceAll("job","attempt") + "*")); |
Contributor
There was a problem hiding this comment.
probably need to cut the line down to keep it under 80 chars width
Member
Author
Thanks, I will update the distCp side of the changes. Regarding FTP, the test suite could be need to modify. After my test using commons-net:3.6, the rename can be work using the full path, just only have the parent directory, and don't have the limit of source path and target path have common parent directory. So, I think the rename method of FTPFilesystem should be modified so that it has the same behavior as other file systems. |
…pache#3042) Contributed by kaifeiYi (yikf). Signed-off-by: Mingliang Liu <liuml07@apache.org> Reviewed-by: Steve Loughran <stevel@apache.org>
Signed-off-by: Akira Ajisaka <aajisaka@apache.org>
…doop-mapreduce-project (apache#3074)
…op-common, hadoop-tools and cloud-storage projects (apache#3072)
Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
|
🎊 +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
6 participants
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.
more information:
https://issues.apache.org/jira/browse/HADOOP-17742#