Retry on 5xx, error on 4xx #465
Conversation
| log::error!("server returned error 500 status: {}", status); | ||
| Box::new(response.validate(StatusCode::OK).await.unwrap_err()) | ||
| } | ||
| Err(error) => { |
There was a problem hiding this comment.
The C# SDK retries IOExceptions so we should retry here too (assuming we can discriminate the error to errors raised by the http policy).
There was a problem hiding this comment.
Are you saying we should only retry IO related errors? Right now any error is retried.
There was a problem hiding this comment.
Yes, sorry, that was what I meant... 😔
There was a problem hiding this comment.
Ok I can tackle that in another PR. This might require that we change the pipeline to not work with Box<dyn Error> but with azure_core::Error instead.
|
@MindFlavor @heaths I've made a change to this PR where we now treat any 2xx (and for that matter any 3xx or 1xx if those should ever happen to be returned) as successes. We are now validating in the retry policy that we get a successful response and if not either retry (if it's a response code we hard code as being a retry status code) or immediately return an error. |
| } | ||
| Err(error) => { | ||
| log::debug!( |
There was a problem hiding this comment.
Line 67 uses log::error for the same message. Should these be consistent?
There was a problem hiding this comment.
This will be retried (and hopefully succeed on retry) where as line 67 will be returned to the user as an error. Therefore, line 67 represents an error that the user will definitely handle while hopefully this line is just a transient state.
An attempt to fix #464
The policy now works as following:
Ok(response)Err(HttpError::UnexpectedStatusCode)Questions: