HADOOP-16380 S3Guard to determine empty directory status for all non-root directories #1123
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
Document that object stores will return false on deletion of root
This merges gabor's PR about the root dir with my test case, reworking the test case to work with subdirectories. Where, sadly, it shows the problem exists. If you have a file under a tombstone and that tombstone is the first item in a directory listing, then the directory will be mistaken for being empty. Change-Id: Id02e821539f319491bd859fe0596913ce5d38287
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
This fixes the problem by having S3Guard.getWithTtl accept and then pass down the empty directory flag; mock tests updated to be in sync Change-Id: Ib5b545e3c0741d6fb3a84015d8335ba9e0914ab7
|
Updated patch fixes the invocation chain so needEmptyDirectoryFlag gets passed through. New test now works happily. Tested: s3 ireland with s3guard and dynamo. I've not done the -scale option; kicking it off now |
steveloughran
changed the title
|
Patch tested at scale, S3 ireland Latest patch
|
uses MetricDiffs to verify that the getFileStatus call doesn't issue a GET or LIST to S3. This ensures that the test success isn't due to some change in how tombstones and s3 entries are resolved -it is because, when DDB is used, there are no queries to the filesystem at this point. This is a better probe as it verifies the desired behaviour rather than just the outcome Change-Id: I6ee719ebfc95698e867972685cfad478d7f27ec2
|
Latest patch: full test run @ scale with Ireland + S3Guard + DDB; ran the empty dirs test with local: |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bgaborg
approved these changes
Jul 22, 2019
There was a problem hiding this comment.
+1; Change is OK, @mackrorysd could review for a second option though
| @@ -2623,7 +2609,8 @@ S3AFileStatus innerGetFileStatus(final Path f, | |||
| // Check MetadataStore, if any. | |||
| PathMetadata pm = null; | |||
| if (hasMetadataStore()) { | |||
| pm = S3Guard.getWithTtl(metadataStore, path, ttlTimeProvider); | |||
| pm = S3Guard.getWithTtl(metadataStore, path, ttlTimeProvider, | |||
| needEmptyDirectoryFlag); | |||
There was a problem hiding this comment.
Good that you found this and before shipping it to a customer.
|
tested against ireland with dynamo: known testMRJob failures |
|
thanks. PR #1115 is working on the testMRJob failures; the initial patch will let us collect failures in the local FS; the test reports will also collect and print the result from the AM. and by working in the parallel phase again (yet still executing each MR job in an isolated sequence), we get a speedup of a minute or two. |
|
@bgaborg , did that "1 approval" mean a +1? If so, can you add it explicitly for the record. Thanks |
steveloughran
added
fs/s3
|
-Dscale against ireland with dynamo: some test failed, so after the rerun: Ireland -Dlocal: Summary: +1 pending on fixing org.apache.hadoop.fs.contract.s3a.ITestS3AContractRootDir#testRmEmptyRootDirNonRecursive, testListEmptyRootDirectory failures |
|
I'm going to say the terasort failures are unrelated: somehow teragen failed and the successors refuse to work at that point. Regarding the other failure, it looks potentially like a bug in the initial delete code which tried to delete a file which was already missing when it looked; that's the one rethrown as we only throw the first failure on the loop. not the final one. I'll deal with those for some better checks and reporting |
… in the listing isn't there on the deletion; and to keep retrying even when the metastore is enabled If things fail now, I'm curious about what is happening Change-Id: I00eb7bbb66773a5661616b8130435d598b2cebc7
|
🎊 +1 overall
This message was automatically generated. |
|
+1 - retested with ireland |
|
thanks; I'll fix the checkstyle on the commit |
|
committed to trunk. |
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.
Gabor's PR #1106 merged with the test of mine in #1079 with that test modified to show that yes, the tombstone problem goes all the way down
Fix: pass down the needEmptyDirectory flag all the way down from S3AFileSystem to metastore.get