-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(NODE-3769): retryable writes are not compliant with specification #3144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
69d655d
to
9d7894b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial feedback so that I don't lose it, still got the main files left to go through
test/integration/retryable-writes/retryable_writes.spec.test.ts
Outdated
Show resolved
Hide resolved
test/integration/retryable-writes/retryable_writes.spec.test.ts
Outdated
Show resolved
Hide resolved
9c19368
to
23fc205
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some partial feedback on the revisions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the one additional comment here
65232e8
to
39b0636
Compare
abc766f
to
6984fca
Compare
6984fca
to
d43abe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting an approve because these are minor:
Description
Fix NODE-3769, test NODE-2111
What is changing?
MongoUnexpectedServerResponseError
which is a subclass ofMongoRuntimeError
I could continue usingMongoRuntimeError
to avoid adding a new class, but the refactor/fix to execute operation called for it.MONGODB_ERROR_LABELS
, This allows us to type check our labels, (mostly could catch future spelling mistakes if we miss actual test coverage) felt like it was still relevant to this work to keep in.clearAndRemoveTimerFrom
function. entirely internal, never reached.connectServers
called only from one place and simply created a level of indirection that is frustrating to debugisRetryableEndTransactionError
, it had one condition, checking for the retryWriteLabel, inlining the condition has better readability, there could be reason to bring it back if there is more conditions.supportsRetryableWrites
and made it accept an optional server (removes an existence gate from a caller if stmt)async
-ifytestOperations
in the legacy spec runner, debugging.global.d.ts
, it doesn't run automagically currently 😿What is the motivation for this change?
Our retryable logic was falling back on error codes, the spec explicitly states that on newer server versions the label should be the ONLY source of truth. So we were still retrying errors, and we will continue to do so, but the rules were incorrect.
Double check the following
npm run check:lint
script<type>(NODE-xxxx)<!>: <description>
commandHandler
ofendTransaction
is it okay that we only check the retry read condition before unpinning / adding the unknown commit label? I don't think so, but also the retry has already occurred at this stage, so I don't think the session gets reused, unpinning might be correct.