You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "Aaron Fabbri (JIRA)" <ji...@apache.org> on 2018/08/01 04:57:00 UTC
[jira] [Commented] (HADOOP-14154) Persist isAuthoritative bit in
DynamoDBMetaStore (authoritative mode support)
[ https://issues.apache.org/jira/browse/HADOOP-14154?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16564734#comment-16564734 ]
Aaron Fabbri commented on HADOOP-14154:
---------------------------------------
Ok.. I actually looked at the right patch today ;). This looks pretty good overall. I'm excited to see this coming together. Inline comments:
{noformat}
@@ -518,12 +520,19 @@ public DirListingMetadata listChildren(final Path path) throws IOException {
PathMetadata meta = itemToPathMetadata(item, username);
metas.add(meta);
}
+
+ PathMetadata dirPathMeta = get(path);
+ boolean isAuthoritative = false;
+ if(dirPathMeta != null) {
+ isAuthoritative = dirPathMeta.isAuthoritativeDir();
+ }
{noformat}
We should try to avoid the change to the public API (PathMetadata) I would create a new class (POJO) DDBPathMetadata which includes all the fields in PathMetadata plus the isAuthoritative flag. This way we don't have to change the public MetadataStore / PathMetadata interface.
DDBPathMetadata can have a constructor which takes a PathMetadata + isAuth flag
for when you are writing to dynamo. It can also have a method that creates
(transforms to) a PathMetadata for when you are reading. More ideas below...
{noformat}
if (deletionBatch.size() == S3GUARD_DDB_BATCH_WRITE_REQUEST_LIMIT) {
Thread.sleep(delay);
processBatchWriteRequest(pathToKey(deletionBatch), null);
+
+ // set authoritative false for each pruned dir listing
+ removeAuthoritativeDirFlag(parentPathSet);
+ parentPathSet.clear();
+
deletionBatch.clear();
}
}
if (deletionBatch.size() > 0) {
Thread.sleep(delay);
processBatchWriteRequest(pathToKey(deletionBatch), null);
+
+ // set authoritative false for each pruned dir listing
+ removeAuthoritativeDirFlag(parentPathSet);
+ parentPathSet.clear();
{noformat}
looks good. You should get test coverage from existing MetadataStoreTestBase#testPruneUnsetsAuthoritative case.
{noformat}
+ private void removeAuthoritativeDirFlag(Set<Path> pathSet) {
+ pathSet.forEach(pp -> {
+ removeAuthoritativeDirFlag(pp);
+ });
+ }
+
+ private void removeAuthoritativeDirFlag(Path path){
+ try {
+ DirListingMetadata dlm = listChildren(path);
{noformat}
looks correct but could be optimized. We are getting the whole listing just
to clear the flag. I would probably have internal (private) getters/setters for
loading / storing a single DDBPathMetadata from dynamo. Then you can just deal
with the single row, instead of getting/putting the whole listing to set a flag.
{noformat}
+ if(dlm == null){
{noformat}
nit: spacing (space after if and close paren.)
{noformat}
@@ -1157,7 +1206,7 @@ private static void checkPathMetadata(PathMetadata meta) {
map.put(WRITE_CAPACITY, throughput.getWriteCapacityUnits().toString());
map.put(TABLE, desc.toString());
map.put(MetadataStoreCapabilities.PERSISTS_AUTHORITATIVE_BIT,
- Boolean.toString(false));
+ Boolean.toString(true));
{noformat}
Woo hoo!
{noformat}
- public PathMetadata(FileStatus fileStatus, Tristate isEmptyDir, boolean
- isDeleted) {
+ public PathMetadata(FileStatus fileStatus, Tristate isEmptyDir,
+ boolean isDeleted) {
+ this(fileStatus, isEmptyDir, isDeleted, false);
+ }
+
+ public PathMetadata(FileStatus fileStatus, Tristate isEmptyDir,
+ boolean isDeleted, boolean isAuthoritativeDir) {
{noformat}
IMO, this change should go away, instead happening on the dynamo-specific
PathMetadata POJO (e.g. DDBPathMetadata)
{noformat}
- return new PathMetadata(fileStatus, Tristate.UNKNOWN, isDeleted);
+ return new PathMetadata(fileStatus, Tristate.UNKNOWN, isDeleted,
+ isAuthoritativeDir);
}
{noformat}
here I'm thinking you could have a new private function, e.g.
private DDBPathMetadata itemToDDBPathMetadata()
then if you need an actual PathMetadata you can just
DDBPathMetadata dpm = itemToDDBPathMetadata(..);
...
return dpm.toPathMetadata()
{noformat}
+ // set parent dir as authoritative
+ if(!allowMissing()) {
{noformat}
nit: spacing
{noformat}
+ @Test
+ public void testPrunePreservesAuthoritative() throws Exception {
<snip>
+ // assert that parent dir is still authoritative (no removed elements
+ // during prune)
+ assertFalse(rootDirMd.isAuthoritative());
+ assertFalse(grandParentDirMd.isAuthoritative());
+ assertTrue(parentDirMd.isAuthoritative());
+ }
+ }
+
{noformat}
Nice, thank you for adding more test coverage to the base test class.
> Persist isAuthoritative bit in DynamoDBMetaStore (authoritative mode support)
> -----------------------------------------------------------------------------
>
> Key: HADOOP-14154
> URL: https://issues.apache.org/jira/browse/HADOOP-14154
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
> Affects Versions: 3.0.0-beta1
> Reporter: Rajesh Balamohan
> Assignee: Gabor Bota
> Priority: Minor
> Attachments: HADOOP-14154-HADOOP-13345.001.patch, HADOOP-14154-HADOOP-13345.002.patch, HADOOP-14154-spec-001.pdf, HADOOP-14154-spec-002.pdf, HADOOP-14154.001.patch
>
>
> Add support for "authoritative mode" for DynamoDBMetadataStore.
> The missing feature is to persist the bit set in {{DirListingMetadata.isAuthoritative}}.
> This topic has been super confusing for folks so I will also file a documentation Jira to explain the design better.
> We may want to also rename the DirListingMetadata.isAuthoritative field to .isFullListing to eliminate the multiple uses and meanings of the word "authoritative".
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org