Aaron Fabbri commented on HADOOP-14756:

Thanks for the v2 patch!
Right now the MetadataStore#getDiagnostics javadoc describes that the 
information from the returned map is for debugging only. If this information 
still valid? If it is true, is it really a good place to store capabilities 
I guess we are expanding the definition to "for debugging and testing only". We 
can add an explicit "getProperty()" API later on if we want. For now I am OK 
with using the getDiagnostics method.

I use final class with private constructor for MetadataStoreCapabilities - to 
store constants because I've seen in the project this is the general way (e.g 
org.apache.hadoop.fs.s3a.Constants) to store constants. Is this sufficient, or 
should I use interface - where all String constants are public static final by 
default? Which choice is preferred?
Not sure it matters, but I slightly prefer {{final class}}, as it seems to 
capture intent better (not an interface to be implemented).

@@ -1154,6 +1154,8 @@ private static void checkPathMetadata(PathMetadata meta) {
       map.put(READ_CAPACITY, throughput.getReadCapacityUnits().toString());
       map.put(WRITE_CAPACITY, throughput.getWriteCapacityUnits().toString());
       map.put(TABLE, desc.toString());
+      map.put(MetadataStoreCapabilities.PERSISTS_AUTHORITATIVE_BIT,
+        Boolean.toString(true));
This is actually {{false}}, and you will know your test is correct when it 
fails if this one is set to {{true}}.

+  @Test
+  public void testListChildrenAuthoritative() throws IOException {
+    Assume.assumeFalse("Missing elements should not be allowed "
+        + "to run this test.", allowMissing());
Correct, but it would be nice to run this test on LocalMetadataStore, since it 
is currently the only one that persists the authoritative bit.  You might want 
to switch this to something like {{Assume.assumeFalse(fs.hasMetadataStore())}}.

+    Assume.assumeTrue("MetadataStore should be capable for authoritative "
+        + "storage of directories to run this test.",
+        isMetadataStoreAuthoritative());
+    setupListStatus();
+    DirListingMetadata dirMeta = ms.listChildren(strToPath("/a1/b1"));
+    dirMeta.setAuthoritative(true);
+    dirMeta.put(makeFileStatus("/a1/b1/file_new", 100));
+    ms.put(dirMeta);
+    assertTrue(dirMeta.isAuthoritative());
+    dirMeta = ms.listChildren(strToPath("/a1/b1"));
+    assertListingsEqual(dirMeta.getListing(), "/a1/b1/file1", "/a1/b1/file2",
+        "/a1/b1/c1", "/a1/b1/file_new");
+  }
I think if you move that last {{assertTrue(dirMeta.isAuthoritative())}} to the 
end of the function it will be correct.  As is you are not testing to see if 
the bit was persisted by the {{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, HADOOP-14756.002.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