Add basic "batteries-included" retry::Policys.
#414
Conversation
These are lifted out of the test and example code. Two policies are provided: * `RetryLimit`, with a bounded number of retry attempts; * `RetryError`, with unbounded retry attempts. Both policies require `Req: Clone` as it's not possible to write a generic retry policy without being able to clone requests. The docs should be updated with a little blurb that explains when they should not be used. Also, the `Policy` docs had an `Attempts` example corresponding to `RetryLimit` (formerly `Limit` in the tests code); I removed it because `RetryLimit` exists and can be view-sourced.
| impl<Req: Clone, Res, E> Policy<Req, Res, E> for RetryLimit { | ||
| type Future = future::Ready<Self>; | ||
| fn retry(&self, _: &Req, result: Result<&Res, &E>) -> Option<Self::Future> { | ||
| if result.is_err() { |
There was a problem hiding this comment.
I wonder if there is a way to traitize this? For example, http retries you may want to retry 500s but not 400s. In this case, if I were to apply this to hyper I would only be able to retry system errors not http errors. What do you think about providing some method to allow users to specify what they want to retry?
There was a problem hiding this comment.
One possibility would be to construct the RetryLimit with a closure F: FnMut(&E) -> bool; then callers who wanted to retry all errors could construct it as, e.g.,
let policy = RetryLimit::new(2, |_| true);
and callers who want to filter retries can stick whatever logic they want in the closure. (Does that seem like the right bound for the closure?)
There was a problem hiding this comment.
Though since this only returns an error that wouldn't support retrying on http::Responses?
I think we could use some trait and a TraitFn adapter for a closure, what do you think?
There was a problem hiding this comment.
Hmm, I'm not sure what kind of trait you're thinking of. I think I'm missing something: if the user-supplied logic has access to the whole response and error, how would that be different from just implementing Policy::retry directly?
There was a problem hiding this comment.
This is closer to the related vector retrylogic thing I sent on discord, but the idea is that as a user I can just provide what i'd like to retry on instead of having to implement a retry policy myself.
There was a problem hiding this comment.
I wouldn't copy this 1-1 but https://github.com/timberio/vector/blob/master/src/sinks/util/retries.rs#L25
There was a problem hiding this comment.
I wonder if there is a way to traitize this? For example, http retries you may want to retry 500s but not 400s.
For example, finagle uses a ResponseClassifier in a similar way.
This should be removed when tower-rs/tower#414 lands but is good enough for our purposes for now.
This should be removed when tower-rs/tower#414 lands but is good enough for our purposes for now.
jonhoo
added
A-retry
|
@hdevalence any interest in wrapping this up, or should we just close the PR? |
|
I'd be happy to wrap this up, but I don't really know what the "good" design would be -- the current PR is just what worked for my needs. But, if there was clarity about what changes would be good, I'd be happy to make them, and I think it would be convenient if something like this was included in Tower. |
I wonder if @LucioFranco has any ideas about this. |
|
@rcoh maybe you can chime in here a bit? |
|
I don't necessarily want to impose our retry needs on others, but as a data point here are things we need:
Our retry policy behavior (around number of retries, backoff length, etc.) is far too complex to be well served by something shared, unfortunately, so I don't think we have much need for batteries included policies. |
|
Those things are all possible using the existing retry trait, right? And, if they're not, that's an issue with the trait itself, not with any default policies, correct? My goal with this PR was just to include some basic implementations that are sufficient for simple retry logic — of course, if there are more complex use cases, nothing prevents a user from implementing the trait themselves, but I'm not sure that that should mean that it's a bad idea to include some simpler default behavior. |
There was a problem hiding this comment.
My 2 cents: I think this is worth merging (once the FIXMEs are fixed) as simple retry policies people can use if they want.
I also think some kind of response classifier is useful and we've actually been designing something over in tower-http https://github.com/tower-rs/tower-http/blob/classify-response/tower-http/src/classify.rs. The traits themselves aren't protocol specific so they could in theory live in tower. I think thats a separate discussion however.
|
@hdevalence sorry this has taken so long to land. My opinion from 2 months still lands. If we fix the small docs things I would be okay with merging this. I think the discussion about whether or not to change Retry in a breaking way is a separate discussion. Do you wanna drive this home or should I take over? |
|
@davidpdrsn I think I've lost context on this one, so feel free to push it over the finish line. |
These are lifted out of the test and example code. Two policies are provided:
RetryLimit, with a bounded number of retry attempts;RetryError, with unbounded retry attempts.Both policies require
Req: Cloneas it's not possible to write a genericretry policy without being able to clone requests.
The docs should be updated with a little blurb that explains when they should
not be used. Also, the
Policydocs had anAttemptsexample correspondingto
RetryLimit(formerlyLimitin the tests code); I removed it becauseRetryLimitexists and can be view-sourced.