[
https://issues.apache.org/jira/browse/HADOOP-13651?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15622984#comment-15622984
]
Steve Loughran edited comment on HADOOP-13651 at 10/31/16 6:45 PM:
-------------------------------------------------------------------
comments on patch 003
{{LocalMetadataStore.listChildren(). get(), put}} are going to be *very*
expensive as the iterative loop is called even when the logging doesn't take
place. Wrap with a LOG.isDebugEnabled(), or have {{DirListingMetadata}}
implement a toString() method with all the log info, and just reference that.
There's a fairly complex interdependency between S3AFS and the metadatastore
(now that LocalMetadataStore is checking for its FS being S3A). The is empty
dir logic here is getting ugly already. I think it may be best to revisit that
entire empty-dir logic and see how it can be moved into this world, rather than
just making things more complex. Currently, there's a couple of main ways the
{{isEmptyDirectory}} bit is used
* when deciding whether to delete an entry during parent delete walks
{{deleteUnnecessaryFakeDirectories}}. This code has already been replaced in
trunk.
* when validating some operations which only apply to an empty directory. It's
essentially being a shortcut to for the predicate "has-no-children", which
again, is expensive in s3-land. If, instead of asking of the s3a status, it was
just something which could be queried off the metadata store, then it gets to
implement the logic behind {{S3Guard.isEmptyDirectory(metadatastore,
s3afilestatus)}}
The base metadatastore (which would have to be renamed something like
DefaultMetadataStore) would implement its check by passthrough from the file
status:
{code}
boolean isEmptyDirectory(S3AFileStatus stat) { return stat.isEmptyDirectory;}
{code}
Other implementations can actually do a listing. It should be possible to
require that there should be no accesses of that flag in the status except
through an MD store class. ({{S3AFileStatus}}) is tagged as private/evolving,
no external code should be using that field.
Finally, now that this starts hooking up to S3, it's going to need to have a
security story consistent with S3A. Which is currently: you get R/O or R/W
filesystems, as well as filesystems an unauthed user may not read. We expect
all FS operations to fail on an unauthed user; if they have read only rights
then mkdirs/delete, rename and file writing must all fail, leaving the FS in
the same state it was before. Which implies that (a) there will have to be
isolation between users and (b) things which update the MD store after, say ,
"delete", will have to take place after the s3 call succeeds, doing nothing on
a failure.
That should be testable: try to delete the landsat CSV, verify that it is still
there on the next list/stat/open. Do bear in mind that other test infras may
not have that file, or supply one in an R/W bucket ( a new complication, given
there aren't yet any tests for attempting a write in an R/O bucket).
was (Author: [email protected]):
comments on patch 003
{{LocalMetadataStore.listChildren(). get(), put}} are going to be *very*
expensive as the iterative loop is called even when the logging doesn't take
place. Wrap with a LOG.isDebugEnabled(), or have {{DirListingMetadata}}
implement a toString() method with all the log info, and just reference that.
There's a fairly complex interdependency between S3AFS and the metadatastore
(now that LocalMetadataStore is checking for its FS being S3A). The is empty
dir logic here is getting ugly already. I think it may be best to revisit that
entire empty-dir logic and see how it can be moved into this world, rather than
just making things more complex. Currently, there's a couple of main ways the
{{isEmptyDirectory}} bit is used
* when deciding whether to delete an entry during parent delete walks
{{deleteUnnecessaryFakeDirectories}}. This code has already been replaced in
trunk.
* when validating some operations which only apply to an empty directory. It's
essentially being a shortcut to for the predicate "has-no-children", which
again, is expensive in s3-land. If, instead of asking of the s3a status, it was
just something which could be queried off the metadata store, then it gets to
implement the logic.
i.e {{S3Guard.isEmptyDirectory(metadatastore, s3afilestatus).
The base metadatastore (which would have to be renamed something like
DefaultMetadataStore) would implement its check by passthrough from the file
status: {{boolean isEmptyDirectory(S3AFileStatus stat) { return
stat.isEmptyDirectory;} }}. Other implementations can actually do a listing. It
should be possible to require that there should be no accesses of that flag in
the status except through an MD store class. ({{S3AFileStatus}}) is tagged as
private/evolving, no external code should be using that field.
Finally, now that this starts hooking up to S3, it's going to need to have a
security story consistent with S3A. Which is currently: you get R/O or R/W
filesystems, as well as filesystems an unauthed user may not read. We expect
all FS operations to fail on an unauthed user; if they have read only rights
then mkdirs/delete, rename and file writing must all fail, leaving the FS in
the same state it was before. Which implies that (a) there will have to be
isolation between users and (b) things which update the MD store after, say ,
"delete", will have to take place after the s3 call succeeds, doing nothing on
a failure.
That should be testable: try to delete the landsat CSV, verify that it is still
there on the next list/stat/open. Do bear in mind that other test infras may
not have that file, or supply one in an R/W bucket ( a new complication, given
there aren't yet any tests for attempting a write in an R/O bucket).
> S3Guard: S3AFileSystem Integration with MetadataStore
> -----------------------------------------------------
>
> Key: HADOOP-13651
> URL: https://issues.apache.org/jira/browse/HADOOP-13651
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
> Reporter: Aaron Fabbri
> Assignee: Aaron Fabbri
> Attachments: HADOOP-13651-HADOOP-13345.001.patch,
> HADOOP-13651-HADOOP-13345.002.patch, HADOOP-13651-HADOOP-13345.003.patch
>
>
> Modify S3AFileSystem et al. to optionally use a MetadataStore for metadata
> consistency and caching.
> Implementation should have minimal overhead when no MetadataStore is
> configured.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
