[
https://issues.apache.org/jira/browse/HADOOP-13786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16006867#comment-16006867
]
Steve Loughran commented on HADOOP-13786:
-----------------------------------------
h3. son ser/deser
good point: done. The registry one is now a subclass, with special handling of
the load phase where it can scan for a marker string before trying to parse the
JSON. That's needed there to fail fast if the entry at the ZK node isn't the
expected JSON type, or indeed, not JSON at all.
bq. In the comment, did you mean "FileOutputCommitter instances"?
yes. fixed
0 byte files
in {{ITestS3ACommitOperations}} {{.testCreateAbortEmptyFile}}
bq. Currently someone can config fs.s3a.multipart.purge.age to 0.
yeah. To be honest, its pretty dangerous to instantiate an fs client with this
set to a low value on any shared bucket. because of the risk of killing ongoing
writes...it's why we've backed off from doing thsi in tests altogether.
the commit protocol merely increases the window of risk, no matter which
committer is used (as for both committers, it holds from task commit onwards,
so stays pending while all other commits take place)
+1 for docs.
h3. {{public getAmazonS3Client()}}
that was exported for the original netflix import; now I've finished moving to
the {{WriteOperationHelper}} methods it can be package scoped again. Done
h3. Should we annotate the interface stability or audience here?
The whole class is tagged private/evolving; javadocs say "If cast to {@code
S3AFileSystem}, extra methods and features may be accessed. Consider those
private and unstable."
Same for the other scope queries. We've tagged the API as unstable. What's
important though is to limit the damage which someone using the APIs can do.
package-scoping {{getAmazonS3Client()}} matters here because it's free access
to the internals, bypassing s3guard & our metrics and monitoring. The others
seem less critical.
h3. Making {{WriteOperationHelper}} standalone
I'd originally created it to isolate the ops needed for the block output
stream: with the constructor private it simply wasn't possible for outside
callers to get at the WriteOperations. The committer expands the ops to cover
everything needed for committing, 20+ methods, and an external interface. So it
probably is time to do it, with a package scoped constructor and uprating
private S3A methods it was calling to package scoped too.
We should also go through all exported S3AFS methods and restrict access just
to stay in control.
*I can do all of these in this patch, which will increase patch size, but avoid
me facing lots of merge/rebase pain*
h3. retry logic
yes, ignoring it right now. But we should have retries on all the ops. I think
we should start with a core design for retrying in our existing code, then
reuse it here. Especially if we do something clever with lambda expressions. On
that topic. I'm thinking of something like:
{code}
MPU = execute(new RetryAllButAuth(), () -> initiateMultipartUpload())
{code}
Added some more detail on that to HADOOP-14303. Something like that should be
good for testability too.
I think that could go into the s3guard branch, where I'd pick it up and use
here, or we merge this in and then adopt.
h3. revert
bq. Does the directory need to exist after revert? Can we just say it doesn't?
OK :)
h3. deleteWithWarning
Fixed javadocs, aslo changed catch to IOEs only. Did the same for
{{deleteQuietly()}}.
h3. typo in {{Abstract3GuardCommitterFactory}}
This is why reviews are so good. I'd missed that completely.
h3. "Configuration options:" javadoc
cut this out; we can use MD files for covering configs
h3. TODOs.
yes, they came in with the original code. Big ones: some about tests, plus we
need to consider whether the the temp dir in HDFS should be configurable to
something other than /tmp/$USER. Also I commented out some other bits.
Anyway, thx for the review, will submit a patch with them in by the end of the
weke.
meanwhile
* spark master now has the module needed for testing downstream
* One big outstanding issue which doesn't have todo is actually having this
work propery with s3guard. So far the code just lives alongside, but I need to
make sure that the commit operations make things visible in s3guard, and that
all the magic stuff gets created with the consistency we need
> Add S3Guard committer for zero-rename commits to consistent S3 endpoints
> ------------------------------------------------------------------------
>
> Key: HADOOP-13786
> URL: https://issues.apache.org/jira/browse/HADOOP-13786
> Project: Hadoop Common
> Issue Type: New Feature
> Components: fs/s3
> Affects Versions: HADOOP-13345
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Attachments: HADOOP-13786-HADOOP-13345-001.patch,
> HADOOP-13786-HADOOP-13345-002.patch, HADOOP-13786-HADOOP-13345-003.patch,
> HADOOP-13786-HADOOP-13345-004.patch, HADOOP-13786-HADOOP-13345-005.patch,
> HADOOP-13786-HADOOP-13345-006.patch, HADOOP-13786-HADOOP-13345-006.patch,
> HADOOP-13786-HADOOP-13345-007.patch, HADOOP-13786-HADOOP-13345-009.patch,
> HADOOP-13786-HADOOP-13345-010.patch, HADOOP-13786-HADOOP-13345-011.patch,
> HADOOP-13786-HADOOP-13345-012.patch, HADOOP-13786-HADOOP-13345-013.patch,
> HADOOP-13786-HADOOP-13345-015.patch, HADOOP-13786-HADOOP-13345-016.patch,
> HADOOP-13786-HADOOP-13345-017.patch, HADOOP-13786-HADOOP-13345-018.patch,
> HADOOP-13786-HADOOP-13345-019.patch, HADOOP-13786-HADOOP-13345-020.patch,
> HADOOP-13786-HADOOP-13345-021.patch, HADOOP-13786-HADOOP-13345-022.patch,
> HADOOP-13786-HADOOP-13345-023.patch, HADOOP-13786-HADOOP-13345-024.patch,
> HADOOP-13786-HADOOP-13345-025.patch, objectstore.pdf, s3committer-master.zip
>
>
> A goal of this code is "support O(1) commits to S3 repositories in the
> presence of failures". Implement it, including whatever is needed to
> demonstrate the correctness of the algorithm. (that is, assuming that s3guard
> provides a consistent view of the presence/absence of blobs, show that we can
> commit directly).
> I consider ourselves free to expose the blobstore-ness of the s3 output
> streams (ie. not visible until the close()), if we need to use that to allow
> us to abort commit operations.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
