HADOOP-16380. S3A non-recursive deletion of root to return false #1106
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
|
Tests run against ireland with known tesMRJob errors. There was an error but cleared on the rerun |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
added
bug
fs/s3
| @@ -2193,6 +2193,13 @@ void removeKeys( | |||
| */ | |||
| @Retries.RetryTranslated | |||
| public boolean delete(Path f, boolean recursive) throws IOException { | |||
| if (f.isRoot()) { | |||
There was a problem hiding this comment.
this should go after the entrypoint, so that the metrics are incremented and FS open/closed state are checked
|
I need to look at the source a bit more here; I think we need to make sure the behaviour on an rm (root, recursive=true) is handled properly too, which for s3a can/should just be an outright rejection This is probably time to make the behaviour on the rm root operations something to not just clarify a bit more in filesystem.md but also in the contract options for all filestores, define what happens. Yes, I am making life hard for you here, but this is how we nail down the corner cases of what filesystems are meant to do |
Document that object stores will return false on deletion of root
|
I have an integration test failure with this patch: I'll take a look at this tomorrow. |
|
🎊 +1 overall
This message was automatically generated. |
|
fixed last failing test. tested against ireland, no failures other than testMRJob |
|
💔 -1 overall
This message was automatically generated. |
| @@ -2195,6 +2195,10 @@ void removeKeys( | |||
| public boolean delete(Path f, boolean recursive) throws IOException { | |||
| try { | |||
| entryPoint(INVOCATION_DELETE); | |||
| if (f.isRoot()) { | |||
There was a problem hiding this comment.
you're going to have to qualify the path. this is done in getFilestatus() in the main delete codepath, but now you'll have to do it first (you can reuse that path in L2199 and L2202, obviously)
Thinking about this, all we need is your change to rejectRootDirectoryDelete(), without needing any other changes to the delete/innerDelete codepath. Yes, we'll do a needless getFileStatus and return the wrong empty flag if there's a tombstone, but that "feature" is still there no matter what. And I don't care about performance here given the operation will always be rejected
|
-1 as is, because we need that qualified path. I'd also like the empty dir test of PR #1079 retained. Even though for the root directory it is now moot, its critical we have the test to ensure that there's never any regression in the empty dir checks for child entries. Let me take this over by merging this PR into mine, modifying my new test to test a child dir where the empty dir checks MUST work |
|
🎊 +1 overall
This message was automatically generated. |
|
This has been superceded by #1123 which is built off this PR. Closing this one to avoid confusion |
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
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.
No description provided.