[
https://issues.apache.org/jira/browse/HADOOP-14756?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16431586#comment-16431586
]
Aaron Fabbri commented on HADOOP-14756:
---------------------------------------
Some comments on v1 patch:
{noformat}
+++
b/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBMetadataStore.java
@@ -202,6 +202,8 @@
static final String STATUS = "status";
@VisibleForTesting
static final String TABLE = "table";
+ @VisibleForTesting
+ static final String METADATASTORE_AUTHORITATIVE = "authoritative";
{noformat}
We'd like to be able to query *any* MetadataStore implementation whether or not
it supports persisting the {{isAuthoritative}} bit set by
{{put(DirListingMetadata)}}, so I'd suggest:
- Putting this constant somewhere else. Maybe just a separate class
"MetadataStoreCapabilities". That way, {{LocalMetadataStore}} and other future
back-ends can use it without depending on Dynamo class.
- Naming the constant "PERSISTS_AUTHORITATIVE_BIT". Want to be as clear as
possible since this is a confusing part of the S3Guard codebase. It doesn't
mean the MetadataStore is source of truth, only that it will persist the
isAuthoritative bit.
- Document the constant, including that if a MetadataStore's
getDiagnositics(PERSISTS_AUTHORITATIVE_BIT) returns null, that is interpreted
as {{false}}.
{noformat}
- // TODO HADOOP-14756 instrument MetadataStore for asserting & testing
dirMeta = ms.listChildren(strToPath("/a1/b1"));
- if (!allowMissing() || dirMeta != null) {
+ if (isMetadataStoreAuthoritative()) {
assertListingsEqual(dirMeta.getListing(), "/a1/b1/file1", "/a1/b1/file2",
"/a1/b1/c1");
}
}
{noformat}
Is the authoritative bit set on /a1/b1 here? I'm not following the logic here.
I would create a separate test for authoritative mode, say
"testListChildrenAuthoritative()". Also make sure
{{isMetadataStoreAuthoritative()}} handles null values from getDiagnostics(). I
can help give some suggestions if it is not obvious what needs testing.
FYI the {{allowMissing()}} predicate is only to avoid flaky tests when a
MetadataStore may discard old results. Technically all implementations do
sometimes discard state: LocalMetadataStore is MRU of fixed max-size, and
Dynamo can drop old stuff when prune() is called. Only the first case (Local)
affects tests, though. There might be a better way to handle this than
{{allowMissing()}}. (BTW NullMetadataStore also counts here: it always discards
everything immediately but still meets the logical criteria to being a
*correct* MetadataStore).
> S3Guard: expose capability query in MetadataStore and add tests of
> authoritative mode
> -------------------------------------------------------------------------------------
>
> Key: HADOOP-14756
> URL: https://issues.apache.org/jira/browse/HADOOP-14756
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
> Affects Versions: 3.0.0-beta1
> Reporter: Steve Loughran
> Assignee: Gabor Bota
> Priority: Major
> Attachments: HADOOP-14756.001.patch
>
>
> {{MetadataStoreTestBase.testListChildren}} would be improved with the ability
> to query the features offered by the store, and the outcome of {{put()}}, so
> probe the correctness of the authoritative mode
> # Add predicate to MetadataStore interface
> {{supportsAuthoritativeDirectories()}} or similar
> # If #1 is true, assert that directory is fully cached after changes
> # Add "isNew" flag to MetadataStore.put(DirListingMetadata); use to verify
> when changes are made
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
