Rate limiting retries

[jattisha]

Resilience4j version: 1.7.0

Java version: 11

Hello!

We are migrating to Resilience4J from an in-house resilience library built for our company. One limitation that we've found in R4J that we had in our old library was the ability to rate-limit retry calls (e.g. only retry a dependency at a rate no greater than 1 TPS) to prevent retry-storms for our dependency.

I tried doing this in Resilience4J by doing something like this:

RateLimiter rateLimiter; // Rate limiter configured for some TPS, let's say 1 TPS
RetryConfig retryConfig = RetryConfig.custom()
        .maxAttempts(2)
        .intervalFunction((numRetries) -> {
            if (rateLimiter.acquirePermission()) {
                return IntervalFunction.ofExponentialBackoff(5L, 2).apply(numRetries);
            } else {
                return 0L;
            }
        })
        .ignoreExceptions(InvalidInputException.class)
        .build();

and I honestly think this mostly worked, but I think it's out of convenience and not out of design, and I want to get confirmation that I'm not doing something illegal that would be broken later. This code heavily relies on this line of code here. The fact that the interval function returns 0 after hitting the rate limit means that we eventually completeExceptionally, which is what we want to do (stop retrying and just propagate the exception up at this point, since we've reached our limit). This does throw a bit of a wrinkle in some metrics that we built around this, but that's fine and we can assume that this is a possibility. Tl;dr: is this a kosher solution that would be endorsed by R4J?

[RobWin]

Hi,
you want to have a different rate limit for retry attempts than for the first call, correct?

That's an interesting challenge.

Of course you can do anything you want in the interval function which calculates the wait duration between retry attempts. That means your solution is not how it was designed, but should work in general. But I agree that it might wrinkle your metrics.

I think I would create two RateLimiter instances and decorate the same endpoint twice and use retry/ratelimiter only in the fallback method.

[jattisha]

That's definitely another option as well! This method is definitely more convenient for us, but do you think if we went with this route that it's reasonable to return 0 from an interval function and force the retry to complete exceptionally? I am a bit worried that 0 is treated more like an "undefined" behavior that may change later on, although it definitely seems reasonable to return 0 and just say "don't retry!".

[RobWin]

Actually 0 means that there should be no wait duration between retry attempts.
It should not force a retry attempts to complete exceptionally. Unfortunately the code seems to be inconsistent :(

In Reactor RetryOperator it is as follows:

long waitDurationMillis = retryContext.onError(throwable);

if (waitDurationMillis == -1) {
    return Mono.error(throwable);
}

In RxJava RetryTransformer:

long waitDurationMillis = retryContext.onError(throwable);

if (waitDurationMillis == -1) {
    return Observable.error(throwable);
}

or see RetryImpl

 private long handleThrowable(Throwable throwable) {
    if (!exceptionPredicate.test(throwable)) {
        failedWithoutRetryCounter.increment();
        publishRetryEvent(() -> new RetryOnIgnoredErrorEvent(getName(), throwable));
        return -1;
    }
    return handleOnError(throwable);
}

I think you've discovered a bug.
-1 should signal to abort retry attempts.

It's the first time that someone wants to use the intervalFunction to abort retries.

[jattisha]

I think it's a pretty good way to abort retries, at least for our use case. I think you're right that it's actually a bug. From my testing as well, I think if you return a non-positive value for the interval function, it completes exceptionally but never ends up emitting the RetryOnErrorEvent, the only events that get emitted are the RetryOnRetryEvents up until we get a non-positive value, and I believe there's also a RetryOnRetryEvent emitted with an interval of whatever the wait duration is. In that case, I believe it should not emit a RetryOnRetryEvent since there was no retry that occurred (we aborted) and also emit a RetryOnErrorEvent when we do finally complete exceptionally.

[RobWin]

Would you like to create a Bugfix?

[jattisha]

Cut a pull request for this issue!

#2067