DRIVERS-3239: Add exponential backoff to operation retry loop for server overloaded errors

[baileympearson]

DRIVERS-3239

Overview

This PR adds support for a new class of errors (SystemOverloadedError) to drivers' operation retry logic, as outlined in the design document.

Additionally, it includes a new argument to the MongoDB handshake (also defined in the design document).

Python will be second implementer.
Node implementation: mongodb/node-mongodb-native#4806

Testing

The testing strategy is two-fold:

• Building off of Ezra's work to generate unified tests for retryable handshake errors, this PR generates unified tests to confirm that:

  • operations are retried using the new SystemOverloadedError label
  • operations are retried no more than 5 (current MAX_ATTEMPTS, as defined in the spec) times

• Following Iris's work in DRIVERS-1934: withTransaction API retries too frequently #1851, this PR adds a prose test that ensures drivers apply exponential backoff in the retryability loop.

• Update changelog.

• Test changes in at least one language driver.

• Test these changes against all server versions and topologies (including standalone, replica set, and sharded
  clusters).

[blink1073]

It looks like you also need to bump the schema version:

source/client-backpressure/tests/backpressure-retry-loop.yml invalid
[
  {
    instancePath: '/tests/0/operations/3/expectError',
    schemaPath: '#/definitions/expectedError/type',
    keyword: 'type',
    params: { type: 'object' },
    message: 'must be object'
  }
]
 using schema v1.3
source/client-backpressure/tests/backpressure-retry-max-attempts.yml invalid
[
  {
    instancePath: '/tests/0/operations/1/expectError',
    schemaPath: '#/definitions/expectedError/type',
    keyword: 'type',
    params: { type: 'object' },
    message: 'must be object'
  }
]
 using schema v1.3source/client-backpressure/tests/backpressure-retry-loop.yml invalid
[
  {
    instancePath: '/tests/0/operations/3/expectError',
    schemaPath: '#/definitions/expectedError/type',
    keyword: 'type',
    params: { type: 'object' },
    message: 'must be object'
  }
]
 using schema v1.3
source/client-backpressure/tests/backpressure-retry-max-attempts.yml invalid
[
  {
    instancePath: '/tests/0/operations/1/expectError',
    schemaPath: '#/definitions/expectedError/type',
    keyword: 'type',
    params: { type: 'object' },
    message: 'must be object'
  }
]
 using schema v1.3

[blink1073]

WIP Python implementation: mongodb/mongo-python-driver#2635

[blink1073]

All unified and prose tests are passing in the Python implementation.

Edit: we're still failing one unified test, "client.clientBulkWrite retries using operation loop", investigating...

Edit 2: we're all good now

[jyemin]

I only reviewed the specification changes, not the pseudocode or tests. Those are best reviewed by implementers.

[jyemin]

Suggested change

	to according to the following formula: `delayMS = j * min(maxBackoff, baseBackoff * 2^i)`
	to the following formula: `delayMS = j * min(maxBackoff, baseBackoff * 2^i)`

[baileympearson]

done

[jyemin]

Since the rules include all the deposits into the token bucket, consider adding withdrawals as well.

[baileympearson]

done

[jyemin]

Anything to add here, or just remove?

[baileympearson]

Nothing yet .. I'll remove it for now. It can always be added back if there are items worth mentioning here.

[jyemin]

Not sure how we handle the date... Is there an automation for this?

[baileympearson]

Not that I know of. Usually the spec author fills it out before merging

I'll just leave this thread open to remind myself to add changelog dates before merging once all changes are completed.

[Jibola]

Suggested change

	##### Implementation notes
	#### Implementation notes

[jmikola]

This spec should also get a changelog entry.

[blink1073]

Done

[baileympearson]

submitting comments

[baileympearson]

Sorry, missed this one. I like your suggestion - done.

[baileympearson]

done

[baileympearson]

done

[baileympearson]

I chose to leave it as-is; as the pseudocode is written in python and follows python conventions. Let me know if the clarified pseudocde works for you

[baileympearson]

I removed the changelog

[baileympearson]

I've reworked the prose to be explicit about what the command is.

[matthewdale]

Looks good! 👍

[sanych-sun]

Why we decided to have retry number start we 0? It makes the requirements confusing. Why don't we start with 1 and in fact we will have the same formula as for withTransaction:

delayMS = j * min(maxBackoff, baseBackoff * 2^(i-1))

With the only difference here we have 2 as the base for pow function, in convinientTransaction API we have 1.5

[sanych-sun]

Here is the formula from withTransaction:
jitter * min(BACKOFF_INITIAL * 1.5 ** (transactionAttempt - 1), BACKOFF_MAX)

Where transactionAttempt started with 0 and is being incremented AFTER the delay, but before executing the callback attempt. Which is also confusing... but in C# implementation we wait AFTER the attempt so it's more natural.

[baileympearson]

The phrasing, including "Retries start at 0", was just taken from the design. There's no need to keep it this way if it causes confusion.

I can adjust the phrasing to more closely align with the transaction spec, if that's preferable?

[sanych-sun]

Can we use the following formula?
delayMS = j * min(maxBackoff, baseBackoff * 2^(i-1))

or

we can keep the formula as is, but adjust the baseBackoff :
delayMS = j * min(maxBackoff, baseBackoff * 2^i)
where baseBackoff is 50 instead of 100.

It produces the same results, but starts i with 1.

[Jibola]

I think it's fine to use any of those three formulas in individual driver implementations if it increases readability so long as they result in the same outputs. As far as reducing confusion, I can say it didn't substantially change my understanding of the formula.

+1 to Bailey changing the phrasing, which I think would suffice.

[stIncMale]

The 100ms comment is helpful, because one can use it to find the same value in the prose part of the specification (though absolutely nothing prevents us from simply writing BASE_BACKOFF = 100ms).

However, the 10s comment is not helpful, because the prose part says 10000ms, and a reader won't find 10000ms in the pseudocode.

Suggested change

	MAX_BACKOFF = 10 # 10s
	MAX_BACKOFF = 10 # 10000ms

[stIncMale]

Judging both from the name of MAX_ATTEMPTS, and this sentence, a reader develops understanding that MAX_ATTEMPTS is the maximum number of attempts, which, includes the first attempt that is not a retry attempt. However, below, the specification says "Any retryable error is retried at most MAX_ATTEMPTS (default=5) times", which means that MAX_ATTEMPTS is the maximum number of retry attempts, implying that the maximum number of attempts is actually 6.

If MAX_ATTEMPTS is meant to specify the maximum number of retry attempts, let's name it accordingly: MAX_RETRY_ATTEMPTS.

[stIncMale]

Item 4.1 requires the SystemOverloadedError label to be present. The highlighted item 4.4 restricts itself to SystemOverloadedError errors only, but no other errors can possibly be considered because of item 4.1¹. Given this, why does item 4.4 has this restriction?

[stIncMale]

Shouldn't these two sub-items about MAX_ATTEMPTS be under item 4.2, which talks about MAX_ATTEMPTS, rather than being under item 4.4, which talks about acquiring a token?

[stIncMale]

The specification currently uses various terms to refer to the same thing¹ when it talks about retry tokens (hopefully, I identified them all, but the author should do the exercise on his own to verify):

• return, deposit, consume
• deposit, return

We should pick a single term for each meaning and use it consistently. Having clear and consistent terminology is a necessity for clear communications, it also allows makes the specification searchable: if one wants to find all the places that talk about consuming retry tokens, one can search the page for a specific term. Currently, that is impossible.

---

¹ Our specifications in general, as well as the official documentation the company produces, suffer from this significant issue.

[stIncMale]

This sentence is the only place in the specification that suggests the specification defines "retryability API". Given that the specification does not define "retryability API", but it defines the "overload retry policy", which is what this sentence is talking about.

Interaction with Existing Retry Behavior
...
retryability behaviors

This specification already calls that behavior "policy", which means that the sentence should use the same term. It would have also been useful if we also named the existing policy, for example, the "read/write retry policy" (or, if we want to separate them, the "read retry policy" and the "write retry policy").

[stIncMale]

Let's make "retryable reads specification" and "retryable writes specification" hyperlinks.

[stIncMale]

Don't these two sentences say the same? If yes, let's leave only one of them.

[stIncMale]

Given that a command may be retried either because of the read/write retry policy, or because of the overload retry policy, what does the i mean in the formula for delayMS? Is it the 1-based retry attempt number among all the retry attempts of a command, or only among the subset caused by the overload retry policy?

The answer to this question should be in the "Interaction with Existing Retry Behavior" section, not in the "Overload retry policy" section.

[stIncMale]

The "RetryableError label" section above says "An error is considered retryable if it includes the "RetryableError" label". After reading that, it is only reasonable to understand that a "retryable errror" is an error with the RetryableError label. However, in the selected items the specification seemingly uses the same term "retryable error" with a different meaning. This needs to be fixed.

I don't think that sections "RetryableError label", "SystemOverloadedError label" defining the terms "retryable error", "overloaded error" help a reader, neither do the the terms. Instead I propose the specification to always talk about an error having the RetryableError, SystemOverloadedError labels, which it already does in many places.

#1862 (comment) is related, but different.

[stIncMale]

I fail to understand what this requirement means. For example, "Any retryable error is retried" is very confusing, since we are trying to execute a command, not trying to execute/produce an error. The current wording also implies that the retry attempts are counted per each specific error, which can't be true, as they are counter per, for example, an execution of a command.

Does the following express the intended meaning?

Once we encounter such a command execution error with the SystemOverloadedError label that it permits a retry attempt according to the overload retry policy, the number of retry attempts for the particular command execution becomes limited by MAX_ATTEMPTS regardless of which retry policy the previous or the future retry attempts are caused by.

If "yes", then let's change the wording.

Note that the above means a command may still be retried more than MAX_ATTEMPTS times in the presence of errors having the SystemOverloadedError label. For example, if a command was retried 6 times according to the read/write retry policy with CSOT, and the 6th retry attempt (it's a 1-based index) failed with an error having the SystemOverloadedError label.

[stIncMale]

crud/bulk-write.md also talks about a "retryable error", but means some thing very different from what "retryable error" means here.

1. Let's not overload this term by re-defining it with a different meaning here, as overloaded terms introduce confusion and have no benefits.
   1.1. If a term has to be overloaded nonetheless, which I doubt, let's at the very least make the changes to crud/bulk-write.md necessary to reduce confusion.
2. The term "retryable error" is also defined in transactions/transactions.md. Fortunately, the meaning there does not need to be clarified.

• DRIVERS-3239: Add exponential backoff to operation retry loop for server overloaded errors #1862 (comment) is related, but different.
• DRIVERS-3239: Add exponential backoff to operation retry loop for server overloaded errors #1862 (comment) is related, but different.

[stIncMale]

Intro

Our specifications, as well as the official documentation, do not use the terms "operation" and "command" clearly, and neither does the specification introduced by this PR: it uses the terms seemingly interchangeably, despite them having very different meanings¹:

• CSOT made an attempt to say what an operation is, but missed at least the client bulk write operation. Whatever "operation" is in our specifications, it is what identified by the operationId field in CommandStartedEvent, CommandSucceededEvent, CommandFailedEvent, and execution of an operation consists of executions of potentially multiple commands, as well as other potentially retryable activities like server selection.
• A command is whatever CommandStartedEvent, CommandSucceededEvent, CommandFailedEvent describe, and execution of a command consists of potentially multiple attempts.

The concern

The overload retry policy seems to be specified as applied at the command level: execution of any command consists of up to (MAX_ATTEMPTS + 1) attempts (and the "If a command fails backpressure retries MAX_ATTEMPTS times, it MUST not be retried again" sentence in transactions/transactions.md supports this interpretation). Given that execution of an operation may consist of many command executions, the above may result in a really large total number of retry attempts of different commands per execution of an operation. Is this indeed intended? I have doubts.

We can take a look at what execution of a client bulk write operation includes, as an example demonstrating how many executions of different commands may constitute execution of an operation:

• potentially connection establishment
  • execution of a hello command
  • potentially authentication (let's assume it's a SASL authentication)
    • execution of a saslStart command
    • execution of a saslContinue command
    • potentially more executions of a saslContinue command
• execution of a bulkWrite command
• execution of a getMore command
• potentially more executions of a getMore command
  • and/or potentially execution of a killCursors command
• potentially more executions of a bulkWrite command, if there are more batches
• potentially more executions of a getMore command, if there are more batches
• potentially reauthentication

I suspect, the retry attempts should be counted not per execution of a command, but somehow else. This, however, may further complicate already convoluted retry policies and implementations. Anyway,

1. the specification should clarify the scope within which retry attempts caused by the overload retry policy are limited by MAX_ATTEMPTS;
2. the specification should use the terms "operation", "command" such that they are appropriate and match the meaning outlined above, unless those outlined meanings are incorrect.

---

¹ I am aware that when trying to outline what an operation and a command are, as well as to clarify their executions, I am saying things that are not explicitly stated in the specifications. However, without introducing a vocabulary it is impossible to communicate, so I have to do that, since the specifications failed to (or I failed to find where they do that, despite spending significant amount of time).

[stIncMale]

The overload retry policy applies to "all commands", requiring drivers to inspect the result of a command attempt. On the other hand, the client bulk write specification says "The result returned from the killCursors command MAY be ignored.". Let's clarify whether the drivers must apply the overload retry policy to killCursors, or if execution of that command should never contain more than one attempt.

Previously @baileympearson said the following in Slack about this: "Operations that clean up resources could be retried but seem less important, because resources will be pruned by the server automatically after their timeout expires server-side.".

[stIncMale]

The client backpressure specification says that "an error is considered retryable if it includes the "RetryableError" label". So there, a command with the "RetryableError" label is called retryable error, but here, the same error is called "backpressure error". Not only we are proposed to have the "retryable error" term overloaded (see #1862 (comment)), but we are also proposed to have multiple terms ("retryable error", "backpressure error") referring to the same exact thing. We should fix this:

• we should use a single term to refer to the same thing;
• we should avoid overloading terms.

P.S. I won't be surprised if the intent here was to express that backpressure error is an error that has both the RetryableError and the SystemOverloadError labels. But that is definitely not what the specification currently expresses.

[stIncMale]

This specification defines the term "backpressure error", not "backpressure error labels". Shouldn't these sentences (there are two that use "backpressure error labels") just say something like "unless the command fails with a backpressure error", "unless the error is a backpressure error"?

[stIncMale]

"If a command fails with backpressure labels" - is it the same as saying that a command fails with a "backpressure error" defined above? If yes, then the specification should use that term.

[stIncMale]

Let's remove the "when the appropriate error labels are added by the server" part. This is just one of multiple conditions that must be met for a command to be retried according to the overload retry policy, and it is weird to mention one of them, while avoiding the rest (I am not proposing to reiterate all of them here).

Suggested change

	All commands in a transaction are subject to the
	[Client Backpressure Specification](../client-backpressure/client-backpressure.md), and MUST be retried accordingly when
	the appropriate error labels are added by the server. This includes the initial command with `startTransaction:true`,
	All commands in a transaction are subject to the
	[Client Backpressure Specification](../client-backpressure/client-backpressure.md), and MUST be retried accordingly. This includes the initial command with `startTransaction:true`,

[stIncMale]

I don't think we know whether the transaction has or has not been applied. If, for example, in the scenario below step 4 fails with a backpressure error, it is obviously false that the transaction has not been applied.

I think, this change should be reverted.

[stIncMale]

Notes for myself:

• The last reviewed commit is 1b7f6df.
• I reviewed only the changes in .md files, and do not plan to review the tests.
• I have not re-reviewed the pseudocode illustrating the overload retry policy. I will do that after us settling on the requirements expressed in the prose part of the specification.