[
https://issues.apache.org/jira/browse/HADOOP-14236?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15944467#comment-15944467
]
Aaron Fabbri commented on HADOOP-14236:
---------------------------------------
Thanks for tagging me [~liuml07]. Good work on this one.
I want to make sure I'm not confused here. I will explain what I think the bug
is. Please correct me where needed:
Conceptually, MetadataStore deals with paths as a normal filesystem. S3A, on
the other hand, does some strange things with directory emulation, where some
intermediate (non-leaf) directories are inferred from other keys. For example,
if /a/b/file exists, we *infer* the existence of /a and /a/b, even though we do
not store an entry (object) for /a nor for /a/b.
S3AFileSystem needs to drive each storage interface correctly: When accessing
MetadataStore, all directories need to be explicitly specified. For example,
in mkdirs(/a/b/c/file), we will put all the directories created into the
MetadataStore, yet only store a single object in S3 (and infer the directories
by key search).
In rename(), we need to ensure that any *inferred* directories are also moved.
However, the implementation uses {{listObjects()}}, which does not enumerate
intermediate, or "inferred" directories.
The fix should be to make sure that intermediate directories are enumerated
when building list of moves for MetadataStore. An intermediate directory is
any non-leaf directory that begins with the source path supplied to rename.
For example:
Assume we have a bucket with the following object:
/a/source/c/d/file
And then we
rename(/a/source, /dest)
Current code will only provide the following moves to MetadataStore, since it
depends on S3's {{listObjects()}}:
{/a/source/c/d/file -> /dest/c/d/file}
leaving /a/source/c and /a/source/c/d present in the MetadataStore, when they
should have been moved under /dest.
Your fix looks pretty good, except it only seems to affect the file case, and
not the directory case. Change my example above to
/a/source/c/d/directory
and I think your code will skip /a/source/c and /a/source/c/d, since you only
add the "intermediate" or "inferred" directories when the leaf is a file?
For your test code, I'd suggest creating a new FileSystem-only test case that
exercises the bug. Would my simple example above work?
The MetadataStore asserts need a check for
{{AbstractMSContract#allowMissing()}}, as your asserts that the new paths are
present may fail for some MetadataStore implementations (e.g. ones that are
short-term caches will end up being flaky tests). I'd suggest moving
MetadataStore test logic to MetadataStoreTestBase and subclasses to get access
to allowMissing(). I'd be happy to help with the minor test refactoring here.
One final thought: Consider moving the "add inferred directories" logic into a
helper function. It is basically your while loop that adds the missing
directories.
> S3Guard: S3AFileSystem::rename() should move non-listed sub-directory entries
> in metadata store
> -----------------------------------------------------------------------------------------------
>
> Key: HADOOP-14236
> URL: https://issues.apache.org/jira/browse/HADOOP-14236
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
> Reporter: Mingliang Liu
> Assignee: Mingliang Liu
> Priority: Critical
> Attachments: HADOOP-14236-HADOOP-13345.000.patch,
> HADOOP-14236-HADOOP-13345.001.patch, HADOOP-14236-HADOOP-13345.002.patch
>
>
> After running integration test {{ITestS3AFileSystemContract}}, I found the
> following items are not cleaned up in DynamoDB:
> {code}
> parent=/mliu-s3guard/user/mliu/s3afilesystemcontract/testRenameDirectoryAsExisting/dir,
> child=subdir
> parent=/mliu-s3guard/user/mliu/s3afilesystemcontract/testRenameDirectoryAsExistingNew/newdir/subdir,
> child=file2
> {code}
> At first I thought it’s similar to [HADOOP-14226] or [HADOOP-14227], and we
> need to be careful when cleaning up test data.
> Then I found it’s a bug(?) in the code of integrating S3Guard with
> S3AFileSystem: for rename we miss sub-directory items to put (dest) and
> delete (src). The reason is that in S3A, we delete those fake directory
> objects if they are not necessary, e.g. non-empty. So when we list the
> objects to rename, the object summaries will only return _file_ objects. This
> has two consequences after rename:
> # there will be left items for src path in metadata store - left-overs will
> confuse {{get(Path)}} which should return null
> # we are not persisting the whole subtree for dest path to metadata store -
> this will break the DynamoDBMetadataStore invariant: _if a path exists, all
> its ancestors will also exist in the table_.
> UPDATE: modified test case {{ITestS3AFileSystemContract::
> testRenameDirectoryAsExistingDirectory()}} will fail w/o this patch; it
> passes w/ this patch. If the test case makes sense, the proposal follows.
> Existing tests are not complaining about this though. If this is a real bug,
> let’s address it here.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
