Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Investigate removing retries from s3 blob store #22845

Closed
rjernst opened this issue Jan 27, 2017 · 3 comments
Closed

Investigate removing retries from s3 blob store #22845

rjernst opened this issue Jan 27, 2017 · 3 comments
Assignees
@abeyad
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs help wanted adoptme

Comments

@rjernst
Copy link
Member

rjernst commented Jan 27, 2017 •
edited
Loading

The s3 client has a maxRetries setting, which is uses internally for retrying throttled requests. The setting is also read by the s3 blob store, and a while{try/catch} is used along with retries (but without backoff). It appears this outer retry mechanism may have existed for throttling before the sdk client did that, or also for eventual consistency. For the latter, this should no longer be a problem, as currently s3 provides read-after-write consistency for new PUTs (and afaik we should not be overwriting blobs). This issue is to discuss removing the retries from the blob store (but not the client).
@abeyad
Copy link

abeyad commented Jan 28, 2017

+1

(and afaik we should not be overwriting blobs)

Correct

@dadoonet
Copy link
Member

I agree. I'd just like to double-check with @tlrx as he introduced this feature with elastic/elasticsearch-cloud-aws@cbe82d2

@tlrx
Copy link
Member

tlrx commented Jan 31, 2017

+1 too, I think we can remove the discuss label.

@tlrx tlrx added help wanted adoptme and removed discuss labels Jan 31, 2017
@lewischen1123 lewischen1123 mentioned this issue Mar 12, 2017
Closed
abeyad pushed a commit to abeyad/elasticsearch that referenced this issue Apr 6, 2017
Removes the retry mechanism from the S3 blob store
88b2028 
Currently, both the Amazon S3 client provides a retry mechanism, and the
S3 blob store also attempts retries for failed read/write requests.
Both retry mechanisms are controlled by the
`repositories.s3.max_retries` setting.  However, the S3 blob store retry
mechanism is unnecessary because the Amazon S3 client provided by the
Amazon SDK already handles retries (with exponential backoff) based on
the provided max retry configuration setting (defaults to 3) as long as
the request is retryable.  Hence, this commit removes the unneeded retry
logic in the S3 blob store and the S3OutputStream.

Closes elastic#22845
@abeyad abeyad self-assigned this Apr 6, 2017
@abeyad abeyad mentioned this issue Apr 6, 2017
Merged
abeyad pushed a commit that referenced this issue Apr 6, 2017
Removes the retry mechanism from the S3 blob store (#23952)
4f12174 
Currently, both the Amazon S3 client provides a retry mechanism, and the
S3 blob store also attempts retries for failed read/write requests.
Both retry mechanisms are controlled by the
`repositories.s3.max_retries` setting.  However, the S3 blob store retry
mechanism is unnecessary because the Amazon S3 client provided by the
Amazon SDK already handles retries (with exponential backoff) based on
the provided max retry configuration setting (defaults to 3) as long as
the request is retryable.  Hence, this commit removes the unneeded retry
logic in the S3 blob store and the S3OutputStream.

Closes #22845
abeyad pushed a commit that referenced this issue Apr 7, 2017
Removes the retry mechanism from the S3 blob store (#23952)
7eb5222 
Currently, both the Amazon S3 client provides a retry mechanism, and the
S3 blob store also attempts retries for failed read/write requests.
Both retry mechanisms are controlled by the
`repositories.s3.max_retries` setting.  However, the S3 blob store retry
mechanism is unnecessary because the Amazon S3 client provided by the
Amazon SDK already handles retries (with exponential backoff) based on
the provided max retry configuration setting (defaults to 3) as long as
the request is retryable.  Hence, this commit removes the unneeded retry
logic in the S3 blob store and the S3OutputStream.

Closes #22845
@clintongormley clintongormley added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository S3 labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@abeyad abeyad

Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs help wanted adoptme
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@clintongormley @dadoonet @rjernst @tlrx @abeyad