Generic retry wrapper to consolidate and streamline retry logic. #397
Conversation
cas_client/src/remote_client.rs
Outdated
| @@ -34,10 +34,10 @@ use utils::singleflight::Group; | |||
| use crate::download_utils::*; | |||
| use crate::error::{CasClientError, Result}; | |||
| use crate::http_client::{Api, ResponseErrorLogger, RetryConfig}; | |||
| use crate::interface::*; | |||
| use crate::interface::{ShardDedupProber, *}; | |||
There was a problem hiding this comment.
this is an odd import? is ShardDedupProber not part of "*"?
| if !response.status().is_success() { | ||
| let response = RetryWrapper::new(api_tag) | ||
| .with_429_no_retry() | ||
| .log_errors_as_info() |
There was a problem hiding this comment.
is this an override over the default log level? what is the default/is there a default?
There was a problem hiding this comment.
The default log level is to log fatal errors as errors, i.e. the connection couldn't be completed either due to non-transient errors or running out of retries. The current logging -- prior to this PR -- is done with a log_error after the send, which isn't as consistent as it doesn't include a lot of the context, just the error.
| if self.log_errors_as_info || log_as_info { | ||
| info!("{msg}"); | ||
| } else { | ||
| error!("{msg}"); | ||
| } |
There was a problem hiding this comment.
instead can we hold a tracing::Level, default it to tracing::Level::ERROR and emit event! here? if you want to override it by passing in log_as_info, then pass in an Option<tracing::Level>?
There was a problem hiding this comment.
I would rather have formally more control, even if we use the same settings everywhere. Fine if you really prefer to not expose that option.
There was a problem hiding this comment.
What's the need for that extra control here? I'm wondering what scenarios would benefit from it. Currently, each of the calls does a log_error call except the global dedup query, which swallows any errors; this reflects that behavior.
Also, this logs retries as info, and the error is for fatal errors.
| ReqFn: Fn() -> ReqFut + Send + 'static, | ||
| ReqFut: std::future::Future<Output = Result<Response, reqwest_middleware::Error>> + 'static, | ||
| ProcFn: Fn(Response) -> ProcFut + Send + 'static, | ||
| ProcFut: std::future::Future<Output = Result<T, RetryableReqwestError>> + 'static, |
There was a problem hiding this comment.
Can you add comments on these types, they are quite long/complex-looking, they aren't too complex, but it's a minor chore to read. similar in run_and_extract_json
This PR consolidates the retry logic for the http connections around a single utility, RetryWrapper, and integrates it more cleanly with the logging and parsing logic. The goal is to replace the retry middleware with this, which doesn't work with streaming connections.
This PR introduces this and replaces the simpler connection paths in RemoteClient with this wrapper but leaves the previous logic intact. The next step is to fully switch the remaining cases over to this wrapper to clean up the code.