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

Introduce exponential backoff for @Retry #17207

Closed
artkonr opened this issue May 13, 2021 · 9 comments · Fixed by #18361
Closed

Introduce exponential backoff for @Retry #17207

artkonr opened this issue May 13, 2021 · 9 comments · Fixed by #18361
Assignees
Labels
kind/enhancement New feature or request
Milestone

Comments

@artkonr
Copy link

artkonr commented May 13, 2021

Description

Hey team.

I think, the @Retry component misses exponential backoff config: subsequent requests could be retried with the delay between requests increasing exponentially. It will allow more graceful retrying against the target servers which tend to fail more frequently.

Implementation ideas

Suggestion boils down to adding a new annotation field, such as backoffRate with a double value. This value is then used in a formula to calculate the effective delay time: e = d * b ^ n, where

  • e - effective delay time before retrying
  • d - base delay time set by @Retry#delay()
  • b - backoff rate value
  • n - retry attempt number

E.g. I set delay to 100 millis, backoffRate to 2.0d and retry at most 3 times, which gives me effective delay values:

  • Attempt 1: 100 * 2 ^ 1 = 200 millis
  • Attempt 2: 100 * 2 ^ 2 = 400 millis
  • Attempt 3: 100 * 2 ^ 3 = 800 millis.

P.S.: this feature is already implemented in Mutiny extension in the similar manner.

@artkonr artkonr added the kind/enhancement New feature or request label May 13, 2021
@michalszynkiewicz
Copy link
Member

cc @Ladicek

@Ladicek
Copy link
Contributor

Ladicek commented May 14, 2021

This is tracked in smallrye/smallrye-fault-tolerance#394, where I suggest a different approach. I still think @Retry is overcrowded, so would add backoff strategies as additional annotations.

EDIT: also, adding exponential backoff directly to @Retry would effectively preclude adding more backoff strategies, which is what some people have already asked for (notably, Fibonacci).

@Ladicek Ladicek self-assigned this May 14, 2021
@artkonr
Copy link
Author

artkonr commented May 14, 2021

Hm, good point, @Ladicek.

Just had a look at the ticket you mentioned in the smallrye tracker. I totally agree that there are already too many config items in the annotation, but IMHO, adding custom annotations may be a bit too wordy and confusing.

I myself would approach this by allowing a sort of a RetryConfig interface, which could have a single method like calculateEffectiveDelay(). FixedDelayRetry or FibonacciDelay could be some of the implementors. These classes could in turn be used in the annotation like this:

@interface Retry {

  public Class<? extends RetryConfig> value() default FixedDelayRetry.class;

}

This way, you could ship some pre-set retry policies and allow client code to come up with configs of their own.

Sure, it's a bit of a complication in comparison to the current design, but it's a bit more flexible from my point of view.

I think there may be a concern with this beign a breaking change (especially, since fault-tolerance extension is no longer in preview), but those policy-specific annotations you suggested (like @RetryWithExponentialBackoff) could be a bridge for the time being.

@Ladicek
Copy link
Contributor

Ladicek commented May 14, 2021

Yea, using an interface like this was also one of the options discussed in MicroProfile Fault Tolerance. See e.g. eclipse/microprofile-fault-tolerance#220 (comment)

There are just too many moving parts :-)

@artkonr
Copy link
Author

artkonr commented May 14, 2021

Thank you for the info, @Ladicek. Just wanted to clarify: the main obstacle in adopting the strategy-based approach is the expected complexity of it, correct?

Also, I think that this strategy-based approach has another benefit: options like jitter could be included into this config item.

@Ladicek
Copy link
Contributor

Ladicek commented May 14, 2021

Complexity, as well as a deviation from a fully declarative approach we're pursuing with annotations. (I have a programmatic API for SmallRye Fault Tolerance in mind, where this would be more natural. It hasn't been a priority, so far.)

@artkonr
Copy link
Author

artkonr commented May 14, 2021

OK, got it, thanks a lot! Should I close this item here, since the discussion is already under way elsewhere?

@Ladicek
Copy link
Contributor

Ladicek commented May 14, 2021

I don't really mind -- feel free to leave this open, so that anyone can track this on the Quarkus level.

@Ladicek
Copy link
Contributor

Ladicek commented Jul 1, 2021

This will be implemented in SmallRye Fault Tolerance 5.2.0, see smallrye/smallrye-fault-tolerance#449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants