Use longer retry interval on RESOURCE_EXHAUSTED - #1465
Conversation
| (state.lastFailureCode == Status.Code.RESOURCE_EXHAUSTED) | ||
| ? longInitialSleep | ||
| : regularInitialSleep; | ||
| double sleepMillis = |
There was a problem hiding this comment.
Do I understand correctly, that (n+1)th sleep may be shorter than nth sleep if the gRPC code changed?
If it's the case and it's by design - a good comment explaining why it's by design is needed. Otherwise, it will look like a bug for the next reader.
There was a problem hiding this comment.
Yes, that is the expected behaviour, per consensus. I agree that it is somewhat counter intuitive and I'll add a comment making it clear that this is known and agreed.
However, I won't try to explain why we settled on that behaviour rather than any other. TBH I'd have a hard time arguing this without falling in bike shedding, so the only rationales that stands are essentially that its what the server team asked for and that none of us as come up with a better proposition...
df1211c to
4709694
Compare
4709694 to
c46d980
Compare
| @@ -206,9 +206,11 @@ private class PollLoopTask implements Runnable { | |||
| this.task = task; | |||
| this.pollBackoffThrottler = | |||
| new BackoffThrottler( | |||
There was a problem hiding this comment.
Technically the resource exhausted retry policy could be completely different from the "normal" retry policy.
I'd instead use two different retryer instances but as long as this is all internal and we're free to customize this behavior as needed later this is fine.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Overall LGTM, I'll wait for @Spikhalskiy's input
| double sleepMillis = Math.pow(backoffCoefficient, failureCount - 1) * initialSleep.toMillis(); | ||
| Duration initial = | ||
| (lastFailureCode == Code.RESOURCE_EXHAUSTED) ? congestionInitialSleep : regularInitialSleep; | ||
| double jitter = Math.random() * maxJitter * 2 - maxJitter; |
There was a problem hiding this comment.
I would add a comment here that "we generate a randomize jitter coefficient in (-maxJitter, maxJitter) range". This line is not the easiest to read.
There was a problem hiding this comment.
Jitter is not the most pleasant mechanic for a user to understand what's going on. I would keep it effective only for codes where jitter actually physically makes sense.
Jitter is needed for Code.RESOURCE_EXHAUSTED to avoid situations when all the parties fail and retry at the same time, getting an uneven throttle. For other codes... probably not?
There was a problem hiding this comment.
Jitter is not the most pleasant mechanic for a user to understand what's going on.
Can you elaborate on what is your concern here, from user's POV? Exact timing in unit tests?
In most cases, for non-congestion errors, I'd expect that the numbers we are talking here should be less that some other external factors that might add delay (DNS lookup, network level congestion, garbage collection...). Assuming defaults values, jitter would add at most +/- 20 ms on the third attempt (i.e. effective sleep time in the range [180, 220] ms). Does that sounds bad?
There was a problem hiding this comment.
Can you elaborate on what is your concern here, from user's POV? Exact timing in unit tests?
Users may be surprised by unexpected unstable retry periods and may percieve it as a problem/bug. Stable retry periods are easier to grasp and more standard.
Does that sounds bad?
It's not bad, it's just more complicated than it has to be.
How is it done in other SDKs? Do we apply jitter for all failures?
There was a problem hiding this comment.
TypeScript applies a jitter of 10% by default (user configurable) on every grpc failures.
Go applies a jitter of 20% (non configurable) on every grpc failures. The formula is little bit different though (actual sleep time is random in the range [ sleep before jitter * 0.8, sleep before jitter ])
3 participants
What was changed
Added longer retry interval on gRPC failures of type
RESOURCE_EXHAUSTED. This interval delay is configurable by the user, but defaults to 1000 ms.Why
This failure code indicates the server is having a hard time. Aggressive retry will worsen the situation and could lead to more serious outage. See temporalio/features#96