[
https://issues.apache.org/jira/browse/HADOOP-13573?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15488782#comment-15488782
]
Chris Nauroth commented on HADOOP-13573:
----------------------------------------
[~fabbri], thank you for the patch. The structure looks good overall. Here
are a few comments.
{code}
public AbstractMSContract() { };
{code}
I suggest removing this line, since Java will give us a default constructor
automatically.
{code}
try (MetadataStore ms = contract.getMetadataStore()) {
assertNotNull("null MetadataStore", ms);
ms.initialize(contract.getConf());
this.ms = ms;
}
{code}
I think we exit this block with the {{MetadataStore}} closed because of the
try-with-resources. It probably wasn't noticeable while testing with
{{LocalMetadataStore}}, because that has a no-op {{close}}.
{code}
assertDirectorySize("/ADirectory1/db1/", 1);
{code}
{code}
assertDirectorySize("/ADirectory1/db1/", 2);
{code}
I suggest removing trailing slashes from all path strings to avoid a potential
source of confusion. The {{Path}} constructor will normalize and drop the
trailing slashes anyway.
{code}
@Test
public void testDeleteRecursiveRoot() throws Exception {
setUpDeleteTest();
ms.deleteSubtree(new Path("/"));
{code}
I don't expect the {{FileSystem}} to call the {{MetadataStore}} this way,
because deletion of root should be rejected. For the DynamoDB implementation,
I don't think it can issue a meaningful delete for root, because root does not
have a parent, so it won't fit well in the schema. This is not a big deal
though, and we can revisit during the DynamoDB implementation.
{code}
assertEquals("File size as expected", meta.getFileStatus().getLen(), 100);
{code}
I think the "expected" and "actual" arguments were inverted in this assertion.
{code}
meta = ms.get(new Path("bollocks"));
{code}
Excellent test data! :-)
Suggestions for additional tests:
* {{put}} overwriting an existing path.
* {{delete}} a path that doesn't exist. (Does nothing, but should complete
without exception.)
* Same thing for {{deleteSubtree}}.
* Some tests already check the returned value of {{FileStatus#getLen}}. It
would be good to do the same for {{isEmptyDirectory}} (on directories) and
{{getModificationTime}} and {{getBlockSize}} (on files).
In general, assertions on values of {{FileStatus}} properties:
isEmptyDirectory, mtime, blockSize, length.
> S3Guard: create basic contract tests for MetadataStore implementations
> ----------------------------------------------------------------------
>
> Key: HADOOP-13573
> URL: https://issues.apache.org/jira/browse/HADOOP-13573
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
> Reporter: Aaron Fabbri
> Assignee: Aaron Fabbri
> Attachments: HADOOP-13573.001.patch
>
>
> We should have some contract-style unit tests for the MetadataStore interface
> to validate that the different implementations provide correct semantics.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
