[
https://issues.apache.org/jira/browse/HADOOP-15079?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16321527#comment-16321527
]
Aaron Fabbri commented on HADOOP-15079:
---------------------------------------
Fragile bits of code.. thanks for digging in to this.
Curious.. why is {{copyFile()}} {{Mixed}} instead of {{Translated}}?
Me: Looks gift horse in the mouth "How did these unrelated annotation changes
get in here? ;-)
{noformat}
- @Retries.OnceTranslated
+ @Retries.OnceTranslated("s3guard not retrying")
{noformat}
Not always retrying, yeah, until we finish HADOOP-13761. I'll add a reminder
comment there to clean this up.
Also, below this, you remove the try / catch / translateException() outer block
but claim it is still {{Translated}}
{noformat}
public RemoteIterator<LocatedFileStatus> listLocatedStatus(final Path f,
final PathFilter filter)
throws FileNotFoundException, IOException {
<snip>
- try {
<snip>
- listing.createFileStatusListingIterator(path,
- createListObjectsRequest(key, "/"),
<snip>
- } catch (AmazonClientException e) {
- throw translateException("listLocatedStatus", path, e);
{noformat}
Looking at createFileStatusListingIterator() above, it creates an
ObjectListingIterator() which calls the {{RetryRaw}}-annotated listObjects().
It does indeed appear to be Raw thus I think you've changed listLocatedStatus()
to Mixed.
{noformat}
// issue: do we need to do this, given that createFakeDirectory will do
that?
S3Guard.makeDirsOrdered(metadataStore, metadataStoreDirs, username, true);
{noformat}
Humm.. Now that finishedWrite() does S3Guard.addAncestors() (IIRC it did not
originally), maybe we don't need this and the whole makeDirsOrdered() thing can
go away. A FS with real directories would still probably want to use that
function but .. it'll always be in git history. I'm fine with trying it out
here, or doing a separate JIRA, 'sup to you.
{noformat}
+ boolean outcome = innerDelete(innerGetFileStatus(f, true), recursive);
+ maybeCreateFakeParentDirectory(f);
+ return outcome;
{noformat}
Do you want an {{if (outcome) }} condition around maybeCreateFakeParent()? I
guess it does its own check for root so probably not necessary.
Moving this out of innerDelete() looks good.
{quote}
fixed up equality check bug in rename() which was comparing parent paths on
object identity, not path, so not skipping if src and dest were in same dir
unless they shared a path instance (which in tests, they did!)
{quote}
Wow, good catch. Looks like that has been there since S3A was committed.
Your tests sharing a path instance implies the you were renaming from root dir
to root dir (the only case {{Path#getParent()}} doesn't allocate a new
{{Path}}). Sound right?
{noformat}
- // this is complicated because getParent(a/b/c/) returns a/b/c, but
- // we want a/b. See HADOOP-14428 for more details.
- deleteUnnecessaryFakeDirectories(new Path(f.toString()).getParent());
return true;
}
{noformat}
Ok. I confirmed that {{S3AFileSystem.qualify(Path)}} does strip trailing slash
so you are not reintroducing HADOOP-14428.
Off to dinner.. Will test and finish review later.
> ITestS3AFileOperationCost#testFakeDirectoryDeletion failing after
> OutputCommitter patch
> ---------------------------------------------------------------------------------------
>
> Key: HADOOP-15079
> URL: https://issues.apache.org/jira/browse/HADOOP-15079
> Project: Hadoop Common
> Issue Type: Sub-task
> Affects Versions: 3.1.0
> Reporter: Sean Mackrory
> Assignee: Steve Loughran
> Priority: Critical
> Attachments: HADOOP-15079-001.patch
>
>
> I see this test failing with "object_delete_requests expected:<1> but
> was:<2>". I printed stack traces whenever this metric was incremented, and
> found the root cause to be that innerMkdirs is now causing two calls to
> delete fake directories when it previously caused only one. It is called once
> inside createFakeDirectory, and once directly inside innerMkdirs later:
> {code}
> at
> org.apache.hadoop.fs.s3a.S3AInstrumentation.incrementCounter(S3AInstrumentation.java:454)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.incrementStatistic(S3AFileSystem.java:1108)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.lambda$deleteObjects$8(S3AFileSystem.java:1369)
> at
> org.apache.hadoop.fs.s3a.Invoker.retryUntranslated(Invoker.java:313)
> at
> org.apache.hadoop.fs.s3a.Invoker.retryUntranslated(Invoker.java:279)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.deleteObjects(S3AFileSystem.java:1366)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.removeKeys(S3AFileSystem.java:1625)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.deleteUnnecessaryFakeDirectories(S3AFileSystem.java:2634)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.finishedWrite(S3AFileSystem.java:2599)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.putObjectDirect(S3AFileSystem.java:1498)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.lambda$createEmptyObject$11(S3AFileSystem.java:2684)
> at org.apache.hadoop.fs.s3a.Invoker.once(Invoker.java:108)
> at org.apache.hadoop.fs.s3a.Invoker.lambda$retry$3(Invoker.java:259)
> at
> org.apache.hadoop.fs.s3a.Invoker.retryUntranslated(Invoker.java:313)
> at org.apache.hadoop.fs.s3a.Invoker.retry(Invoker.java:255)
> at org.apache.hadoop.fs.s3a.Invoker.retry(Invoker.java:230)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.createEmptyObject(S3AFileSystem.java:2682)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.createFakeDirectory(S3AFileSystem.java:2657)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.innerMkdirs(S3AFileSystem.java:2021)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.mkdirs(S3AFileSystem.java:1956)
> at org.apache.hadoop.fs.FileSystem.mkdirs(FileSystem.java:2305)
> at
> org.apache.hadoop.fs.contract.AbstractFSContractTestBase.mkdirs(AbstractFSContractTestBase.java:338)
> at
> org.apache.hadoop.fs.s3a.ITestS3AFileOperationCost.testFakeDirectoryDeletion(ITestS3AFileOperationCost.java:209)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.lang.reflect.Method.invoke(Method.java:498)
> at
> org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
> at
> org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
> at
> org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
> at
> org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
> at
> org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
> at
> org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
> at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
> at
> org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74
> {code}
> {code}
> at
> org.apache.hadoop.fs.s3a.S3AInstrumentation.incrementCounter(S3AInstrumentation.java:454)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.incrementStatistic(S3AFileSystem.java:1108)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.lambda$deleteObjects$8(S3AFileSystem.java:1369)
> at
> org.apache.hadoop.fs.s3a.Invoker.retryUntranslated(Invoker.java:313)
> at
> org.apache.hadoop.fs.s3a.Invoker.retryUntranslated(Invoker.java:279)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.deleteObjects(S3AFileSystem.java:1366)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.removeKeys(S3AFileSystem.java:1625)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.deleteUnnecessaryFakeDirectories(S3AFileSystem.java:2634)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.innerMkdirs(S3AFileSystem.java:2025)
> at
> org.apache.hadoop.fs.s3a.S3AFileSystem.mkdirs(S3AFileSystem.java:1956)
> at org.apache.hadoop.fs.FileSystem.mkdirs(FileSystem.java:2305)
> at
> org.apache.hadoop.fs.contract.AbstractFSContractTestBase.mkdirs(AbstractFSContractTestBase.java:338)
> at
> org.apache.hadoop.fs.s3a.ITestS3AFileOperationCost.testFakeDirectoryDeletion(ITestS3AFileOperationCost.java:209)
> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.lang.reflect.Method.invoke(Method.java:498)
> at
> org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
> at
> org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
> at
> org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
> at
> org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
> at
> org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
> at
> org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
> at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
> at
> org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
