Implement support for the Proxy-Upstream-Request-Attempts header #2430
Conversation
This file contains 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
Generate changelog in
|
carterkozak
commented
Nov 21, 2024
dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java
Outdated
Show resolved
Hide resolved
This aims to provide coordination between clients and reverse proxy servers which implement retries, allowing us to avoid multiplying out the total retry count. When dialogue is configured with more retries than a proxy, we still may retry more than we want, but not DIALOGUE_ATTEMPTS * PROXY_ATTEMPTS. The worst case scenario should be when the proxy retry attempts don't quite reach the configured dialogue retry limit, and another full round of proxy retries must be accumulated. This should be a reaosnable trade-off because it allows the calling client to be configured with a larger value as needed for stability, without multiplying all retries across the board unnecessarily. Note that when a Dialogue client receives a tcp error/closure, it can make no assumptions about proxy retry attempts, so this only applies to requests which successfully receive a response.
carterkozak
force-pushed
the
ckozak/implement_proxy_retry_attempts_awareness
branch
from
bcf10ba to
1853f05
Compare
carterkozak
added a commit
that referenced
this pull request
Nov 25, 2024
This feature functions in combination with `Proxy-Upstream-Request-Attempts` (#2430) to allow negotiation for retries to occur based on proxy configuration, without requiring clients to be updated or reconfigured.
carterkozak
changed the title
mpritham
reviewed
Nov 25, 2024
mpritham
reviewed
Nov 25, 2024
dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java
Show resolved
Hide resolved
mpritham
reviewed
Nov 25, 2024
|
|
||
| @Test | ||
| void proxyUpstreamRequestAttempts_valid() { | ||
| try (Response response = new TestResponse().withHeader(Responses.PROXY_UPSTREAM_REQUEST_ATTEMPTS, "1")) { |
There was a problem hiding this comment.
Thoughts on also adding some misc spaces to check that it's trim'd?
mpritham
previously approved these changes
Nov 25, 2024
policy-bot
bot
dismissed
mpritham’s stale review
Invalidated by push of 9b9cf0a
carterkozak
commented
Nov 25, 2024
Comment on lines
+316
to
+321
| if (proxyUpstreamRequestAttempts > 1) { | ||
| log.info( | ||
| "Received a Proxy-Upstream-Request-Attempts response header " | ||
| + "indicating retries have already occurred", | ||
| SafeArg.of("proxyUpstreamRequestAttempts", proxyUpstreamRequestAttempts)); | ||
| } |
There was a problem hiding this comment.
I added an info-level log statement here as well
|
Thanks, this needs another +1 since I've pushed again! |
mpritham
approved these changes
Nov 25, 2024
bulldozer-bot
bot
deleted the
ckozak/implement_proxy_retry_attempts_awareness
branch
|
Released 4.5.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This aims to provide coordination between clients and reverse proxy servers which implement retries, allowing us to avoid multiplying out the total retry count.
When dialogue is configured with more retries than a proxy, we still may retry more than we want, but not DIALOGUE_ATTEMPTS * PROXY_ATTEMPTS. The worst case scenario should be when the proxy retry attempts don't quite reach the configured dialogue retry limit, and another full round of proxy retries must be accumulated. This should be a reaosnable trade-off because it allows the calling client to be configured with a larger value as needed for stability, without multiplying all retries across the board unnecessarily.
Note that when a Dialogue client receives a tcp error/closure, it can make no assumptions about proxy retry attempts, so this only applies to requests which successfully receive a response.
==COMMIT_MSG==
Implement support for the Proxy-Upstream-Request-Attempts header
==COMMIT_MSG==
Note this feature is meant to be used in tandem with #2434 for flexibility