Resume partial download from S3 on connection drop #46589

DaveCTurner · 2019-09-11T11:09:44Z

Today if the connection to S3 times out or drops after starting to download an
object then the SDK does not attempt to recover or resume the download, causing
the restore of the whole shard to fail and retry. This commit allows
Elasticsearch to detect such a mid-stream failure and to resume the download
from where it failed.

Today if the connection to S3 times out or drops after starting to download an object then the SDK does not attempt to recover or resume the download, causing the restore of the whole shard to fail and retry. This commit allows Elasticsearch to detect such a mid-stream failure and to resume the download from where it failed.

elasticmachine · 2019-09-11T11:09:46Z

Pinging @elastic/es-distributed

DaveCTurner · 2019-09-11T11:13:20Z

The retry behaviour is perhaps not ideal here - we only fail completely after maxRetries consecutive failures to make any progress at all. It also doesn't try and interact with the retry tracking within the SDK. Thus if we're seeing lots of different kinds of failures happening all at the same time we might retry a lot more than maxRetries times.

I'm also not sure about trying to handle the case where a retrying openStream() throws an exception, leaving S3RetryingInputStream#currentStream set to the old (failed and closed) stream. I don't think that's so bad, we shouldn't be using it after such an exception, but your thoughts are welcome.

original-brownbear

LGTM, thanks @DaveCTurner good stuff :)

...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RetryingInputStream.java

original-brownbear · 2019-09-11T12:04:25Z

I'm also not sure about trying to handle the case where a retrying openStream() throws an exception, leaving S3RetryingInputStream#currentStream set to the old (failed and closed) stream. I don't think that's so bad, we shouldn't be using it after such an exception, but your thoughts are welcome.

I think this is fine. We're not using these streams in any tricky-async way anyway so if we run into an Exception it's always inside a try-with-resources I think, so no point in doing extra work here imo.

tlrx

This looks very nice, thanks @DaveCTurner. I left a comment on ensuring that a closed S3 stream will prevent more reads.

tlrx · 2019-09-11T15:41:03Z

...pository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java

+    public void testReadNonexistentBlobThrowsNoSuchFileException() {
+        final BlobContainer blobContainer = createBlobContainer(between(1, 5), null, null, null);
+        final Exception exception = expectThrows(NoSuchFileException.class, () -> blobContainer.readBlob("read_nonexistent_blob"));
+        assertThat(exception.getMessage().toLowerCase(Locale.ROOT),


nit: can fit on the same line

...pository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java

...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RetryingInputStream.java

…ntent

DaveCTurner · 2019-09-12T07:22:24Z

Thanks @tlrx and @original-brownbear. I thought about this overnight and decided it'd be safer to limit the number of retries per blob rather than per read() call, see efb2422.

I also think it might be useful to collect the exceptions that result in retries and add them as suppressed exceptions in case we hit the limit and fail the download. WDYT?

tlrx · 2019-09-12T07:29:01Z

I thought about this overnight and decided it'd be safer to limit the number of retries per blob rather than per read()

I agree

I also think it might be useful to collect the exceptions that result in retries and add them as suppressed exceptions in case we hit the limit and fail the download. WDYT?

I also thought about it when reviewing the PR. I think we can add them as suppressed exceptions but maybe just limit their numbers in case the max retries is set to a large value?

DaveCTurner · 2019-09-12T07:57:05Z

Yes, a bound is a good idea. I added suppressed exceptions in 22f5703.

tlrx

LGTM, thanks David!

tlrx · 2019-09-12T08:00:02Z

...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RetryingInputStream.java

+/**
+ * Wrapper around an S3 object that will retry the {@link GetObjectRequest} if the download fails part-way through, resuming from where
+ * the failure occurred. This should be handled by the SDK but it isn't today. This should be revisited in the future (e.g. before removing
+ * the {@link Version#V_7_0_0} version constant) and removed when the SDK handles retries itself.


+1, otherwise we'll adding retries over retries

original-brownbear · 2019-09-12T08:16:25Z

@DaveCTurner @tlrx

I thought about this overnight and decided it'd be safer to limit the number of retries per blob rather than per read()

I'm not sure sure about this. This has the strange side effect that once again larger blobs are more likely to fail downloading which is exactly what motivated this in the first place.

What do you think is the safety issue here?
I don't think it's mathematically possible (likely enough) to run into something like thousands of retries here? The only case I could see would be some bug in S3 that forces retrying every chunk once (to fix some cache or whatever) exactly once. If it's not once per chunk but some larger number of retries with every chunk failing at a rate x, then you will quickly exceed your retry count on one chunk anyway? (tl;dr ... I don't see what distribution of exceptions could realistically lead to retrying tousands of times per blob but never exceed the limit of retries on a single chunk here).

But if you're still worried after that argument, could we make the retry count proportional to the size of the blob (could make it proportional to the blob's size divided by the upload chunk size we used for it ... it may have changed since the snapshot was taken but it's a good enough estimate IMO)?

DaveCTurner · 2019-09-12T08:37:19Z

What do you think is the safety issue here?

I am thinking of something like a badly-configured security device that drops connections it believes to have downloaded an unreasonable amount of data like, say, 1MB. In the presence of such a device I don't think we should push on through and download each object in 1MB chunks - this would result in a rather surprising bill at the end of the month!

By default each blob has bounded size (1GB) which bounds the probability of it failing outright. I think the default of 3 retries for each chunk is ample.

original-brownbear

I am thinking of something like a badly-configured security device that drops connections it believes to have downloaded an unreasonable amount of data like, say, 1MB. In the presence of such a device I don't think we should push on through and download each object in 1MB chunks - this would result in a rather surprising bill at the end of the month!

Assuming we want to protect people from this LGTM :)

In practice this fixes our issues anyway I think, given the low rate of failure we were observing. Sorry for the noise :)

ywelsch · 2019-09-17T09:11:41Z

...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RetryingInputStream.java

+    }
+
+    @Override
+    public synchronized void reset() {


no need to make this synchronized

Thanks, not sure where that came from. Fixed in 3f8c20e.

ywelsch · 2019-09-17T09:21:35Z

...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RetryingInputStream.java

+
+/**
+ * Wrapper around an S3 object that will retry the {@link GetObjectRequest} if the download fails part-way through, resuming from where
+ * the failure occurred. This should be handled by the SDK but it isn't today. This should be revisited in the future (e.g. before removing


can you link to the corresponding open AWS SDK issue? i.e. aws/aws-sdk-java#856

Yes, done in 3f8c20e. I am not convinced that that's the whole issue, because the problem we were chasing was to do with S3 actively closing the connection rather than a network timeout, but there doesn't seem to be an issue for that.

…ntent

Today if the connection to S3 times out or drops after starting to download an object then the SDK does not attempt to recover or resume the download, causing the restore of the whole shard to fail and retry. This commit allows Elasticsearch to detect such a mid-stream failure and to resume the download from where it failed.

Exactly as elastic#46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS. Closes elastic#52319

* Add Blob Download Retries to GCS Repository Exactly as #46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS. Closes #52319

* Add Blob Download Retries to GCS Repository Exactly as elastic#46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS. Closes elastic#52319

* Add Blob Download Retries to GCS Repository Exactly as #46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS. Closes #52319

DaveCTurner added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.5.0 labels Sep 11, 2019

DaveCTurner requested review from tlrx and original-brownbear September 11, 2019 11:09

Simplify

35fb904

original-brownbear approved these changes Sep 11, 2019

View reviewed changes

DaveCTurner added 3 commits September 11, 2019 13:15

inline noop

ad43a77

Suppress exceptions when closing

ebd4e08

Extract common handler

f97cec8

tlrx requested changes Sep 11, 2019

View reviewed changes

DaveCTurner added 6 commits September 12, 2019 07:52

Merge branch 'master' into 2019-09-11-retry-s3-download-on-partial-co…

0e396dc

…ntent

Add timebomb to ensure we remove this when no longer necessary

9a6f5c6

Count retries per blob not per read

efb2422

Ensure we do not use the stream after close

d9890c6

Make test helpers static and collect at bottom

81f8a35

Unnecessary throws

410fa62

DaveCTurner requested review from tlrx and original-brownbear September 12, 2019 07:22

Include a bounded number of suppressed exceptions on failure

22f5703

tlrx approved these changes Sep 12, 2019

View reviewed changes

original-brownbear approved these changes Sep 12, 2019

View reviewed changes

ywelsch approved these changes Sep 17, 2019

View reviewed changes

DaveCTurner added 2 commits September 17, 2019 11:12

Merge branch 'master' into 2019-09-11-retry-s3-download-on-partial-co…

9604df1

…ntent

Review feedback

3f8c20e

DaveCTurner merged commit d391446 into elastic:master Sep 17, 2019

DaveCTurner deleted the 2019-09-11-retry-s3-download-on-partial-content branch September 17, 2019 12:10

tlrx mentioned this pull request Oct 30, 2019

Add docker-compose based test fixture for Azure #48636

Merged

DaveCTurner mentioned this pull request Feb 13, 2020

GCS SDK does not resume downloads that fail part-way through #52319

Closed

original-brownbear mentioned this pull request Feb 18, 2020

Add Blob Download Retries to GCS Repository #52479

Merged

original-brownbear mentioned this pull request Feb 19, 2020

Add Blob Download Retries to GCS Repository (#52479) #52521

Merged

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resume partial download from S3 on connection drop #46589

Resume partial download from S3 on connection drop #46589

DaveCTurner commented Sep 11, 2019

elasticmachine commented Sep 11, 2019

DaveCTurner commented Sep 11, 2019

original-brownbear left a comment

original-brownbear commented Sep 11, 2019

tlrx left a comment

tlrx Sep 11, 2019

DaveCTurner commented Sep 12, 2019

tlrx commented Sep 12, 2019

DaveCTurner commented Sep 12, 2019

tlrx left a comment

tlrx Sep 12, 2019

original-brownbear commented Sep 12, 2019 •

edited

Loading

DaveCTurner commented Sep 12, 2019

original-brownbear left a comment

ywelsch Sep 17, 2019

DaveCTurner Sep 17, 2019

ywelsch Sep 17, 2019

DaveCTurner Sep 17, 2019

Resume partial download from S3 on connection drop #46589

Resume partial download from S3 on connection drop #46589

Conversation

DaveCTurner commented Sep 11, 2019

elasticmachine commented Sep 11, 2019

DaveCTurner commented Sep 11, 2019

original-brownbear left a comment

Choose a reason for hiding this comment

original-brownbear commented Sep 11, 2019

tlrx left a comment

Choose a reason for hiding this comment

tlrx Sep 11, 2019

Choose a reason for hiding this comment

DaveCTurner commented Sep 12, 2019

tlrx commented Sep 12, 2019

DaveCTurner commented Sep 12, 2019

tlrx left a comment

Choose a reason for hiding this comment

tlrx Sep 12, 2019

Choose a reason for hiding this comment

original-brownbear commented Sep 12, 2019 • edited Loading

DaveCTurner commented Sep 12, 2019

original-brownbear left a comment

Choose a reason for hiding this comment

ywelsch Sep 17, 2019

Choose a reason for hiding this comment

DaveCTurner Sep 17, 2019

Choose a reason for hiding this comment

ywelsch Sep 17, 2019

Choose a reason for hiding this comment

DaveCTurner Sep 17, 2019

Choose a reason for hiding this comment

original-brownbear commented Sep 12, 2019 •

edited

Loading