-
Notifications
You must be signed in to change notification settings - Fork 965
Introduce RetryLimiter
#6409
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
base: main
Are you sure you want to change the base?
Introduce RetryLimiter
#6409
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.
Overall looks good.
core/src/main/java/com/linecorp/armeria/client/retry/RetryLimiter.java
Outdated
Show resolved
Hide resolved
| * Returns a {@link RetryLimiter} which limits the number of concurrent retry requests. | ||
| * This limiter does not consider {@link RetryDecision#permits()} when limiting retries. | ||
| */ | ||
| static RetryLimiter concurrencyLimiting(long maxRequests) { |
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.
Optional) Would it be useful to support rateLimiting by default?
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.
I forgot to leave a comment on this - the RetryLimiter API doesn't go well with guava's RateLimiter as there is no way to determine if a the state should be rate-limited at a given time.
I'd like to revisit this later on unless I'm missing something/you have a better idea.
core/src/main/java/com/linecorp/armeria/client/retry/RetryLimiters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/retry/RetryLimiters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/retry/TokenBasedRetryLimiter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/retry/TokenBasedRetryLimiter.java
Outdated
Show resolved
Hide resolved
| * <a href="https://github.com/grpc/proposal/blob/master/A6-client-retries.md#throttling-retry-attempts-and-hedged-rpcs">gRPC's documentation</a> | ||
| * for more details. | ||
| */ | ||
| static RetryLimiter tokenBased(float maxTokens, float tokenRatio) { |
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.
It seems that int is more appropriate type for maxTokens. Is there a reason a float is being used?
https://github.com/grpc/proposal/blob/master/A6-client-retries.md#validation-of-retrythrottling
maxTokens MUST be specified and MUST be a JSON integer value in the range (0, 1000].
core/src/main/java/com/linecorp/armeria/client/retry/RetryLimiter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java
Outdated
Show resolved
Hide resolved
| if (permits > 0) { | ||
| if (currentCount == 0) { | ||
| break; | ||
| } | ||
| newCount = Math.max(currentCount - THREE_DECIMAL_PLACES_SCALE_UP, 0); |
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.
The Javadoc says
each positive {@link RetryDecision#permits()} will increment available tokens by {@code tokenRatio}
Which is different from the actual logic.
Does the permit always -1, 0, 1?
I'm asking because this logic doesn't take into account the permit numbers, so I was wondering if we could use more specific names than 'permit'.
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.
I've generalized tokenBased. Although users who prefer gRPC-style retry limiting will need to add a little more logic, the probably makes more sense for general users.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6409 +/- ##
============================================
- Coverage 74.46% 74.13% -0.33%
- Complexity 22234 23036 +802
============================================
Files 1963 2067 +104
Lines 82437 86222 +3785
Branches 10764 11323 +559
============================================
+ Hits 61385 63924 +2539
- Misses 15918 16881 +963
- Partials 5134 5417 +283 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks! 👍 👍
core/src/main/java/com/linecorp/armeria/client/retry/TokenBasedRetryLimiter.java
Outdated
Show resolved
Hide resolved
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.
👍 👍 👍
Motivation:
This changeset attempts to solve the same problem as #6318.
Retry limiting is a concept which limits the number of retries in case a system undergoes a prolonged period of service degradation.
gRPC offers a token-based configuration which limits retries depending on certain predicates, whereas envoy offers a simple concurrency limiting configuration based on the number of active retries.
To support token-based retry limiters, I propose that
RetryDecision#permitsis added as metadata which could signal toRetryLimiterwhether retries should be further made. This could be useful for systems behind load balancers, as the load balancer may return certain status codes depending on the health upstream.To support simple concurrency-based retry limiters, I propose that a
RetryLimiter#shouldRetrymethod is called right before a retry is executed.Modifications:
RetryLimiterwhich acts as an extension to dynamically limit retries.RetryLimiter#shouldRetrydecides whether a retry should be executedRetryLimiter#handleDecisionis invoked when aRetryDecisionis made.RetryDecision#permitsmay be used to update the internal state and decide whether retries should be allowed.RetryDecision#permitsRetryLimitertoRetryConfigResult:
RetryLimiterto limit retry requests.