Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
retry: Add
StandardRetryPolicy
andstandard_policy
mod #698base: master
Are you sure you want to change the base?
retry: Add
StandardRetryPolicy
andstandard_policy
mod #698Changes from 4 commits
70544a2
a3cdc6f
88b7df9
2d24ccb
d97afa5
31e3326
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not sure if I would call the
standard_policy
module a "default" unless it's used by default. calling it "batteries-included" or something might be better?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.
could maybe be nice to make "congestive collapse" a link to Wikipedia or something?
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.
take it or leave it: it seems kind of like these paragraphs could make sense as a bulleted list?
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'm not sure how I feel about calling this "standard policy"; I might prefer a name that actually describes what the policy does, rather than just saying it's "standard"...
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.
It seems kind of unfortunate to me that the default behavior for the batteries-included retry policy is to...never retry anything. In particular, defaulting to a
CloneRequest
implementation that always returnsNone
feels a bit weird to me.What do you think about an API where the default implementation expects that the request type implements
Clone
, and just calls itsClone
method? That way, users whose requests implementClone
never have to think about theCloneRequest
trait, and providing aCloneRequest
implementation is only necessary if you want to retry!Clone
requests?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.
Hmm, it seems like this trait should be able to consider the request as well as the response in determining whether a result is retryable. For example, some HTTP request methods are idempotent, and others are non-idempotent; and non-idempotent requests should generally not be retried. With the current implementation, the
IsRetryable
function can't determine that a given request method is never retryable (without some kind of unfortunate contortion to stuff metadata about the request into the response type, or something).It seems like it would be good to allow the request type to be considered as part of making this decision, especially if we want the
StandardPolicy
to be usable for HTTP retries...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.
it seems like there probably ought to be an implementation of this that's
impl CloneRequest<Req> where Req: Clone
that just callsClone
. Perhaps that ought to be the default?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.
maybe "retry budget" and "backoff" here should link to the respective modules?
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.
a little bit on the fence about whether always
Arc
ing these is the right choice or not --- a question that's definitely come up in the past with other middleware that needs to clone aFn
value. for example,MapRequest
is generic over aF: FnMut(...) + Clone
, andMapResponse
is generic over aFnOnce + Clone
, rather thanArc
ing aFn
...tower/tower/src/builder/mod.rs
Line 200 in 878b10f
in this case, since the functions are always cloned into each request, it wouldn't make sense to allow
FnMut + Clone
, since the mutable state wouldn't be shared across instances of the closure. but, it would allow the use of function pointers (not closures) to avoid allocating anArc
and bumping two reference counts on every request...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.
also, maybe a microoptimization, but it could be worth considering combining the
IsRetryable
andCloneRequest
impls into a single struct that's thenArc
ed, so that we only increment/decrement one atomic ref count on each request, rather than two...but, OTTOH, we would probably have to either box them or make the struct generic, if we did that, and boxing them would mean two heap ptr derefs...:woman_shrugging: