HADOOP-18893. Make Trash Policy pluggable for different FileSystems #6061
+360
−60
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
|
Putting up an initial PR for Trash policy separation. Need to verify this in a cluster still, but have added a small test for S3A and ABFS. HDFS has some good tests for Trash too. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Sep 26, 2023
| * @param fs the file system to be used | ||
| * @return an instance of TrashPolicy | ||
| */ | ||
| @SuppressWarnings("ClassReferencesSubclass") | ||
| public static TrashPolicy getInstance(Configuration conf, FileSystem fs) { |
There was a problem hiding this comment.
does the conf come from the filesystem? as if so it'd let us do per-bucket stuff. if not, well, it's possibly too late
| @@ -193,7 +193,7 @@ public boolean moveToTrash(Path path) throws IOException { | |||
| // move to current trash | |||
| fs.rename(path, trashPath, | |||
There was a problem hiding this comment.
should check return value here. "false" isn't that useful, but it does mean the rename failed
| @@ -347,31 +354,42 @@ private void createCheckpoint(Path trashRoot, Date date) throws IOException { | |||
| while (true) { | |||
| try { | |||
| fs.rename(current, checkpoint, Rename.NONE); | |||
There was a problem hiding this comment.
again, handle failure here
| @@ -347,31 +354,42 @@ private void createCheckpoint(Path trashRoot, Date date) throws IOException { | |||
| while (true) { | |||
| try { | |||
| fs.rename(current, checkpoint, Rename.NONE); | |||
| LOG.info("Created trash checkpoint: " + checkpoint.toUri().getPath()); | |||
| LOG.info("Created trash checkpoint: {}", checkpoint.toUri().getPath()); | |||
| break; | |||
| } catch (FileAlreadyExistsException e) { | |||
| if (++attempt > 1000) { | |||
There was a problem hiding this comment.
lets make this a constant
| } catch (FileNotFoundException fnfe) { | ||
| return; | ||
| return 0; |
There was a problem hiding this comment.
note that the listStatus calls may not raise the FNFE until the next/hasNext call, so catch it in the iterator below too
| Configuration conf = new Configuration(); | ||
| Trash trash = new Trash(getFileSystem(), conf); | ||
| assertEquals("Mismatch in Trash Policy set by the config", | ||
| trash.getTrashPolicy().getClass(), EmptyTrashPolicy.class); |
| @Test | ||
| public void testTrashSetToEmptyTrashPolicy() throws IOException { | ||
| Configuration conf = new Configuration(); | ||
| Trash trash = new Trash(getFileSystem(), conf); |
There was a problem hiding this comment.
use conf from filesystem
| @@ -3450,23 +3454,29 @@ public Path getTrashRoot(Path path) { | |||
| public Collection<FileStatus> getTrashRoots(boolean allUsers) { | |||
| Path userHome = new Path(getHomeDirectory().toUri().getPath()); | |||
| List<FileStatus> ret = new ArrayList<>(); | |||
| // an operation to look up a path status and add it to the return list | |||
| if (getFileSystem().delete(path, true)) { | ||
| LOG.info("Deleted trash checkpoint: {}", path); | ||
| } else { | ||
| LOG.warn("Couldn't delete checkpoint: {}. Ignoring.", path); |
There was a problem hiding this comment.
add a check to see if path exists. if it doesn't exist any more, just log at info rather than warn
|
|
||
| /** | ||
| * parse the name of a checkpoint to extgact its timestamp. | ||
| * Uses the Hadoop 0.23 checkpoint as well as the older version (!). |
There was a problem hiding this comment.
could be time to return the older version code if it is over complex
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
Follow up from #4729
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?