-
Notifications
You must be signed in to change notification settings - Fork 770
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.
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
[object_store] Retry S3 requests with 200 response with "Error" in body #6508
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thank you
use crate::RetryConfig; | ||
use hyper::header::LOCATION; | ||
use hyper::Response; | ||
use reqwest::{Client, Method, StatusCode}; | ||
use std::time::Duration; | ||
|
||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big fan of the test coverage 👍
@@ -33,6 +33,12 @@ pub enum Error { | |||
#[snafu(display("Received redirect without LOCATION, this normally indicates an incorrectly configured region"))] | |||
BareRedirect, | |||
|
|||
#[snafu(display("Server error, body contains Error, with status {status}: {}", body.as_deref().unwrap_or("No Body")))] | |||
Server { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've confirmed this isn't public and therefore a breaking change
2cb16dc
to
c2d041f
Compare
@tustvold thanks for taking a look, I rebased & fixed lint. Should be good to go now :) |
Which issue does this PR close?
Closes #5938
Rationale for this change
Some S3 requests like CopyObject and CompleteMultipartUpload may return a 200 response but have "InternalError" or "SlowDown" in the response body. These should be retried as if they were a 5xx error.
Relevant AWS documentation
e.g. we received this as a body of a 200 response
<?xml version=\"1.0\" encoding=\"UTF-8\"?><Error><Code>SlowDown</Code><Message>Please reduce your request rate.</Message><RequestId>123</RequestId><HostId>456</HostId></Error>
What changes are included in this PR?
This PR adds the
retry_body_error
attribute forRequest
/RetryableRequest
, which isfalse
by default. It is set to true for S3copy
andcomplete_multipart
requests.If
retry_body_error
istrue
, and if the body contains an error, we will retry it as if we got a 5xx resopnse.Unfortunately AWS doesn't seem to provide us with the exact format of the error message, so we can only look for
InternalError
orSlowDown
in the response body and not much else.Are there any user-facing changes?
No