hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Mackrory (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-14457) create() does not notify metadataStore of parent directories or ensure they're not existing files
Date Tue, 13 Jun 2017 20:41:00 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-14457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16048376#comment-16048376
] 

Sean Mackrory commented on HADOOP-14457:
----------------------------------------

Had lot of back-and-forth about this offline with [~fabbri]. Notes are below. Would like some
input from [~liuml07] / [~stevel@apache.org] about this since either solution involves adding
to the MetadataStore interface. The gist is this:

- I think it's reasonable to only do this if authoritative mode is enabled.
- I'm concerned about code complexity, and having the Dynamo DB implementation add all ancestors,
and then replicate the same thing under certain conditions in the FS only for implementations
that don't do this. I suspect future implementations are more likely than not to require the
tree to be traversable from root like DynamoDB does. I'm actually not clear why Local doesn't
have this same problem, unless it's just that we don't care about doing a full-table scan
on an in-memory hashmap.
- [~fabbri] is concerned about performance and wants the MetadataStore interface to be as
simple as possible to make future implementations easy to add, so having more of the logic
live in the FS when possible is desirable.

There are 2 options we discussed:

- Add putRecursive to the metadata store interface and use that in finishedWrite() instead
of having it loop up the directory hierarchy itself. For Dynamo or similar implementations,
it can just wrap put. Mkdir might be able to use it, although it already traverses the tree
making sure none of the parents are a file anyway, similar to what create() should eventually
do.

- Go with the change as-is, but wrap the finishedWrite changes in a capabilities API so that
it only adds ancestors if the parent needs them and won't do it itself.

Personally, I prefer the former. It addresses my concerns about adding recursive operations
at multiple points in the code (I think it's more likely than not that any future implementations
will have the same need for DynamoDB to have the entire tree be traversable from root anyway),
the FS doesn't need to decide whether or not it should recurse because it can always assume
the underlying operation will do it the first time it's called, and the one point that operation
is implemented can take advantage of any store-specific optimizations rather than something
unaware of the storage layer just making repeated calls.

On the other hand, if we feel like we're going to eventually need a capabilities API anyway,
we may as well add it now and use that to solve this problem too.

Any thoughts, [~stevel@apache.org] / [~liuml07]?

Any nuances I didn't call out here, [~fabbri]?

> create() does not notify metadataStore of parent directories or ensure they're not existing
files
> -------------------------------------------------------------------------------------------------
>
>                 Key: HADOOP-14457
>                 URL: https://issues.apache.org/jira/browse/HADOOP-14457
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: fs/s3
>            Reporter: Sean Mackrory
>            Assignee: Sean Mackrory
>         Attachments: HADOOP-14457-HADOOP-13345.001.patch, HADOOP-14457-HADOOP-13345.002.patch,
HADOOP-14457-HADOOP-13345.003.patch, HADOOP-14457-HADOOP-13345.004.patch, HADOOP-14457-HADOOP-13345.005.patch,
HADOOP-14457-HADOOP-13345.006.patch, HADOOP-14457-HADOOP-13345.007.patch, HADOOP-14457-HADOOP-13345.008.patch,
HADOOP-14457-HADOOP-13345.009.patch
>
>
> Not a great test yet, but it at least reliably demonstrates the issue. LocalMetadataStore
will sometimes erroneously report that a directory is empty with isAuthoritative = true when
it *definitely* has children the metadatastore should know about. It doesn't appear to happen
if the children are just directory. The fact that it's returning an empty listing is concerning,
but the fact that it says it's authoritative *might* be a second bug.
> {code}
> diff --git a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
> index 78b3970..1821d19 100644
> --- a/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
> +++ b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
> @@ -965,7 +965,7 @@ public boolean hasMetadataStore() {
>    }
>  
>    @VisibleForTesting
> -  MetadataStore getMetadataStore() {
> +  public MetadataStore getMetadataStore() {
>      return metadataStore;
>    }
>  
> diff --git a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/ITestS3AContractRename.java
b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/ITestS3AContractRename.java
> index 4339649..881bdc9 100644
> --- a/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/ITestS3AContractRename.java
> +++ b/hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/ITestS3AContractRename.java
> @@ -23,6 +23,11 @@
>  import org.apache.hadoop.fs.contract.AbstractFSContract;
>  import org.apache.hadoop.fs.FileSystem;
>  import org.apache.hadoop.fs.Path;
> +import org.apache.hadoop.fs.s3a.S3AFileSystem;
> +import org.apache.hadoop.fs.s3a.Tristate;
> +import org.apache.hadoop.fs.s3a.s3guard.DirListingMetadata;
> +import org.apache.hadoop.fs.s3a.s3guard.MetadataStore;
> +import org.junit.Test;
>  
>  import static org.apache.hadoop.fs.contract.ContractTestUtils.dataset;
>  import static org.apache.hadoop.fs.contract.ContractTestUtils.writeDataset;
> @@ -72,4 +77,24 @@ public void testRenameDirIntoExistingDir() throws Throwable {
>      boolean rename = fs.rename(srcDir, destDir);
>      assertFalse("s3a doesn't support rename to non-empty directory", rename);
>    }
> +
> +  @Test
> +  public void testMkdirPopulatesFileAncestors() throws Exception {
> +    final FileSystem fs = getFileSystem();
> +    final MetadataStore ms = ((S3AFileSystem) fs).getMetadataStore();
> +    final Path parent = path("testMkdirPopulatesFileAncestors/source");
> +    try {
> +      fs.mkdirs(parent);
> +      final Path nestedFile = new Path(parent, "dir1/dir2/dir3/file4");
> +      byte[] srcDataset = dataset(256, 'a', 'z');
> +      writeDataset(fs, nestedFile, srcDataset, srcDataset.length,
> +          1024, false);
> +
> +      DirListingMetadata list = ms.listChildren(parent);
> +      assertTrue("MetadataStore falsely reports authoritative empty list",
> +          list.isEmpty() == Tristate.FALSE || !list.isAuthoritative());
> +    } finally {
> +      fs.delete(parent, true);
> +    }
> +  }
>  }
> {code}



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message