[
https://issues.apache.org/jira/browse/HADOOP-13449?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15622747#comment-15622747
]
Steve Loughran commented on HADOOP-13449:
-----------------------------------------
I'm just catching up with this, apologies if I say things that are clearly
wrong to anyone who knows the code or its history: I don't, yet.
h2. Build
# [~cnauroth] I see your point about declaring the dependency; you are correct.
It does need to be something published for downstream users.
# I do still want the AWS update to be a standalone patch, and with a matching
Jackson update. Those can perhaps be done to trunk/ itself, and merged in here,
so that any/all other trunk work will be with the upgraded artifacts.
h2. Code
Anything in source marked TODO scares me. There's a lot here. Presumably the
plan is to have them addressed by the time the patch goes in? Or at least
pulled out into explicit followup JIRAs?
h3. {{DynamoDBMetadataStore}}
* just use .* on the static imports of the s3a constant, util,
PathMetadataDynamoDBTranslation entries
* L108: no need to mix {{@code}} and {{<pre>}} tags. For multiline, {{<pre>}}
should suffice ... check with the generated javadocs to see
* L218: that endpoint map/convert logic should be pulled into a static s3a util
method, with tests.
* L465: what if close() is called twice? If a re-entrant call is made?
* L492, 531: Throw {{InterruptedIOException}}, or set the thread's interrupted
bit again. We don't want that thread interrupt to be lost if at all possible.
* Most of those info-level per-operation logs should be at Debug
* Operation param to calls of {{translateException}} could be more informative.
Consider: what info would you need there in order to debug this from the logs.
h2. Tests
As well as the unit tests, I need to be able to run the entire existing suite
with s3guard enabled. This could be done with a new maven profile which would
enable it, or simply a property passed down through the build. That's what's
done in the scale tests in trunk, using methods in {{S3ATestUtils}} to allow a
maven-defined property to override one in the core-site.xml, allowing you to
enable it permanently in your {{test/resources/auth-keys.xml}} reference, or
via maven.
I see that the tests are using java 8 language features. That is going to make
backporting to branch-2 in future harder. Is everyone happy there (i.e. willing
to do the effort to downgrade the code if the need arises)?
h3. {{MetadataStoreTestBase}}
* L234: I know it's not in this patch, but I think the path name should be
changed to something else.
h3. {{TestDynamoDBMetadataStore}}
I'll need to spend some time looking/playing with this.
* There's an inevitable risk the native libs aren't around/going to work with
the native OS running the build. What policy is good there? Fail? or downgrade
to skip? It's probably easiest to leave it as it is now (fail) and see what
needs to change as/when failures surface.
* Add {{S3AFS.close()}} call in {{tearDownAfterClass}} just to make sure
threads all get cleaned up.
> S3Guard: Implement DynamoDBMetadataStore.
> -----------------------------------------
>
> Key: HADOOP-13449
> URL: https://issues.apache.org/jira/browse/HADOOP-13449
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
> Reporter: Chris Nauroth
> Assignee: Mingliang Liu
> Attachments: HADOOP-13449-HADOOP-13345.000.patch,
> HADOOP-13449-HADOOP-13345.001.patch
>
>
> Provide an implementation of the metadata store backed by DynamoDB.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
