[
https://issues.apache.org/jira/browse/HADOOP-14457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16078465#comment-16078465
]
Aaron Fabbri commented on HADOOP-14457:
---------------------------------------
Thanks for the patch [~mackrorysd]. This looks really good.
Couple comments
{noformat}
@@ -251,9 +252,10 @@ public static boolean isNullMetadataStore(MetadataStore
ms) {
* an empty, dir, and the other dirs only contain their child
* dir.
* @param owner Hadoop user name.
+ * @param authoritative Whether to mark new directories as authoritative.
*/
public static void makeDirsOrdered(MetadataStore ms, List<Path> dirs,
- String owner) {
+ String owner, boolean authoritative) {
if (dirs == null) {
return;
}
@@ -286,7 +288,7 @@ public static void makeDirsOrdered(MetadataStore ms,
List<Path> dirs,
children.add(new PathMetadata(prevStatus));
}
DirListingMetadata dirMeta =
- new DirListingMetadata(f, children, true);
+ new DirListingMetadata(f, children, authoritative);
try {
ms.put(dirMeta);
ms.put(new PathMetadata(status));
{noformat}
IIRC, the only reason I put the DirListingMetadata in addition to the
PathMetadata here, was to mark the directory as authoritative. (In mkdirs
case, we actually know we are creating the ancestors, so we can mark them as
fully cached, as we know that contain exactly one entry; the next child in the
path).
I think we can elide the ms.put(dirMeta) here (and associated creation of
DirListingMetadata) when authoritative is false. That cuts round trips in half.
We could also use the new batched put() here, which would speed things up even
more.
{noformat}
+ public static void addAncestors(MetadataStore metadataStore,
+ Path qualifiedPath, String username) throws IOException {
+ Collection<PathMetadata> newDirs = new ArrayList<>();
+ Path parent = qualifiedPath.getParent();
+ while (!parent.isRoot()) {
+ PathMetadata directory = metadataStore.get(parent);
+ if (directory == null || directory.isDeleted()) {
+ FileStatus status = new FileStatus(0, true, 1, 0, 0, 0, null, username,
+ null, parent);
+ PathMetadata meta = new PathMetadata(status, Tristate.UNKNOWN, false);
{noformat}
Being a little pedantic: This can be {{Tristate.FALSE}}: we know these are not
empty dirs since they are ancestors of other files.
{noformat}
+ newDirs.add(meta);
+ } else {
+ break;
+ }
+ parent = parent.getParent();
+ }
+ metadataStore.put(newDirs);
+ }
{noformat}
> 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,
> HADOOP-14457-HADOOP-13345.010.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: [email protected]
For additional commands, e-mail: [email protected]
