DO NOT MERGE: Review discussion points for NODE-7122 #4818
Conversation
|
|
||
| // 6.ii (cont.) If elapsed time + backoffMS > TIMEOUT_MS, then raise last known error | ||
| // DP: the lines above do this math slightly indirectly, but this break does NOT raise the last known error | ||
| // because breaking the while loop takes us to the return result statement outside the while(!committed) |
There was a problem hiding this comment.
actually, to the top of the for retry loop (but with the same effect re: last known error)
| } | ||
|
|
||
| // 6.ii (cont.) Otherwise, sleep for backoffMS, increment retry, and jump back to step two. | ||
| // DP: Assuming the spec means step 8 |
There was a problem hiding this comment.
if the spec means 2, this is correct
|
|
||
| // 9.ii If the commitTransaction error includes a "TransientTransactionError" label | ||
| // and the elapsed time of withTransaction is less than TIMEOUT_MS, jump back to step two. | ||
| // DP: Step two makes no sense here, perhaps the intent is to perform the instructions in 6.ii and then jump back to 8? |
There was a problem hiding this comment.
if my comment is correct, then this is ok here, but needs to also be in 6.ii
|
|
||
| // 1.i Record the current monotonic time, which will be used to enforce the 120-second / CSOT timeout before later retry attempts. | ||
| const startTime = this.timeoutContext?.csotEnabled() ? this.timeoutContext.start : now(); | ||
| // DP: the CSOT check is redundant, because of the definition in L725 |
There was a problem hiding this comment.
Durran brought this up a while ago - the timeoutContext abstraction needs a bit of work. start is only defined on the CSOT timeout context, so we need to narrow. Otherwise, we'd get a TS error:
| // 6. If the callback reported an error: | ||
| } catch (fnError) { | ||
| // DP: The preemptive abort isn't spec; this !MongoError would be an error thrown by the callback. | ||
| // DP: Is it safe to assume that the callback hasn't committed the transaction before throwing? |
There was a problem hiding this comment.
// DP: The preemptive abort isn't spec; this !MongoError would be an error thrown by the callback.
No context here
// DP: Is it safe to assume that the callback hasn't committed the transaction before throwing?
Yes - a withTransaction callback should never commit a transaction (we commit the transaction for users).
There was a problem hiding this comment.
When I say the preemptive abort is not spec, I mean the spec says to go through steps 6.i-6.iii before throwing
baileympearson
force-pushed
the
NODE-7122
branch
from
87ff29e to
0668f2a
Compare
Description
Summary of Changes
Notes for Reviewers
What is the motivation for this change?
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript