Cleanup object_store::retry client error handling
#4915
Conversation
| } | ||
|
|
||
| /// Returns the error body if any | ||
| pub fn body(&self) -> Option<&str> { |
There was a problem hiding this comment.
Being able to provide this method is what originally motivated this PR
alamb
changed the title
object_store client error handling
There was a problem hiding this comment.
Thank you @tustvold
I found the naming to be a little confusing, as I commented.
I verified that this type does not appear to be public: https://docs.rs/object_store/0.7.1/object_store/?search=retry
Since it isn't a public API I think it could be merged as is (though I do think we should change the names / display of the errors)
| #[snafu(display("Received redirect without LOCATION, this normally indicates an incorrectly configured region"))] | ||
| BareRedirect, | ||
|
|
||
| #[snafu(display("Client error with status {status}: {}", body.as_deref().unwrap_or("No Body")))] |
There was a problem hiding this comment.
Isn't this techically a ServerError as is a value returned from the server (rather than some error trying to make the connecton?)
There was a problem hiding this comment.
It's a client error in the sense, the client did something wrong, which the server then complained about.
As per https://docs.rs/http/latest/http/status/struct.StatusCode.html#method.is_client_error
Or https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses
object_store/src/client/retry.rs
Outdated
| body: Option<String>, | ||
| }, | ||
|
|
||
| #[snafu(display("Response error after {retries} retries: {source}"))] |
There was a problem hiding this comment.
Isn't this kind of error normally called a ClientError or ConnectionError as it denotes a problem communicating with the server?
There was a problem hiding this comment.
I removed "Response" to avoid confusion
alamb
changed the title
object_store client error handlingobject_store::retry client error handling
| @@ -39,8 +39,8 @@ pub enum Error { | |||
| body: Option<String>, | |||
| }, | |||
|
|
|||
| #[snafu(display("Response error after {retries} retries: {source}"))] | |||
| Response { | |||
| #[snafu(display("Error after {retries} retries: {source}"))] | |||
Which issue does this PR close?
Relates to #4880
Rationale for this change
The existing logic was hard to follow, with various different error variants overlayed onto the same structure. This makes these variants explicit, making the code easier to follow, and better able to support acting on error payloads.
What changes are included in this PR?
Are there any user-facing changes?
No, this error type is crate-private