PinUntilErrorChannel doesn't switch on 429 #661
Merged
Conversation
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
Generate changelog in
|
iamdanfox
requested review from
carterkozak and
ferozco
and removed request for
fawind
iamdanfox
commented
Apr 17, 2020
| live_reloading[UNLIMITED_ROUND_ROBIN].txt: success=60.2% client_mean=PT2.84698S server_cpu=PT1H58M37.45S client_received=2500/2500 server_resps=2500 codes={200=1504, 500=996} | ||
| one_big_spike[CONCURRENCY_LIMITER_BLACKLIST_ROUND_ROBIN].txt: success=79.0% client_mean=PT1.478050977S server_cpu=PT1M59.71393673S client_received=1000/1000 server_resps=790 codes={200=790, Failed to make a request=210} | ||
| one_big_spike[CONCURRENCY_LIMITER_PIN_UNTIL_ERROR].txt: success=100.0% client_mean=PT1.286733552S server_cpu=PT2M48.75S client_received=1000/1000 server_resps=1125 codes={200=1000} | ||
| one_big_spike[CONCURRENCY_LIMITER_PIN_UNTIL_ERROR].txt: success=100.0% client_mean=PT1.135007332S server_cpu=PT2M49.65S client_received=1000/1000 server_resps=1131 codes={200=1000} |
Contributor
Author
There was a problem hiding this comment.
As expected, the graph shows one big spike of requests (i.e. this exact use case) remains pinned to one uri. https://github.com/palantir/dialogue/blob/dfox/pin-until-error-fix/simulation/src/test/resources/report.md#one_big_spikeconcurrency_limiter_pin_until_error
Contributor
There was a problem hiding this comment.
We may want to update this simulation to respond 429 instead of 503
Contributor
Author
There was a problem hiding this comment.
It's intended to be representative of this exact workflow, so it responds 429 above some threshold:
public void one_big_spike() {
int capacity = 100;
servers = servers(
SimulationServer.builder()
.serverName("node1")
.simulation(simulation)
.handler(h -> h.respond200UntilCapacity(429, capacity).responseTime(Duration.ofMillis(150)))
.build(),
SimulationServer.builder()
.serverName("node2")
.simulation(simulation)
.handler(h -> h.respond200UntilCapacity(429, capacity).responseTime(Duration.ofMillis(150)))
.build());
carterkozak
reviewed
Apr 17, 2020
dialogue-core/src/main/java/com/palantir/dialogue/core/PinUntilErrorChannel.java
Show resolved
Hide resolved
ferozco
reviewed
Apr 17, 2020
dialogue-core/src/main/java/com/palantir/dialogue/core/PinUntilErrorChannel.java
Outdated
Show resolved
Hide resolved
carterkozak
reviewed
Apr 17, 2020
dialogue-core/src/test/java/com/palantir/dialogue/core/PinUntilErrorChannelTest.java
Outdated
Show resolved
Hide resolved
…dialogue into dfox/pin-until-error-fix
carterkozak
approved these changes
Apr 17, 2020
Collaborator
|
Released 1.23.1 |
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.
Before this PR
In PDS-117063, a user of our internal atlas-replacement switched from c-j-r -> dialogue and saw server errors.
Looking at the
pinuntilerror.nextNodemetric, it seemed we switched channels 5 times during a supposedly transactional workflow. This meant that some requests landed on one node and others landed on a different node, which caused the second node to return a hard error.cc @LucasIME and @jkozlowski
After this PR
==COMMIT_MSG==
PinUntilErrorChannel doesn't switch on 429, to unblock transactional workflows
==COMMIT_MSG==
Possible downsides?