HADOOP-19221. S3A: Unable to recover from failure of multipart block upload attempt #6938
Conversation
steveloughran
force-pushed
the
s3/HADOOP-19221-multipart-put-failures
branch
from
bf5d5ec
to
a93afe6
Compare
steveloughran
force-pushed
the
s3/HADOOP-19221-multipart-put-failures
branch
from
a93afe6
to
9b942c7
Compare
|
I believe I have a way to test this by injecting 500 exceptions with a custom execution interceptor added to the audit chain |
steveloughran
force-pushed
the
s3/HADOOP-19221-multipart-put-failures
branch
from
76deb75
to
65fd797
Compare
|
tested s3 london with prefetch failures, timing related (12 too big...) |
steveloughran
force-pushed
the
s3/HADOOP-19221-multipart-put-failures
branch
2 times, most recently
from
2790d56
to
b95ee7c
Compare
|
s3 london with: -Dparallel-tests -DtestsThreadCount=8 -Dscale This is ready to be reviewed. @mukund-thakur, @HarshitGupta11 and @shameersss1 could you all look at this? |
steveloughran
force-pushed
the
s3/HADOOP-19221-multipart-put-failures
branch
from
b187942
to
1fb04e9
Compare
| */ | ||
| public final class ByteBufferInputStream extends InputStream { | ||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(DataBlocks.class); |
There was a problem hiding this comment.
Shoudn't this be ByteBufferInputStream.class ?
| } catch (ExecutionException ee) { | ||
| //there is no way of recovering so abort | ||
| //cancel all partUploads |
There was a problem hiding this comment.
Aren't we cancelling all the uploads here ?
There was a problem hiding this comment.
looking at this. may need some more review to be confident we are doing abort here properly
There was a problem hiding this comment.
..done a lot more work into aborting.
| */ | ||
| public class AWSStatus500Exception extends AWSServiceIOException { | ||
| public AWSStatus500Exception(String operation, | ||
| AwsServiceException cause) { | ||
| super(operation, cause); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean retryable() { |
There was a problem hiding this comment.
Will this make all 500 retriable ? I mean if we S3 throws exception like 500 S3 Server Internal error. Do we need to retry from S3A client as well ?
There was a problem hiding this comment.
see the latest change..I've made it an option
| // there is specific handling for some 5XX codes (501, 503); | ||
| // this is for everything else | ||
| policyMap.put(AWSStatus500Exception.class, fail); | ||
| policyMap.put(AWSStatus500Exception.class, retryAwsClientExceptions); |
There was a problem hiding this comment.
Do we need to selectively retry 500 exception? Say only when the cause is "Your socket connection......."
There was a problem hiding this comment.
see the full comment below. along with that I really don't like looking in error strings, way too brittle for production code. Even in tests I like to share the text across production and test classes as constants.
(yes, I know about org.apache.hadoop.fs.s3a.impl.ErrorTranslation ....doesn't mean I like it)
| @Override | ||
| protected ByteBufferInputStream createNewStream() { | ||
| // set the buffer up from reading from the beginning | ||
| blockBuffer.limit(initialPosition); |
There was a problem hiding this comment.
Wondering why setting the limit is important?
There was a problem hiding this comment.
we want to start reading from the initial position every time the stream is opened.
| Retrying _should_ make it go away. | ||
|
|
||
| The 500 error is considered retryable by the AWS SDK, which will have already | ||
| tried it `fs.s3a.attempts.maximum` times before reaching the S3A client -which |
|
@shameersss1 We have massively cut back on the number of retries which take place in the V2 SDK compared to V1; even though we have discussed in the past turning it off completely and handling it all ourselves. However, that would break things the transfer manager does in separate threads. The thing is, I do not know how often we see 500 errors against AWS S3 stores (rather than third party ones with unrecoverable issues) -and now we have seen them I don't know what the right policy should be. The only documentation on what to do seems more focused on 503s, and doesn't provide any hints about why a 500 could happen or what to do other than "keep trying maybe it'll go away": https://repost.aws/knowledge-center/http-5xx-errors-s3 . I do suspect it is very rare -otherwise the AWS team might have noticed their lack of resilience here, and we would've found it during our own testing. Any 500 error at any point other than multipart uploads probably gets recovered from nicely so that could've been a background noise of these which we have never noticed before. s3a FS stats will now track these, which may be informative. I don't want to introduce another configuration switch if possible because that at more to documentation testing maintenance et cetera. One thing I was considering is should we treat this exactly the same as a throttling exception which has its own configuration settings for retries? Anyway, if you could talk to your colleagues and make some suggestions based on real knowledge of what can happen that would be really nice. Note that we are treating 500 as idempotent, the way we do with all the other failures even though from a distributed computing purism perspective it is not in fact true. Not looked at the other comments yet; will do later. Based on a code walk-through with Mukud, Harshit and Saikat, I've realised we should make absolutely sure that the stream providing a subset of file fails immediately if the read() goes past the allocated space. With tests, obviously. |
|
@shameersss1 here is what I propose: add a boolean config option fs.s3a.retry.all.http.errors retry on all "maybe unrecoverable" http errors; default is false. I did think about a comma separated list "500, 4xx, 510" but decided it was overcomplex |
based on @shameersss1 comments I've reviewed S3ABlockOutputStream aborting
Now unsure about what is the best policy to avoid ever leaking buffers
We could do this now we have our own content provider: raise a nonrecoverable AwsClientException... |
steveloughran
force-pushed
the
s3/HADOOP-19221-multipart-put-failures
branch
from
0d47447
to
b793661
Compare
…upload attempt Adds custom set of content providers in UploadContentProviders which * restart on failures * do not copy buffers/byte buffers into new private byte arrays, so avoid exacerbating memory problems. org.apache.hadoop.fs.store.ByteBufferInputStream has been pulled out of org.apache.hadoop.fs.store.DataBlocks to assist. ITestUploadRecovery triggers the failure mode through fault injection new ContentProviders used as a appropriate in S3ABlockOutputStream and CommitOperations IOStatistics * new IOStatistics for select http error codes * s3a auditor updates filesystem IOStatistics when these happen Consider AWSStatus500Exception recoverable * AWSStatus500Exception is now an optionally recoverable * section in troubleshooting on it * and one on 503 The core work here is to ensure that 1. cancel of PUT/POST of data outcome "correctly" in close() 2. blocks are always closed, so as to ensure files are deleted. S3ABlockOutputStream has major changes here Key changes * FutureIO javadocs cover CancellationException and new method cancelAllFuturesAndAwaitCompletion() to trigger a cancel then await (briefly) for threads to be cancelled. * UploadContentProviders and BlockUploadData take a Supplier<Boolean> predicate to probe for a stream still being open; this is wired up so if a block is closed, newStream() raises IllegalStateException. * ProgressListenerEvent enum expanded to track many more events * S3ABlockOutputStream reports as a appropriate * CountingProgressListener pulled from AbstractSTestS3AHugeFiles; expanded to track new progress events * New test suite ITestS3ABlockOutputStreamInterruption to abort streams
…ream abort Make sure that when abort is rejected by s3 this is caught and increments the statistics rather than escalated to a failure. Change-Id: I66e65219086cb2d90d2a65dcb4000703f66cf15d
Final bit of work on what was already a significant enough change: move all the s3client upload commands from S3AFS into S3AStore, as part of the incremental migration and lining up for a move to AsyncClient everywhere. * WriteOperationHelper still takes an S3AFS instance, as most of the methods it calls are still implemented there. * Some changes to the mock testing as required + review of the retry and translation attributes of the operations, pulling up through interface and verification that all looks good. + review handling of CancellationExceptions Change-Id: Icb3a8286b6e2a423469f534b244dd90e383fd662
Change-Id: Ie4955bc0ef887dbd8686f5ae6681334055e22429
steveloughran
force-pushed
the
s3/HADOOP-19221-multipart-put-failures
branch
from
8be4c96
to
0e1207a
Compare
|
@shameersss1 can you review this again? |
|
💔 -1 overall
This message was automatically generated. |
Change-Id: I956c3eeded349291cc13feaf6eac68a1a368949a
|
🎊 +1 overall
This message was automatically generated. |
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Unstable | ||
| public final class FutureIO { | ||
|
|
||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(TaskPool.class); |
There was a problem hiding this comment.
shouldn't this be FutureIO.class ?
HADOOP-19221
Adds custom set of content providers in UploadContentProviders which
org.apache.hadoop.fs.store.ByteBufferInputStream has been pulled out of org.apache.hadoop.fs.store.DataBlocks to assist.
Description of PR
How was this patch tested?
Fault injection through AWS SDK extension point which changes the status from 200 to 400 after the targeted operation completes. This puts the SDK into retry/recovery mode.
Some new unit tests
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?