HADOOP-18444 Add Support for localized trash for ViewFileSystem in Trash.moveToAppropriateTrash #4869
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
…ash.moveToAppropriateTrash Signed-off-by: Xing Lin <xinglin@linkedin.com>
|
🎊 +1 overall
This message was automatically generated. |
|
interesting. In #4729 i'm proposing the ability to choose a trash policy for different fs fschemas, so the one for viewfs could be different to hdfs and then from abfs and s3a. The changes are handling the actual rename/delete checkpoint stuff, but not the changes you are proposing into Trash. having Trash choose its policy based on instanceof values is a bad path to follow. I'd be happier if the decision was done in a viewfs specific trash policy. Would there be a way to add the extra methods needed for the policy itself -the existing plugin point- to handle the viewfs details? That could maybe also line up with having the stores use the same extension points if they want to be extra clever. Not sure what they would want to do; the work I'm adding is to address these problems
I'm away for a week, back sept 19. if you looked at my PR and especially the schema-specific plugin, would that work if you added the trash root awareness to the policy? if so, you could take that bit of the PR and run with it, as you are ahead of me and my work. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java
Outdated
Show resolved
Hide resolved
...-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java
Outdated
Show resolved
Hide resolved
...-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java
Outdated
Show resolved
Hide resolved
...-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewFsTrash.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
xinglin
force-pushed
the
HADOOP-18444
branch
from
e58b17b
to
3d92861
Compare
|
Hi @steveloughran, Thanks for your comments and sharing your WIP. I updated the description of this PR. Please take a look and hopefully it provides more context. We have actually extended using an existing extention point in HADOOP-18144 , which is the getTrashRoot() implemementation in ViewFileSystem. It can now returns a viewfs trash root (a viewfs path) with the new localized trash root flag. That provides a fs specific trash root. I don't think a new ViewFSTrashPolicy is strictly required. ViewFileSystem is also different from other fs, such as hdfs/abfs in that ViewFileSystem is an indirection layer which points to other fs. It is possible that given the current moveToAppropriateTrash() implementation which resolves the targetFs and use targetFs moveToTrash policy, the new viewFsTrashPolicy you have suggested may also be bypassed.
|
|
🎊 +1 overall
This message was automatically generated. |
omalley
reviewed
Sep 14, 2022
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Trash.java
Outdated
Show resolved
Hide resolved
|
changes after my comments all good. leaving the rest of the reviewing to @omalley |
mccormickt12
approved these changes
Sep 22, 2022
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
omalley
pushed a commit
that referenced
this pull request
Sep 23, 2022
…ash.moveToAppropriateTrash (#4869) * HADOOP-18444 Add Support for localized trash for ViewFileSystem in Trash.moveToAppropriateTrash Signed-off-by: Xing Lin <xinglin@linkedin.com>
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…ash.moveToAppropriateTrash (apache#4869) * HADOOP-18444 Add Support for localized trash for ViewFileSystem in Trash.moveToAppropriateTrash Signed-off-by: Xing Lin <xinglin@linkedin.com>
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
Trash.moveToAppropriateTrash was added to hadoop in HADOOP-7284, because the authors found out Trash/hadoop cli -rm did not work for viewFS. The reason I believe is when we initialized a Trash object with a ViewFileSystem object and a ViewFileSystem path, the original getTrashRoot() in ViewFileSystem returns a targeFS path. This leads to the wrong FS exception when we use a ViewFileSystem object to operate on a targetFS path.
java.lang.IllegalArgumentException: Wrong FS: file:/Users/xinglin/projs/hadoop-branchs/trunk/hadoop-common-project/hadoop-common/target/test/data/testTrash/.Trash/Current/data, expected: viewfs:/In HADOOP-18144, we have fixed getTrashRoot in ViewFileSystem, to return a ViewFileSystem path. Thus, moveToTrash() works now when we initialize the Trash object with a ViewFileSystem object and a ViewFileSystem path.
The existing moveToAppropriateTrash implementation is also causing a problem for the fix we put in HADOOP-18144. The reason is it first resolves a path and then uses the resolvedFs, to initialize the trash object. As a result, it uses getTrashRoot() implementation from targetFs. The fix we put in HADOOP-18144 in ViewFileSystem is bypassed and not used.
With this patch, we check whether the localized trash policy flag is set. If this is true, we initialize the Trash object with the ViewFileSystem FS object and call moveToTrash() with the logical path directly.
How was this patch tested?
Passed a new test added to TestViewFsTrash
mvn test -Dtest="TestViewFsTrash"