hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron Fabbri (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 Fri, 09 Jun 2017 00:08:18 GMT

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

Aaron Fabbri commented on HADOOP-14457:
---------------------------------------

Thank you for being flexible and splitting this up [~mackrorysd], it is much easier to digest.

In {{S3Guard#addAncestors()}}, which is called from {{S3AFileSystem#finishedWrite()}}:

{code}
+      PathMetadata directory = metadataStore.get(parent);
+      if (directory == null || directory.isDeleted()) {
+        metadataStoreDirs.add(parent);
+      } else {
+        if (directory.getFileStatus().isDirectory()) {
+          break;
+        } else {
+          throw new FileAlreadyExistsException("Parent directory " + parent +
+              " already exists as a file");
+        }
{code}

I would probably just make this whole else block do a {{break}}, and remove the inner if/else
that checks if ancestor is a directory.  A couple of reasons I think enforcing that ancestors
have to be directories here is bad:

1. It is a fail on close(), after data has been written.  This is the big one: failure should
mean the file was not materialized.
2. It only will fail when S3Guard is enabled *and* we happen to have metadata loaded for the
ancestor file.  So the enforcement is situational.
3. The FS client should be doing this, if it is done at all, IMO.

 I can't think of any issues this causes with MetadataStore implementations, if we just break
here and ignoring file ancestors.  Shout if you think otherwise.

Also, for the createNonRecursive() case, this work should not be necessary.  We'd need some
operation context / plumbing to know in finishedWrite() though (I think).  I was already thinking
of plumbing some more state through the operation (to record stats on inconsistency, a la
HADOOP-14467 and HADOOP-14468), so we can address this part with a followup JIRA assigned
to me.

For the new test case {{testCreatePopulatesFileAncestors()}}, can you move it to another test
class?  I recently moved some unrelated tests out of this class and clarified its purpose
in the class comment.  I would give your case its own class, actually, with a class comment
to the effect of
{code}
/**
 * Test that ancestor creation during S3AFileSystem#create() is properly accounted 
 * for in the MetadataStore.  This should be handled by the FileSystem, and be a FS contract
 * test, but S3A does not handle ancestors on create(), so we need to take care in the S3Guard

 * code to do the right thing.  This may change: See HADOOP-13221 for more detail.
 */
{code}

About the new boolean parameter here:
{code}
+   * @param authoritative Whether to mark new directories as authoritative.
    */
   public static void makeDirsOrdered(MetadataStore ms, List<Path> dirs,
-      String owner) {
+      String owner, boolean authoritative) {
{code}

This makes sense.  An unfortunate side effect of the FS not enforcing this stuff.  Usually
if we're making directories, we by definition know their full contents (leaf is empty, ancestors
all have one entry for child).  In this case we do not know if the directories already exist
on the underlying FS, so we have to mark them as "fully cached: unknown".  Ok.

{code}
+      assertTrue("MetadataStore falsely reports authoritative empty list",
+          list.isEmpty() == Tristate.FALSE);
{code}

This should pass with current implementations, but  {{ != Tristate.TRUE }} is more precise.
 A MetadataStore is not *required* to know directory emptiness. 



> 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
>
>
> 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.3.15#6346)

---------------------------------------------------------------------
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