-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add short-circuit support in retry pattern #32035
Conversation
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.
Seems useful
* shouldRetry = { (ex) => ex.isInstanceOf[IllegalArgumentException] }) | ||
* }}} | ||
*/ | ||
def retry[T](attempt: () => Future[T], shouldRetry: Throwable => Boolean, attempts: Int)( |
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.
Align order of parameters with the other signature with more params, either attempts in between in both or lambdas first in both? (Unless that causes compiler ambiguity, but I don't think it should)
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.
attempts
in-between does cause compiler confusion (with the delayFunction
variants), so would break source compatibility, but shouldRetry
after attempt
does work.
|
||
private val alwaysRetry: Throwable => Boolean = { _ => | ||
true | ||
} |
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.
Use ConstantFun.anyToTrue
instead
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.
looking good
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.
LGTM
I have something like this too,eg: @SneakyThrows
public static <V> V retryWithBackoff(
final Callable<V> callable,
final Predicate<V> shouldRetryPredicate,
final int maxRetryTime,
final int sleepInMills,
final String hint) {
return retryWithBackoff0(callable, shouldRetryPredicate, maxRetryTime, 0, sleepInMills, hint);
} So I think the |
There are situations where the nature of failure of a future indicates that any retry (regardless of delay) is extremely likely to fail in the same way. In such a situation, the retry is unlikely to be of value and it may be better to surface the failure more quickly.
Consider, for instance, an API which validates inputs asynchronously (e.g. imagine that the validations to perform are dynamically obtained from a server, but the validations themselves only change on a fairly long timeframe). It is unlikely that the validations change from one retry to another, so retrying the validations is unlikely to change the result. However, we'd like the API calls to be retried if they fail for some other reason that's more amenable to retries.
This change adds support for classifying failed futures as "should retry" or "should not retry" based on the nature of the failure (in HTTP terms, think of the difference between 4xx and 5xx, ignoring that Akka HTTP will give successful futures for 4xx/5xx responses).
An alternative approach in the current API is to have a
recoverWith
as part of the attempt which transforms the "should not retry" failures into distinguished successes so that no more attempts are performed, and aflatMap
outside of the retry transforms those distinguished successes back to a failure: