Skip to content

Commit

Permalink
RetryingHttpRequesterFilter: rename ofImmediate to `ofImmediateWi…
Browse files Browse the repository at this point in the history
…thDefaultLimit`

Motivation:

It's not obvious for users that `BackOffPolicy.ofImmediate()` retries only 3 times.

Modifications:

- Deprecated `BackOffPolicy.ofImmediate()`;
- Introduce `BackOffPolicy.ofImmediateWithDefaultLimit()` that behaves the same way;
- Cache the returning value for `BackOffPolicy.ofImmediateWithDefaultLimit()`;

Result:

Easier for users to understand that immediate `BackOffPolicy` has a limit for
number of retries.
  • Loading branch information
idelpivnitskiy committed Dec 9, 2022
1 parent d5918cc commit a3c7e6b
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,9 @@ public String toString() {
public static final class BackOffPolicy {

private static final Duration FULL_JITTER = ofDays(1024);
// Subtract 1 because the total strategy anticipates 1 failure from LB not being ready (due to SD available
// events not yet arriving), however this level of retry is strictly applied to request/response failures.
private static final BackOffPolicy IMMEDIATE_DEFAULT_RETRIES = new BackOffPolicy(DEFAULT_MAX_TOTAL_RETRIES - 1);

// FIXME: 0.43 - change field accessor to default
/**
Expand Down Expand Up @@ -373,12 +376,23 @@ public static final class BackOffPolicy {

/**
* Creates a new {@link BackOffPolicy} that retries failures instantly up-to 3 max retries.
*
* @return a new {@link BackOffPolicy} that retries failures instantly up-to 3 max retries.
* @deprecated Use {@link #ofImmediateWithDefaultLimit()}.
*/
@Deprecated
public static BackOffPolicy ofImmediate() {
// Subtract 1 because the total strategy anticipates 1 failure from LB not being ready (due to SD available
// events not yet arriving), however this level of retry is strictly applied to request/response failures.
return new BackOffPolicy(DEFAULT_MAX_TOTAL_RETRIES - 1);
return ofImmediateWithDefaultLimit();
}

/**
* Creates a new {@link BackOffPolicy} that retries failures instantly up-to 3 max retries.
*
* @return a new {@link BackOffPolicy} that retries failures instantly up-to 3 max retries.
* @see #ofImmediate(int)
*/
public static BackOffPolicy ofImmediateWithDefaultLimit() {
return IMMEDIATE_DEFAULT_RETRIES;
}

/**
Expand Down Expand Up @@ -616,12 +630,12 @@ public static final class Builder {
private int maxTotalRetries = DEFAULT_MAX_TOTAL_RETRIES;
private boolean retryExpectationFailed;

private BiFunction<HttpRequestMetaData, RetryableException, BackOffPolicy>
retryRetryableExceptions = (requestMetaData, e) -> BackOffPolicy.ofImmediateWithDefaultLimit();

@Nullable
private Function<HttpResponseMetaData, HttpResponseException> responseMapper;

private BiFunction<HttpRequestMetaData, RetryableException, BackOffPolicy>
retryRetryableExceptions = (requestMetaData, e) -> BackOffPolicy.ofImmediate();

@Nullable
private BiFunction<HttpRequestMetaData, IOException, BackOffPolicy>
retryIdempotentRequests;
Expand Down Expand Up @@ -840,7 +854,7 @@ public RetryingHttpRequesterFilter build() {
if (retryExpectationFailed && throwable instanceof ExpectationFailedException &&
requestMetaData.headers().containsIgnoreCase(EXPECT, CONTINUE)) {
requestMetaData.headers().remove(EXPECT);
return BackOffPolicy.ofImmediate();
return BackOffPolicy.ofImmediateWithDefaultLimit();
}

if (retryIdempotentRequests != null && throwable instanceof IOException
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import static io.servicetalk.http.netty.HttpClients.forSingleAddress;
import static io.servicetalk.http.netty.HttpServers.forAddress;
import static io.servicetalk.http.netty.RetryingHttpRequesterFilter.BackOffPolicy.ofImmediate;
import static io.servicetalk.http.netty.RetryingHttpRequesterFilter.BackOffPolicy.ofImmediateWithDefaultLimit;
import static io.servicetalk.http.netty.RetryingHttpRequesterFilter.BackOffPolicy.ofNoRetries;
import static io.servicetalk.http.netty.RetryingHttpRequesterFilter.Builder;
import static io.servicetalk.http.netty.RetryingHttpRequesterFilter.HttpResponseException;
Expand Down Expand Up @@ -139,7 +140,7 @@ void requestRetryingPredicate() {
.appendClientFilter(new Builder()
.retryRetryableExceptions((requestMetaData, e) -> ofNoRetries())
.retryOther((requestMetaData, throwable) ->
requestMetaData.requestTarget().equals("/retry") ? ofImmediate() :
requestMetaData.requestTarget().equals("/retry") ? ofImmediateWithDefaultLimit() :
ofNoRetries()).build())
.buildBlocking();
assertRequestRetryingPred(failingClient);
Expand All @@ -151,7 +152,7 @@ void requestRetryingPredicateWithConditionalAppend() {
.appendClientFilter((__) -> true, new Builder()
.retryRetryableExceptions((requestMetaData, e) -> ofNoRetries())
.retryOther((requestMetaData, throwable) ->
requestMetaData.requestTarget().equals("/retry") ? ofImmediate() :
requestMetaData.requestTarget().equals("/retry") ? ofImmediateWithDefaultLimit() :
ofNoRetries()).build())
.buildBlocking();
assertRequestRetryingPred(failingClient);
Expand Down

0 comments on commit a3c7e6b

Please sign in to comment.