-
Notifications
You must be signed in to change notification settings - Fork 1k
Componentize RetryingClient and RetryingRpcClient
#6292
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
Closed
Closed
Changes from all commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
aa2c19e
refactor: tidy up RetryingClient
schiemon 4aaa678
refactor: tidy up RetryingRpcClient
schiemon 3739f0c
[WIP] refactor: encapsulate an attempt into a separate class
schiemon 26a4e0d
[WIP] refactor: continue modelling: push commit and abort into Retryi…
schiemon b31c646
[WIP] refactor: further extraction
schiemon eeb179f
fix: RetryClientTest
schiemon 6d1ca15
fix: GrpcWebRetryTest
schiemon 5ea6ebd
docs: remove comments
schiemon 7501816
docs: correct comment
schiemon a9d9cd2
refactor: deprecate underdefined `onRetryingComplete`
schiemon 6a0e105
fix: assert RETRYING state when creating attempt context
schiemon e428900
refactor: move req duplicator init into RetryingContext
schiemon 784c54e
refactor: move attempt context creation and attempt execution into `R…
schiemon ceb30ed
feat: log when rctx initialization failed
schiemon cdb6b25
Merge branch 'main' into tidy-up-retrying-client
schiemon 51bb121
Merge remote-tracking branch 'origin/main' into tidy-up-retrying-client
schiemon 337c992
refactor: extract State from AbstractRetryingClient into RetryingCont…
schiemon 60c50bd
refactor: extract counter bookkeeping into RetryCounter
schiemon 111c711
refactor: extract scheduling into RetryScheduler
schiemon 280aea4
refactor: move res and req listeners to init function of the Retrying…
schiemon f1591fe
refactor: rename numberAttemptsWithThisBackoffSoFar to numberAttempts…
schiemon 2d2e65b
refactor: refactor scheduling and counting out of RetryingContext and…
schiemon 286e4bf
refactor: use Precondition for parameter checks in RetryCounter
schiemon c4dd4a9
fix: make HttpRetryingContext/HttpRetryAttempt thread-safe
schiemon 1a174c9
fix: make RpcRetryingContext/RpcRetryAttempt thread-safe
schiemon d55fc03
docs: add doc to RetryingContext
schiemon 3e2b7fa
feat: refactor (Abstract)RetryingClient into RetriedRequest, RetrySch…
schiemon 0f83d69
fix: refactor deadline setting to ClientUtil
schiemon f7655ad
feat: refactor RetryingRpcClient
schiemon 126729a
fix: RpcRetryAttempt.execute
schiemon ee3fd4a
Merge branch 'main' of github.com:schiemon/armeria into tidy-up-retry…
schiemon 402ed1c
fix: fix offset mishap in RetryingRpcClientTest.doNotRetryWhenRespons…
schiemon 1b397b7
docs: fix docs for RetryScheduler
schiemon 6c6f7e8
refactor: remove legacy RetryingContext
schiemon 2f4b698
refactor: remove responsibility of completing original res from Retri…
schiemon 7a7c0a6
Merge branch 'main' into tidy-up-retrying-client
schiemon df10849
fix: fix RetryingRpcClientTest.doNotRetryWhenResponseIsCancelled
schiemon c7a607f
fix: fix blockhound for RetryingRpcClientTest.doNotRetryWhenResponseI…
schiemon 07f3fd7
fix: skip retry task when scheduler after deadline in DefaultRetrySch…
schiemon 1fb4259
refactor: simplify RetryScheduler for sequential retrying
schiemon c5b883b
test: add RetrySchedulerTest
schiemon df62fbe
fix: fix overflow when calculating deadline
schiemon 0785355
fix: fix RetrySchedulerTest
schiemon a7fc28b
docs: add comment in DefaultRetryScheduler
schiemon 006e74e
test: stabilize and extend RetrySchedulerTest
schiemon edfc33b
test: reduce number of direct invocations for CI pipeline
schiemon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
44 changes: 44 additions & 0 deletions
44
core/src/main/java/com/linecorp/armeria/client/retry/AbortedAttemptException.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * Copyright 2025 LY Corporation | ||
| * | ||
| * LY Corporation licenses this file to you under the Apache License, | ||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package com.linecorp.armeria.client.retry; | ||
|
|
||
| import com.linecorp.armeria.common.Flags; | ||
|
|
||
| /** | ||
| * A {@link RuntimeException} that is raised by a {@link RetriedRequest} to signal to the caller of | ||
| * {@link RetriedRequest#executeAttempt} that the attempt has been aborted because the request has been | ||
| * completed - either at call time or during the execution of the attempt. | ||
| */ | ||
| public final class AbortedAttemptException extends RuntimeException { | ||
| private static final long serialVersionUID = -1L; | ||
| private static final AbortedAttemptException INSTANCE = new AbortedAttemptException(true); | ||
|
|
||
| /** | ||
| * Returns a {@link AbortedAttemptException} which may be a singleton or a new instance, depending on | ||
| * {@link Flags#verboseExceptionSampler()}'s decision. | ||
| */ | ||
| public static AbortedAttemptException get() { | ||
| return Flags.verboseExceptionSampler().isSampled( | ||
| AbortedAttemptException.class) ? | ||
| new AbortedAttemptException() : INSTANCE; | ||
| } | ||
|
|
||
| private AbortedAttemptException() {} | ||
|
|
||
| private AbortedAttemptException(@SuppressWarnings("unused") boolean dummy) { | ||
| super(null, null, false, false); | ||
| } | ||
| } |
452 changes: 210 additions & 242 deletions
452
core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java
Large diffs are not rendered by default.
Oops, something went wrong.
85 changes: 85 additions & 0 deletions
85
core/src/main/java/com/linecorp/armeria/client/retry/DefaultRetryCounter.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| /* | ||
| * Copyright 2025 LINE Corporation | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.linecorp.armeria.client.retry; | ||
|
|
||
| import static com.google.common.base.Preconditions.checkArgument; | ||
| import static com.google.common.base.Preconditions.checkState; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| import com.google.common.base.MoreObjects; | ||
|
|
||
| import com.linecorp.armeria.common.annotation.Nullable; | ||
|
|
||
| final class DefaultRetryCounter implements RetryCounter { | ||
| private final int maxAttempts; | ||
|
|
||
| private int numberAttemptsSoFar; | ||
| @Nullable | ||
| private Backoff lastBackoff; | ||
| private int numberAttemptsSoFarForLastBackoff; | ||
|
|
||
| DefaultRetryCounter(int maxAttempts) { | ||
| checkArgument(maxAttempts > 0, "maxAttempts: %s (expected: > 0)", maxAttempts); | ||
| this.maxAttempts = maxAttempts; | ||
| numberAttemptsSoFar = 0; | ||
| lastBackoff = null; | ||
| numberAttemptsSoFarForLastBackoff = 0; | ||
| } | ||
|
|
||
| @Override | ||
| public void consumeAttemptFrom(@Nullable Backoff backoff) { | ||
| checkState(!hasReachedMaxAttempts(), "Exceeded the maximum number of attempts: %s", maxAttempts); | ||
|
|
||
| ++numberAttemptsSoFar; | ||
|
|
||
| if (backoff != null) { | ||
| if (lastBackoff != backoff) { | ||
| lastBackoff = backoff; | ||
| numberAttemptsSoFarForLastBackoff = 0; | ||
| } | ||
| numberAttemptsSoFarForLastBackoff++; | ||
| } else { | ||
| assert lastBackoff == null; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public int attemptsSoFarWithBackoff(Backoff backoff) { | ||
| requireNonNull(backoff, "backoff"); | ||
| if (lastBackoff != backoff) { | ||
| return 0; | ||
| } else { | ||
| return numberAttemptsSoFarForLastBackoff; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public boolean hasReachedMaxAttempts() { | ||
| return numberAttemptsSoFar >= maxAttempts; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return MoreObjects | ||
| .toStringHelper(this) | ||
| .add("maxAttempts", maxAttempts) | ||
| .add("numberAttemptsSoFar", numberAttemptsSoFar) | ||
| .add("lastBackoff", lastBackoff) | ||
| .add("numberAttemptsSoFarForLastBackoff", numberAttemptsSoFarForLastBackoff) | ||
| .toString(); | ||
| } | ||
| } | ||
234 changes: 234 additions & 0 deletions
234
core/src/main/java/com/linecorp/armeria/client/retry/DefaultRetryScheduler.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| /* | ||
| * Copyright 2025 LY Corporation | ||
| * | ||
| * LY Corporation licenses this file to you under the Apache License, | ||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
| * License for the specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package com.linecorp.armeria.client.retry; | ||
|
|
||
| import static com.google.common.base.Preconditions.checkState; | ||
|
|
||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import com.google.common.math.LongMath; | ||
|
|
||
| import com.linecorp.armeria.client.ClientFactory; | ||
| import com.linecorp.armeria.client.ResponseTimeoutException; | ||
| import com.linecorp.armeria.common.annotation.Nullable; | ||
| import com.linecorp.armeria.common.util.UnmodifiableFuture; | ||
|
|
||
| import io.netty.channel.EventLoop; | ||
| import io.netty.util.concurrent.ScheduledFuture; | ||
|
|
||
| final class DefaultRetryScheduler implements RetryScheduler { | ||
| private static final Logger logger = LoggerFactory.getLogger(DefaultRetryScheduler.class); | ||
|
|
||
| private final EventLoop retryEventLoop; | ||
| private final long deadlineTimeNanos; | ||
|
|
||
| private final CompletableFuture<Void> closedFuture; | ||
| private final RetryTaskWrapper retryTaskWrapper; | ||
|
|
||
| @Nullable | ||
| private Runnable nextRetryTask; | ||
| @Nullable | ||
| private ScheduledFuture<?> nextRetryTaskFuture; | ||
| // Long.MIN_VALUE if not set. | ||
| private long earliestRetryTimeNanos; | ||
| private boolean isClosed; | ||
|
|
||
| private final class RetryTaskWrapper implements Runnable { | ||
| @Override | ||
| public void run() { | ||
| assert retryEventLoop.inEventLoop(); | ||
|
|
||
| if (isClosed) { | ||
| // Very unexpected as we would cancel this future on the same event loop here in the scheduler | ||
| // upon close() or closeExceptionally(). | ||
| logger.debug("Tried to run a retry task after the scheduler was closed. Skipping this task."); | ||
| return; | ||
| } | ||
|
|
||
| if (System.nanoTime() > deadlineTimeNanos) { | ||
| closeExceptionally(ResponseTimeoutException.get()); | ||
| return; | ||
| } | ||
|
|
||
| assert nextRetryTask != null; | ||
| final Runnable retryTaskToRun = nextRetryTask; | ||
| nextRetryTask = null; | ||
| nextRetryTaskFuture = null; | ||
| earliestRetryTimeNanos = Long.MIN_VALUE; | ||
|
|
||
| try { | ||
| retryTaskToRun.run(); | ||
| } catch (Throwable t) { | ||
| // Normally we are running retry() which does not throw an exception | ||
| // but let us be defensive here. | ||
| closeExceptionally(t); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| DefaultRetryScheduler(EventLoop retryEventLoop, long deadlineTimeNanos) { | ||
| this.retryEventLoop = retryEventLoop; | ||
| this.deadlineTimeNanos = deadlineTimeNanos; | ||
|
|
||
| retryTaskWrapper = new RetryTaskWrapper(); | ||
| closedFuture = new CompletableFuture<>(); | ||
|
|
||
| nextRetryTask = null; | ||
| nextRetryTaskFuture = null; | ||
| earliestRetryTimeNanos = Long.MIN_VALUE; | ||
| isClosed = false; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean trySchedule(Runnable retryTask, long delayMillis) { | ||
| checkInRetryEventLoop(); | ||
|
|
||
| if (isClosed) { | ||
| return false; | ||
| } | ||
|
|
||
| // We are a sequential scheduler there must not be a nextRetryTask already set. | ||
| checkState(!hasNextRetryTask(), "cannot schedule a retry task when another is scheduled"); | ||
|
|
||
| assert nextRetryTask == null; | ||
| assert nextRetryTaskFuture == null; | ||
|
|
||
| final long retryTimeNanos = Math.max( | ||
| LongMath.saturatedAdd( | ||
| System.nanoTime(), | ||
| TimeUnit.MILLISECONDS.toNanos(delayMillis) | ||
| ), | ||
| earliestRetryTimeNanos | ||
| ); | ||
|
|
||
| if (retryTimeNanos >= deadlineTimeNanos) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| final long nextRetryDelayMillis = TimeUnit.NANOSECONDS.toMillis( | ||
| retryTimeNanos - System.nanoTime()); | ||
|
|
||
| nextRetryTask = retryTask; | ||
| if (nextRetryDelayMillis <= 0) { | ||
| // Run immediately. | ||
| nextRetryTaskFuture = null; | ||
| retryTaskWrapper.run(); | ||
| } else { | ||
| nextRetryTaskFuture = retryEventLoop.schedule( | ||
| retryTaskWrapper, nextRetryDelayMillis, | ||
| TimeUnit.MILLISECONDS); | ||
| nextRetryTaskFuture.addListener(future -> { | ||
| if (isClosed) { | ||
| return; | ||
| } | ||
|
|
||
| if (future.isCancelled()) { | ||
| // future is cancelled when the client factory is closed. | ||
| closeExceptionally(new IllegalStateException( | ||
| ClientFactory.class.getSimpleName() + " has been closed.")); | ||
| } else if (future.cause() != null) { | ||
| // Other unexpected exceptions. | ||
| closeExceptionally(future.cause()); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return true; | ||
| } catch (Throwable t) { | ||
| closeExceptionally(t); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void applyMinimumBackoffMillisForNextRetry(long minimumBackoffMillis) { | ||
| checkInRetryEventLoop(); | ||
|
|
||
| if (isClosed) { | ||
| return; | ||
| } | ||
|
|
||
| // We explicitly disallow that just to avoid having to implement cancelling and rescheduling | ||
| // logic. A scheduler implementing hedging would need to do support that, however. | ||
| checkState(!hasNextRetryTask(), | ||
| "cannot apply minimum backoff when a retry task is scheduled"); | ||
|
|
||
| earliestRetryTimeNanos = | ||
| Math.min( | ||
| Math.max(earliestRetryTimeNanos, | ||
| LongMath.saturatedAdd(System.nanoTime(), | ||
| TimeUnit.MILLISECONDS.toNanos( | ||
| minimumBackoffMillis)) | ||
| ), | ||
| deadlineTimeNanos | ||
| ); | ||
| } | ||
|
|
||
| private boolean hasNextRetryTask() { | ||
| checkInRetryEventLoop(); | ||
| // NOTE: nextRetryTask is null when scheduler is closed. | ||
| return nextRetryTask != null; | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| checkInRetryEventLoop(); | ||
|
|
||
| if (isClosed) { | ||
| return; | ||
| } | ||
|
|
||
| isClosed = true; | ||
| clearRetryTaskIfExists(); | ||
| closedFuture.complete(null); | ||
| } | ||
|
|
||
| private void closeExceptionally(Throwable cause) { | ||
| if (isClosed) { | ||
| return; | ||
| } | ||
|
|
||
| isClosed = true; | ||
| clearRetryTaskIfExists(); | ||
| closedFuture.completeExceptionally(cause); | ||
| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<Void> whenClosed() { | ||
| checkInRetryEventLoop(); | ||
|
|
||
| return UnmodifiableFuture.wrap(closedFuture); | ||
| } | ||
|
|
||
| private void clearRetryTaskIfExists() { | ||
| if (nextRetryTaskFuture != null) { | ||
| nextRetryTaskFuture.cancel(false); | ||
| } | ||
| nextRetryTaskFuture = null; | ||
| earliestRetryTimeNanos = Long.MIN_VALUE; | ||
| nextRetryTask = null; | ||
| } | ||
|
|
||
| private void checkInRetryEventLoop() { | ||
| checkState(retryEventLoop.inEventLoop(), "not in the retryEventLoop: %s but in thread %s", | ||
| retryEventLoop, Thread.currentThread().getName()); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Normally I would store a map from backoff to attempt number however before this change we "forgot" the attempt number once we changed the backoffs. This seems a bit counterintuitive to me. Do you know the reasoning behind this @ikhoon?