HADOOP-18801. Delete path directly when it can not be parsed in trash. #5744
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
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
befcfea
to
aec9e59
Compare
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
aec9e59
to
078150c
Compare
|
@Hexiaoqiao @ayushtkn Hi, Sir, could you please help me to review this PR if you have bandwidth? Thanks a lot. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
fix checkstyle warning. |
|
🎊 +1 overall
This message was automatically generated. |
|
@hfutatzhanghb Thanks to involve me here. I am not sure if you could solve the issue as HDFS-17046 mentioned. I think we should choose one of the following ways, |
@Hexiaoqiao , Sir, thanks a lot for your suggestions. Agree with the second approach and I will fix it soonly. Thanks again. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
87479ab
to
54c9366
Compare
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jun 20, 2023
There was a problem hiding this comment.
Thanks @hfutatzhanghb for your works. leave some comments inline. JFYI. Please let me know if something I missed.
| LOG.warn("Unexpected item in trash: "+dir+". Ignoring."); | ||
| continue; | ||
| if (cleanNonCheckpointUnderTrashRoot) { | ||
| this.moveToTrash(path, true); |
There was a problem hiding this comment.
Why move to trash again since it has been already under Trash home AND has configed to clean? IMO, it is safe to delete them directly.
| */ | ||
| public abstract boolean moveToTrash(Path path) throws IOException; | ||
| */ | ||
| public abstract boolean moveToTrash(Path path, boolean force) throws IOException; |
There was a problem hiding this comment.
Sorry I don't get meaning of the new parameter boolean force.
| * @throws Exception | ||
| */ | ||
| @Test | ||
| public void testTrashEmptierWithNonCheckpointDir() throws Exception { |
There was a problem hiding this comment.
This new unit test seems not cover the case you try to fix, right? It could pass with/without this improvement.
There was a problem hiding this comment.
For this unit test, IMO, we should enable this config and then mkdir one path under Trash home directly, then wait to clean by Emptier. JFYI.
|
🎊 +1 overall
This message was automatically generated. |
|
@Hexiaoqiao Sir, could you please take a look at this PR when you have free time? Thanks a lot. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jul 12, 2023
| continue; | ||
| if (cleanNonCheckpointUnderTrashRoot) { | ||
| fs.delete(path, true); | ||
| LOG.warn("Unexpected item in trash: " + dir + ". Force moving to trash."); |
There was a problem hiding this comment.
Force moving to trash. -> Force to delete it.
| @@ -579,6 +579,7 @@ public void testExistingFileTrash() throws IOException { | |||
| conf.setClass("fs.file.impl", TestLFS.class, FileSystem.class); | |||
| FileSystem fs = FileSystem.getLocal(conf); | |||
| conf.set("fs.defaultFS", fs.getUri().toString()); | |||
| conf.setBoolean(FS_TRASH_CLEAN_TRASHROOT_ENABLE_KEY, true); | |||
There was a problem hiding this comment.
I do not think we need to update this configuration item here. (L582,L639,L649, etc)
| * @throws Exception | ||
| */ | ||
| @Test | ||
| public void testTrashEmptierWithNonCheckpointDir() throws Exception { |
There was a problem hiding this comment.
For this unit test, IMO, we should enable this config and then mkdir one path under Trash home directly, then wait to clean by Emptier. JFYI.
| } | ||
| assertTrue(val == 0); | ||
| } | ||
| Thread.sleep(18000); |
There was a problem hiding this comment.
Please try to use GenericTestUtils.waitFor rather than Thread.sleep.
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
e6a59b1
to
1bf64b2
Compare
hfutatzhanghb
force-pushed
the
HDFS-17046
branch
from
1bf64b2
to
dcf58a7
Compare
hfutatzhanghb
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
approved these changes
Jul 14, 2023
There was a problem hiding this comment.
LGTM. +1. Let's wait if @ayushtkn will give furthermore comments before checkin.
ayushtkn
approved these changes
Jul 14, 2023
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestTrash.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/TrashPolicyDefault.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
approved these changes
Jul 15, 2023
There was a problem hiding this comment.
LGTM.
Thanx @Hexiaoqiao for holding, nothing more from my side, can be pushed :-)
|
Committed to trunk. Thanks @hfutatzhanghb for your contribution and @ayushtkn for your reviews! |
|
@Hexiaoqiao @ayushtkn Thanks sir for reviewing and merging. |
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
apache#5744). Contributed by farmmamba. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
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
Please see HADOOP-18801.
How was this patch tested?
Add an UT.