feat: support request hedging

[schiemon]

THIS PR IS CURRENTLY BLOCKED

We extracted sub-PRs to reduce the overall size of this PR. Those need to be handled before we can continue:

• PR for endResponseWithChild: Add endResponseWithChild to RequestLogBuilder #6294
• PR to add tests to RetryClientTest: Add verifications of ClientRequestContexts to RetryClientTest and RetryingRpcClientTest #6296
• PR to clean up Retrying(Rpc)Client: Componentize RetryingClient and RetryingRpcClient #6292

Motivation:

This PR enables RetryingClient and RetryingRpcClient to perform request hedging.

What is request hedging?

Detailed explanations can be found in the paper The Tail at Scale 1 and the section for request hedging in the gRPC documentation 2.

How does this implementation compare to the gRPC spec?

This implementation of request hedging tries to follow the semantics defined by gRPC as closely as possible. As such, let us take the gRPC request hedging spec 4 as a baseline and discuss how it differs from it. For that, it makes sense to study the spec first before continuing reading.

Configuration

gRPC drives request hedging via its service config in which one can specify a so-called hedgingPolicy. It consists of three fields:

• maxAttempts

maximum number of in-flight requests while waiting for a successful response. This is a mandatory field, and must be specified. If the specified value is greater than 5, gRPC uses a value of 5.

Limiting the number of attempts is still supported by RetryConfig.maxTotalAttempts(). This hedging mechanism respects that limit. However, we are not imposing an absolute limit on the number of possible attempts like gRPC.

• hedgingDelay

amount of time that needs to elapse before the client sends out the next request while waiting for a successful response. This field is optional, and if left unspecified, results in maxAttempts number of requests all sent out at the same time.

We allow specifying this via hedgingDelay and hedgingDelayMillis in RetryConfigBuilder/ RetryConfig. The difference here is that we are not going to send all requests at once in case this option is unspecified, but instead assume that the user wants to use the standard retry policy instead of hedging.

• nonFatalStatusCodes

an optional list of grpc status codes. If any of the hedged requests fails with a status code that is not present in this list, all outstanding requests are canceled and the response is returned to the application.

The pre-existing RetryRules are able to model this behavior (and more). More specifically, a user can use RetryRuleBuilder.onStatus to decide whether to continue retrying. Note that if a rule does not provide a backoff (e.g. through RetryingDecision.noRetry()) retrying will be stopped completely and ongoing attempts will be aborted.

Server Pushback

gRPC defines a metadata key, grpc-retry-pushback-ms 3, for the server to communicate to the client a minimum retry delay. This implementation does not add support for the grpc-retry-pushback-ms metadata key but instead continues to react on the standard Retry-After header.

I want to highlight a slight difference in how gRPC expects clients to behave on the grpc-retry-pushback-ms header key compared to how Armeria now reacts on Retry-After. gRPC says:

If the value for pushback is negative or unparseable, then it will be seen as the server asking the client not to retry at all.

Currently, Armeria ignores the field in the unparseable case and with that just continues retrying. This implementation does not change this behavior.

Throttling

gRPC defines a throttling mechanism for the client 5. This is left unimplemented and tracked by #6282.

Modifications

RetryConfigBuilder, RetryConfig

To enable request hedging, RetryConfigBuilder now offers two methods to specify the hedging delay:

• hedgingDelay(Duration hedgingDelay)
• hedgingDelayMillis(long hedgingDelayMillis)

As mentioned above, setting this to zero is valid and makes the Retrying(Rpc)Client immediately push out all maxRequests and make it wait for one to finish. If not set, hedging is disabled and Retrying(Rpc)Client behaves like the RetryingClient before this change. Negative values are illegal.

RetryScheduler

With request hedging, Retrying(Rpc)Client needs to handle concurrent retry attempts
trying to schedule retry tasks with different backoff delays and (optional) minimal delays from servers (see RETRY_AFTER HTTP header).
To handle this complexity and improve testability, I introduced RetryScheduler. It is used behind the scenes by AbstractRetryingClient to keep track of the next retry task to be executed while enforcing scheduling constraints given by the minimal delays from the servers (earliestRetryTimeNanos) and the attempt/response timeout (latestNextRetryTimeNanos).
With that, it takes care of retry tasks "overtaking" each other when a retry rule specifies a shorter delay than the currently scheduled task, and rescheduling when a server reports a longer minimum delay than the one already scheduled.

AbstractRetryingClient

The following modifications were made (some in a breaking way):

1. Removed getNextDelay

getNextDelay was a mutable operation that increased the attempt count. This is not safe with hedging, as we need to account for concurrent attempts between this call and the subsequent call to scheduleNextRetry. For example, during that window, another attempt with an earlier backoff might call getNextDelay, but fail because we already consumed an attempt. As a result, we would incorrectly schedule the late retry task. To fix this, we would need to re-check the state of pending attempts during scheduling, rendering getNextDelay useless.

This is why backoff calculation and scheduling must happen synchronously. Therefore, I removed getNextDelay and moved the delay calculation into the scheduleNextRetry method.

2. Replaced onRetryingComplete with different "retry lifecycle API"

With hedging, we need to handle interleaved attempt starts and completions. As such, having a method that just completes retrying does not make sense as the caller does not/should not have any knowledge about other pending attempts or if there is a scheduled retry task. With that in mind, I replaced onRetryingComplete with the following "retry lifecycle API":

protected void startRetryAttempt(ClientRequestContext ctx,
    ClientRequestContext attemptCtx,
    O attemptRes,
    AttemptWinHandler<O> onWinHandler,
    AttemptAbortHandler<O> onAbortHandler
)

startRetryAttempt registers the attempt identified with attemptCtx as being started and now pending. It also registers handlers that define what should happen when the attempt is going to win the retry process (i.e. when its response should be propagated back to the decorator chain) and what should happen when it aborts (e.g. when another attempt returned with an earlier response).

Internally, it is used to track the number of started attempts to be able to decide when the retrying process needs to be concluded. That is why in a retry task/in the first call to doExecute this method needs to be called before the task/method returns. Otherwise, AbstractRetryingClient may see that no attempt is pending and conclude the retrying process with the last response if it exists.

protected void completeRetryAttempt(
    ClientRequestContext ctx,
    ClientRequestContext attemptCtx,
    O attemptRes,
    boolean isWinning
)

completeRetryAttempt is the counterpart to startRetryAttempt. It tells AbstractRetryingClient whether the current attempt concludes the retry process (isWinning) or finishes while giving other attempts the chance to conclude. If the attempt is winning, we abort all other pending attempts by calling their abort handlers, then invoke the win handler for this attempt with attemptCtx.

protected static void completeRetryingIfNoPendingAttempts(ClientRequestContext ctx)

Checks whether there are any pending attempts or scheduled retry tasks and if not, concludes the retry process with the last completed attempt as the winning attempt. It can be used inside the actionOnException handler for scheduleNextRetry when a non-critical exception is caught. There, we need to make sure that we are not leaving the handler without checking that we are the last point to conclude retrying. For that, it can use completeRetryingIfNoPendingAttempts.

protected static void completeRetryingExceptionally(
    ClientRequestContext ctx,
    Throwable cause
)

Concludes retrying by aborting all pending attempts by calling their abort handlers. It also cancels the current retry task if there is any. The caller must make sure to properly abort the original response themselves. This method can be called after the moment the first time doExecute is invoked.

3. Amended scheduleNextRetry

I moved from

protected static void scheduleNextRetry(
    ClientRequestContext ctx,
    Consumer<? super Throwable> actionOnException,
    Runnable retryTask,
    long nextDelayMillis
)

to

protected void scheduleNextRetry(ClientRequestContext ctx,
    Consumer<Integer> retryTask,
    Backoff backoff,
    Consumer<? super Throwable> actionOnException
)
protected static void scheduleNextRetry(
    ClientRequestContext ctx,
    Consumer<Integer> retryTask,
    Backoff backoff,
    long retryDelayFromServerMillis,
    Consumer<? super Throwable> actionOnException
)

As mentioned above, the new scheduleNextRetry combines the work for getRetryDelay and the old scheduleNextRetry.

To give a rationale, scheduleNextRetry tries to schedule the retryTask according to backoff and retryDelayFromServerMillis. Potential exceptions are reported to actionOnException. There is a list of expected exception types in RetrySchedulingException the user can react to. The actionOnException must either call completeRetryingExceptionally or completeRetryingIfNoPendingAttempts.
If it is unable to schedule the retryTask, it will at least communicate the retryDelayFromServerMillis to the scheduler so that the current retry task can be rescheduled and future attempts delayed accordingly.
I need to pass in the Backoff because consuming an attempt for a backoff occurs upon task execution and not as before upon scheduling. This is because there is a possibility that this task is "overtaken" by a retry task with lower delay during the time the attempt is waiting for execution. In that case, we would increment the total attempt count twice but retry only once.

4. Renamed newDerivedContext to newAttemptContext

In case this class stays public, it is weird for users to see the word "derived". With the renaming, we make it clear that this is a helper function for creating new contexts for attempts.

RetryingClient

1. Adapted the class to fit the new API exposed by AbstractRetryingClient.
2. I had a difficult time differentiating ClientRequestContexts and response objects from retry attempts from the top-level one. To assist future new readers like me, I renamed variables accordingly (e.g. attemptCtx).
3. As suggested by @ikhoon in a comment, I added RetryingContext to bundle as the parameter-passing was getting unwieldy.

RetryingRpcClient

1. Adapted the class to fit the new API exposed by AbstractRetryingClient.
2. As with RetryingClient, I renamed the variables to be more intuitive (hopefully).

DefaultRequestLog / RequestLogBuilder

Added endResponseWithChild(RequestLogAccess child) to be able to end the request log with any child log and not only with the one from the last retry attempt. This is necessary as a hedged request can still complete after the (hedged) requests started before it.

Result:

• Closes Feature Request: gRPC request hedging #5200

If users did not extend AbstractRetryingClient, there should be no breaking changes or changes in behavior by this change. They can opt-in to request hedging by configuring a hedgingDelay on a per RetryConfig basis.

In case users did extend from AbstractRetryingClient, they would face a breaking change as they would need to adapt startRetryAttempt, completeRetryAttempt, completeRetryingExceptionally, completeRetryingIfNoPendingAttempts and the amended scheduleNextRetry method. As a minor breaking change, they would need to rename their usages of newDerivedContext to newAttemptContext. I was questioning before and now I am questioning even more having AbstractRetryingClient in the public API.

Open Questions/ TODOs

• Should I open a separate ticket for introducing a RetryingContext to RetryingClient. It is supposed to bundle the huge amount of arguments that are passed inside the methods of RetryingClient? Is there something else I can do to ease review?
• Should we stop retrying in case the server sends an unparseable grpc-retry-pushback-ms header?
• Should supporting grpc-retry-pushback-ms be a follow-up?
• Add more specialized tests (e.g. with streaming).
• Add missing Javadoc
• Add missing docs

[github-actions]

🔍 Build Scan® (commit: 9c1ecca)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	❌ (failure)	https://ge.armeria.dev/s/52o5im5qioljk
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/h2jbo6jcoba2i
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	❌ (failure)	https://ge.armeria.dev/s/46u57fq5vvvt6
build-ubicloud-standard-16-jdk-17-min-java-11	❌ (failure)	https://ge.armeria.dev/s/i2koucx7cc6wu
build-ubicloud-standard-16-jdk-17-leak	❌ (failure)	https://ge.armeria.dev/s/nhpe3ffpx64s6
build-ubicloud-standard-16-jdk-11	❌ (failure)	https://ge.armeria.dev/s/gxwpfgssg3np6
build-macos-latest-jdk-21	❌ (failure)	https://ge.armeria.dev/s/wkyyphusnkzci

[codecov]

Codecov Report

Attention: Patch coverage is 82.13802% with 132 lines in your changes missing coverage. Please review.

Project coverage is 74.65%. Comparing base (8150425) to head (7927a92).
Report is 104 commits behind head on main.

Files with missing lines	Patch %	Lines
...p/armeria/client/retry/AbstractRetryingClient.java	78.64%	25 Missing and 35 partials ⚠️
.../linecorp/armeria/client/retry/RetryScheduler.java	78.20%	18 Missing and 16 partials ⚠️
.../linecorp/armeria/client/retry/RetryingClient.java	89.17%	7 Missing and 10 partials ⚠️
...ecorp/armeria/client/retry/RetryConfigBuilder.java	52.94%	7 Missing and 1 partial ⚠️
...corp/armeria/common/logging/DefaultRequestLog.java	80.55%	3 Missing and 4 partials ⚠️
...necorp/armeria/client/retry/RetryingRpcClient.java	90.47%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6252      +/-   ##
============================================
+ Coverage     74.46%   74.65%   +0.18%     
- Complexity    22234    22538     +304     
============================================
  Files          1963     1987      +24     
  Lines         82437    83520    +1083     
  Branches      10764    10859      +95     
============================================
+ Hits          61385    62350     +965     
- Misses        15918    15961      +43     
- Partials       5134     5209      +75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[schiemon]

Hi team,
while I’m squashing the last bugs, I’d love to hear what you think about this PR. First, it would be interesting to know if:

• you're fine with enabling hedging via hedgingDelay(Millis) on a RetryConfig
• you have suggestions for breaking this PR into smaller pieces (if you prefer that)
• you're fine with the changes to AbstractRetryingClient, and what you think about making it private

[ikhoon]

Would you revert the changes which are unrelated to this PR?

[schiemon]

OK by looking at other PRs it seems like in Armeria, we are not updating the copyright header upon change. I correct that. As the developer guide does not mention it, let us update it. Happy to contribute that too.

Furthermore I suggest to off-load following changes into separate PRs:

• Adding RetryingContext in RetryingClient to reduce parameter count of private methods.
• Adding endResponseWithChild to RequestLog and RequestLogBuilder.
• Adding checks to tests inside RetryingClientTest verifying request logs are completed properly.

Let me know if this makes sense to you or if they are changes that you want to see together in this PR @ikhoon.

[ikhoon]

As the developer guide does not mention it, let us update it. Happy to contribute that too.

Sounds great.

Furthermore I suggest to off-load following changes into separate PRs:

That is a good idea. It would help us understand the changes better.

[schiemon]

Here we go:

• PR for updating docs: Developer guide: add clarification on copyright header #6295
• PR for endResponseWithChild: Add endResponseWithChild to RequestLogBuilder #6294
• PR to add tests to RetryClientTest: Add verifications of ClientRequestContexts to RetryClientTest and RetryingRpcClientTest #6296
• PR to clean up Retrying(Rpc)Client: Componentize RetryingClient and RetryingRpcClient #6292

PTAL