Aaron Fabbri commented on HADOOP-14756:

Some comments on v1 patch:
@@ -202,6 +202,8 @@
   static final String STATUS = "status";
   static final String TABLE = "table";
+  @VisibleForTesting
+  static final String METADATASTORE_AUTHORITATIVE = "authoritative";

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

-    // 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",

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

To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

Reply via email to