HADOOP-18582. skip unnecessary cleanup logic in distcp #5251
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. |
cnauroth
requested changes
Dec 22, 2022
There was a problem hiding this comment.
@MaxWk , thank you for the patch. I entered one comment. Can you also please confirm if your Apache JIRA ID is 10000kang? If so, I can get HADOOP-18582 assigned to you.
| @@ -152,6 +152,15 @@ public void abortJob(JobContext jobContext, | |||
| } | |||
|
|
|||
| private void cleanupTempFiles(JobContext context) { | |||
| final boolean directWrite = context.getConfiguration().getBoolean( | |||
There was a problem hiding this comment.
There is an existing call to Configuration conf = context.getConfiguration(); below. Can you please lift that up and then reuse it for the conf.getBoolean(...) calls that you need to add?
|
yes,that's me |
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
approved these changes
Dec 23, 2022
There was a problem hiding this comment.
Some nits and you need to fix the checkstyle warnings reported
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5251/2/artifact/out/results-checkstyle-hadoop-tools_hadoop-distcp.txt
Rest changes LGTM
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyCommitter.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyCommitter.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyCommitter.java
Outdated
Show resolved
Hide resolved
MaxWk
force-pushed
the
trunk
branch
2 times, most recently
from
021e8f2
to
1d7dcd5
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Dec 24, 2022
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyCommitter.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyCommitter.java
Outdated
Show resolved
Hide resolved
MaxWk
requested review from
steveloughran,
ayushtkn and
cnauroth
and removed request for
cnauroth,
steveloughran and
ayushtkn
|
🎊 +1 overall
This message was automatically generated. |
MaxWk
requested review from
steveloughran and
cnauroth
and removed request for
cnauroth and
steveloughran
cnauroth
reviewed
Jan 17, 2023
cnauroth
pushed a commit
that referenced
this pull request
Jan 24, 2023
Co-authored-by: 万康 <mingge@xiaohongshu.com> Reviewed-by: Steve Loughran <stevel@apache.org> Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: Chris Nauroth <cnauroth@apache.org> (cherry picked from commit 3b7b79b)
cnauroth
reviewed
Jan 24, 2023
There was a problem hiding this comment.
I have committed this to trunk and branch-3.3. (This code doesn't compile when applied to branch-3.2, so I couldn't commit it there.)
@MaxWk , thank you for the contribution. @steveloughran and @ayushtkn , thank you for code reviewing.
|
nice. |
| final boolean append = conf.getBoolean( | ||
| DistCpOptionSwitch.APPEND.getConfigLabel(), false); | ||
| final boolean useTempTarget = !append && !directWrite; | ||
| if (!useTempTarget) { |
There was a problem hiding this comment.
Reviewing this, I think the cleanup should only be skipped on direct not append.
why so, append will choose between overwrite and append depending on the state of the destination file -a decision made case by case. when doing distcp with -append, new files or those whose dest checksum doesn't match that of the source for the same #of bytes will use a temp file unless -direct is set.
There was a problem hiding this comment.
Isn't that same check as here in RetriableFileCopyCommand
final boolean toAppend = action == FileAction.APPEND;
final boolean useTempTarget = !toAppend && !directWrite;
Path targetPath = useTempTarget ? getTempFile(target, context) : target;
There was a problem hiding this comment.
it's doing it file by file: some files will be appended to, but new ones, ones with checksum mismatch are overwritten by first writing to temp, then rename, as usual
any -append job which added files can create temp paths
There was a problem hiding this comment.
Thanx for the explanation. Will give some time to the original author, In case no one volunteers for few days, I will fix this.
|
Going to have to do a followup here; we still need to do cleanup on -append. anyone want to take that up? |
|
Have removed it for append in #5409 |
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
reduce execution time when distcp in direct or append mode.https://issues.apache.org/jira/browse/HADOOP-18582
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?