HADOOP-18096. Distcp: Sync moves filtered file to home directory rather than deleting. #3940
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
…er than deleting.
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
looks like yarn won't build. not this patch's fault |
| new DistCp(conf, diffBuilder.build()).execute(); | ||
|
|
||
| // Check the only qualified directory dir2 is there in target | ||
| assertTrue(dfs.exists(new Path(target, "dir2"))); |
Contributor
There was a problem hiding this comment.
use the ContractTestUtils asserts here as they report on failures, including with dir listing info
Member
Author
There was a problem hiding this comment.
Thanx @steveloughran for the review, Have changed it to use ContractTestUtils
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Member
|
I'm not very familiar with the HDFS snapshot feature so can't help much with the reviewing here. cc @bshashikant @steveloughran : could you review this? |
saintstack
reviewed
Feb 9, 2022
Contributor
saintstack
left a comment
saintstack
left a comment
There was a problem hiding this comment.
(I don't know much about snapshots...)
I see this code in DistCpSync
private void moveToTarget(DiffInfo[] diffs,
DistributedFileSystem targetFs) throws IOException {
// sort the diffs based on their target paths to make sure the parent
// directories are created first.
Arrays.sort(diffs, DiffInfo.targetComparator);
for (DiffInfo diff : diffs) {
if (diff.getTarget() != null) {
targetFs.mkdirs(diff.getTarget().getParent());
targetFs.rename(diff.getTmp(), diff.getTarget());
}
}
}
When the DiffInfo 'target' was non-null, its parent dir was the home directory?
Thanks.
steveloughran
approved these changes
Feb 10, 2022
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
+1
going to assume the code does what you intend, as the tests imply all is good.
added a suggestion about test tuning, but its minor and you can skip if.
| dfs.createSnapshot(target, "s1"); | ||
|
|
||
| // Now do a rename to a filtered name on source. | ||
| dfs.rename(new Path(sourcePath, "dir1"), |
Contributor
There was a problem hiding this comment.
check result for non zero or that the dest file exists
Member
Author
There was a problem hiding this comment.
Done, Will push if tests are green
Member
Author
|
@saintstack For rename entries it is made absolute here: For normal delete there won't be any target, it would be always In this particular case when using filters. The actual entry is a And when converting it to a But since it is a delete entry the path isn't made absolute wrt target. So it stays like a relative path. like Then the code that you shared does the magic, it moves it... One example of target being set to May be there could be sanity check in delete diff for target, but not very confident about that part, will explore sometime if there is any use case possible where it can be not-null & compat stuff. Further general optimisations as well are possible, like don't rename to tmp and then delete, directly delete(There is a reason why it is like that), that is something in my TODO list, will chase in future General Info: Filters are like quite used in DR setups, some time we don't want to copy some data to replica clusters. One example could be Trash data, many other use cases as well. Lemme know if it isn't still convincing.. |
saintstack
approved these changes
Feb 10, 2022
Contributor
saintstack
left a comment
saintstack
left a comment
There was a problem hiding this comment.
Thank you for the very helpful write-up (You might want to copy/paste it up onto the JIRA).
There looks to be good test coverage around these parts so I'd think if this change damaging to general flow, it would have surfaced as test failures.
+1
5 tasks
Member
|
Thanks @steveloughran and @saintstack for the review! @ayushtkn could you merge this and backport to branch-3.3.2? I'll kick off 3.3.2 RC4 right after. |
|
🎊 +1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Feb 10, 2022
…er than deleting. (#3940). Contributed by Ayush Saxena. Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: stack <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Feb 10, 2022
…er than deleting. (#3940). Contributed by Ayush Saxena. Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: stack <[email protected]>
Member
Author
|
Thanx @saintstack @steveloughran and @sunchao for the help here. I have posted the comment on the jira as well |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…er than deleting. (apache#3940). Contributed by Ayush Saxena. Reviewed-by: Steve Loughran <[email protected]> Reviewed-by: stack <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Description of PR
Fixes creation of Delete Diff entry, the target in the delete diff should be null.
How was this patch tested?
Added a UT
For code changes: