DRIVERS-3535 - Client Backpressure with retryAfterMS#1953
Conversation
Co-authored-by: Sergey Zelenov <sergey.zelenov@mongodb.com>
tadjik1
left a comment
tadjik1
left a comment
There was a problem hiding this comment.
Thanks @NoahStapp, everything looks good! I will keep an eye on retryAfterMS max value, once it's set I'll approve this PR.
Co-authored-by: Steven Silvester <steve.silvester@mongodb.com>
|
After further discussion with Server, I've simplified the backoff calculation formula to remove the custom jitter for |
|
As a note, the two failing checks should be fixed by #1958 |
| retry_after_ms = exc.retry_after_ms | ||
| if retry_after_ms: | ||
| retry_after = retry_after / 1000 # Convert from milliseconds to seconds | ||
| backoff = jitter * min(MAX_BACKOFF, retry_after * 2 ** (attempt - 1)) |
There was a problem hiding this comment.
jitter is undefined in this if block, should it be hoisted to above it?
|
|
||
| # If present on the error, retryAfterMS sets the base backoff | ||
| retry_after_ms = exc.retry_after_ms | ||
| if retry_after_ms: |
There was a problem hiding this comment.
I think the retry_after should be set as BASE_BACKOFF, then set to the new value if retry_after_ms is given, then use a single line to set the backoff
There was a problem hiding this comment.
🙏 even more concise, thanks!
| - `MAX_BACKOFF` is 10000ms. | ||
| - This results in delays of 100ms and 200ms before accounting for jitter. | ||
| 3. If the request is eligible for retry (as outlined in step 2 above), the client MUST apply backoff according to the | ||
| following formula: `backoff = jitter * min(MAX_BACKOFF, BASE_BACKOFF * 2^(attempt - 1))` |
There was a problem hiding this comment.
Is attempt the number of retries, or does it include the first time the command is executed? It seems like it should be the former, but can we be explicit in the list below?
There was a problem hiding this comment.
attempt is the retry number, so attempt=1 is the first retry.
| 2. `MAX_BACKOFF` is 10000ms. | ||
| 3. `BASE_BACKOFF` is constant 100ms. | ||
| 4. This results in delays of 100ms and 200ms before accounting for jitter. | ||
| 5. If `retryAfterMS` is present on the error and has a positive value, the client MUST use that value instead of |
There was a problem hiding this comment.
Is retryAfterMS mis-named? It seems weird to multiply a value with that name by 2^(attempt - 1).
I'm not asking that we change it, but consider a sentence of explanation.
There was a problem hiding this comment.
retryAfterMS is the server-supplied base backoff to use in place of the driver's default BASE_BACKOFF. Is the explaination you're looking for making that more clear?
| else: | ||
| cmd = {"legacy hello": 1, "helloOk": 1} | ||
| cmd["backpressure"] = True | ||
| cmd["backpressure"] = "2" |
There was a problem hiding this comment.
Can we add this to the normative part of the spec, perhaps as the third paragraph in "Connection handshake"? Something with a MUST, that also defines the semantics of the value "2", as compared to True.
5 participants
Please complete the following before merging:
clusters).