DRIVERS-2501 Break NoWritesPerformed-Only Error Sequence #1349
Conversation
prestonvasquez
requested review from
jmikola and
ShaneHarvey
and removed request for
a team
|
@juliusgeo is going to test this in: mongodb/mongo-python-driver#1117 |
source/retryable-writes/tests/unified/insertOne-noWritesPerformedError.json
Outdated
Show resolved
Hide resolved
| @@ -405,7 +405,8 @@ If a retry attempt also fails, drivers MUST update their topology according to | |||
| the SDAM spec (see: `Error Handling`_). If an error would not allow the caller | |||
| to infer that an attempt was made (e.g. connection pool exception originating | |||
| from the driver) or the error is labeled "NoWritesPerformed", the error from | |||
| the previous attempt should be raised. | |||
| the previous attempt should be raised. If all server errors are labeled | |||
| "NoWritesPerformed", then the first error should be raised. | |||
There was a problem hiding this comment.
If all server errors are labeled "NoWritesPerformed", then the first error should be raised.
Is previousError in the code below intended to be the first error encountered? If not, and previousError could be updated multiple times, the behavior below seems to conflict with this statement as the first error is never permanently saved.
There was a problem hiding this comment.
Assuming that an error occurs, the idea is to always set "previousError" with the first error encountered, without condition. Otherwise, if the error is NOT labeled "NoWritesPerformed", then update the "previousError" to the "originalError":
if (originalError is not DriverException && ! originalError.hasErrorLabel("NoWritesPerformed") ||
previousError == null) {
previousError = originalError;
}
This behavior will "save" the first error only on the condition that the sequence of errors encountered are all labeled "NoWritesPerformed". Otherwise, a sequence of errors all labeled "NoWritesPerformed" will result in a "null" error.
| @@ -494,7 +506,7 @@ The above rules are implemented in the following pseudo-code: | |||
| * is very rare, and likely means that the cluster is in the midst of a | |||
| * downgrade. */ | |||
| if ( ! isRetryableWritesSupported(server)) { | |||
| throw originalError; | |||
| throw currentError; | |||
There was a problem hiding this comment.
I beleive this should be previousError too.
There was a problem hiding this comment.
(adding another comment because I accidentally approved.)
There was a problem hiding this comment.
Originally added in SPEC-972 , the comment for this block says
If the server selected for retrying is too old, throw the original error
What is the purpose for changing the logic here?
There was a problem hiding this comment.
Using currentError here is no longer correct when there are multiple retries. Imagine:
- Attempt 1 fails with NoWritesPerformed
- Attempt 2 fails with ShutdownInProgress
- Attempt 3 fails with NoWritesPerformed
- Attempt 4 selects a server that doesn't support retryable writes
We have to raise the previous non-NoWritesPerformed error which would be the ShutdownInProgress from attempt 2, not the one from attempt 3.
| @@ -494,7 +506,7 @@ The above rules are implemented in the following pseudo-code: | |||
| * is very rare, and likely means that the cluster is in the midst of a | |||
| * downgrade. */ | |||
| if ( ! isRetryableWritesSupported(server)) { | |||
| throw originalError; | |||
| throw currentError; | |||
There was a problem hiding this comment.
(adding another comment because I accidentally approved.)
DRIVERS-2501
Please complete the following before merging: