Skip to content
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 middleware improvements #682

Open
6 of 19 tasks
LucioFranco opened this issue Aug 11, 2022 · 4 comments
Open
6 of 19 tasks

Retry middleware improvements #682

LucioFranco opened this issue Aug 11, 2022 · 4 comments
Assignees
Labels
A-retry Area: The tower "retry" middleware C-enhancement Category: A PR with an enhancement or a proposed on in an issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate P-high High priority

Comments

@LucioFranco
Copy link
Member

LucioFranco commented Aug 11, 2022

This issue scopes out the changes we are proposing to the retry middleware to improve its ergonomics. Currently, the retry middleware is quite hard to use and requires implementing a custom Policy. Writing this policy is non-trivial and is more work than it should be.

These improvements are aimed at setting up retries with tower to be easier and more user friendly. As well as providing good defaults that work out of the box.

List of improvements to tower and tower-http:

  • Simplify Policy (retry: Change Policy to accept &mut self #681).
    • Change trait fn to take &mut self.
    • Change Future output to ().
    • Alllow Policy to be object safe.
  • Add generic backoff utilities. retry: Add generic backoff utilities #685
    • Add some trait Backoff that has an associated future type that allows others to use this utility without being tied to tokio::time.
    • Add a ExponentialBackoff type that implements Backoff, ported from linkerd2-proxy.
    • Add Rng utilities util: Add rng utilities #686
  • Budget improvements.
  • Add a new batteries included standard retry policy retry: Add StandardRetryPolicy and standard_policy mod #698
    • New StandardRetryPolicy combining impl Backoff and impl Budget.
    • Add StandardRetryPolicyBuilder that accept closures (?) for is_retryable(&mut Req, &mut Result<Res, E>) -> bool and a clone_request(&Req) -> Option<Req>.
  • tower-http improvements.
    • Add new retry module
    • Implement ReplayBody similar to the one implemented in linkerd2-proxy.
    • Add new HttpRetry layer that accepts higher level constructs for retrying, like ClassifyResponse, and will wrap http request bodies with ReplayBody.
  • Documentation
    • Blog post on how to setup retries with tower and tower-http.
    • Examples for thick clients with retries in both tower and tower-http.

Example code

tower examples with no http:

let policy = StandardRetryPolicy::builder()
    .should_retry(|res| match res {
        Ok(_) => false,
        Err(_) => true,
    })
    .clone_request(|r| Some(*r))
    .build();

let mut svc = ServiceBuilder::new()
    .retry(policy)
    .buffer(10)
    .timeout(Duration::from_secs(10))
    .service(svc);

tower-http examples:

let make_classifier = ServerErrorsAsFailures::make_classifier();

let mut svc = ServiceBuilder::new()
    .set_request_id("Request-Id".try_into().unwrap(), MakeRequestUuid)
    .retry(StandardHttpPolicy::new(
        make_classifier,
        ExponentialBackoff::default(),
        Budget::default(),
        |e| match e {
            ServerErrorsFailureClass::Status(s) => true,
            ServerErrorsFailureClass::Error(s) => false,
        },
    ))
    .timeout(Duration::from_secs(5))
    .service(client);

cc @rcoh @hawkw @jonhoo @seanmonstar @davidpdrsn

@LucioFranco LucioFranco added C-enhancement Category: A PR with an enhancement or a proposed on in an issue. E-help-wanted Call for participation: Help is requested to fix this issue. A-retry Area: The tower "retry" middleware P-high High priority E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Aug 11, 2022
@LucioFranco LucioFranco self-assigned this Aug 11, 2022
@lovesegfault
Copy link

lovesegfault commented Aug 11, 2022

It would be cool if this could integrate with 429/Retry-After header out-of-the-box.

@LucioFranco
Copy link
Member Author

@lovesegfault yeah, good idea, this seems like a config option that could live in HttpRetry.

@DontBreakAlex
Copy link

+1 to lovesegfault, I found this crate will looking for a way to handle Retry-After easily

LucioFranco added a commit that referenced this issue Oct 7, 2022
This PR adds a new `standard_policy` module within the retry module
that provides a batteries included policy to be used with the retry
middleware. The policy combines the `Budget` type and generic backoff
utlities from the `backoff` module to provide an easy to use policy
with good defaults.

This PR also includes a `StandardRetryPolicyBuilder` as well as two
new traits `IsRetryable` and `CloneRequest`. These each have blanket
impls for closures. The reason that this implementation breaks these
out into two different traits is to allow `tower-http` to provide
a custom `CloneRequest` implementation that will be able to clone
some sort of `ReplayBody` and let the user pass in the retry decision
implementation.

Ref #682
@xj524598
Copy link

so this issue delay ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-retry Area: The tower "retry" middleware C-enhancement Category: A PR with an enhancement or a proposed on in an issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate P-high High priority
Projects
None yet
Development

No branches or pull requests

4 participants