GODRIVER-3399: PoolClearedError should have TransientTransactionError label appended to it #2114
Conversation
mongodb-drivers-pr-bot
bot
added
the
review-priority-low
API Change ReportNo changes found! |
zhouselena
force-pushed
the
GODRIVER-3399
branch
from
b8b5aa9 to
f4fc9f4
Compare
x/mongo/driver/topology/pool.go
Outdated
| wrappedErr := fmt.Errorf( | ||
| "%v: connection pool for %v was cleared because another operation failed with: %v %w", | ||
| driver.TransientTransactionError, pce.address, pce.err, pce) | ||
| return wrappedErr.Error() |
There was a problem hiding this comment.
Error "labels" in this context means that the error type must satisfy the LabeledError interface and have a particular label string. See example code that checks if an error has the "TransientTransactionError" label here.
There are a few ways to accomplish that:
- Wrap the
poolClearedErrorwith adriver.Errorand add the "TransientTransactionError" label to the labels list. - Add a
HasErrorLabel(string) boolmethod to thepoolClearedErrortype and return true if the arg is "TransientTransactionError".
Option 2 is more complex, and it's not clear how we'd determine if a transaction is running when the poolClearedError is created. Option 1 is already used when handling server selection errors while running a transaction (see here), so I recommend that approach.
In the above example, the "TransientTransactionError" label is always added if selectServer returns an error while a transaction is running. However, some errors that server.Connection returns should not have the "TransientTransactionError" label applied. You'll need to check if the returned error is a poolClearedError before adding the "TransientTransactionError" label.
There was a problem hiding this comment.
@matthewdale I fixed it to wrap the driver.Error and updated the test accordingly! however, I noticed that PoolClearedError is created here as well and passed through a wantConn trydeliver. I'm not super familiar with what exactly wantConn does, but it is a poolClearedError that is being passed through so I was wondering if it also needs to be labelled there?
| pcErr := poolClearedError{err: p.lastClearErr, address: p.address} | ||
| err := driver.Error{ | ||
| Message: pcErr.Error(), | ||
| Labels: []string{driver.TransientTransactionError}, | ||
| Wrapped: pcErr, |
There was a problem hiding this comment.
Technically this will add the "TransientTransactionError" to all poolClearedErrors, independent of if they happen during a transaction or not. Considering the spec is fuzzy on the exact expected behavior, I think the current code is good, but it's worth leaving a comment that describes the deviation from other cases where we add "TransientTransactionError".
E.g. recommended comment:
// Wrap poolCleared in a driver.Error so we can add the
// "TransientTransactionError" label. This will add
// "TransientTransactionError" to all poolClearedError instances, not
// just those that happened during transactions. While that behavior is
// different than other places we add "TransientTransactionError", it is
// consistent with the Transactions specification and simplifies the
// code.
|
re-requested review for updated comment 😄 |
… label appended to it (mongodb#2114)
* master: (65 commits)
Replace all uses of 'interface{}' with 'any' in the bson package. (#2138)
GODRIVER-3473 Short-cicruit cursor.next() on invalid timeouts (#2135)
GODRIVER-3622 Automatically retry some test tasks. (#2147)
Replace all uses of 'interface{}' with 'any' in the repo docs. (#2142)
GODRIVER-3102: Perf comparison (#2134)
GODRIVER-3587 Use raw bytes in valueReader (#2120)
Replace all uses of 'interface{}' with 'any' in the internal/ packages. (#2140)
Replace all uses of 'interface{}' with 'any' in the x/ packages. (#2137)
GODRIVER-2016 Unskip all Transactions unified spec tests. (#2132)
GODRIVER-2540 Run govulncheck in CI builds. (#2136)
GODRIVER-3549 Update Client BulkWrite prose tests. (#2131)
Add guidelines for contributing features to the Go Driver (#2125)
Bump alcaeus/automatic-merge-up-action from 1.0.0 to 1.0.1 in the actions group (#2126)
Add wrappedMsgOnly to mongo.MarshalError and mongo.MongocryptError.
Bump testdata/specifications from `db69351` to `6689929`
fix wiremessage oob in case of intmin (#2076)
GODRIVER-3399: PoolClearedError should have TransientTransactionError label appended to it (#2114)
PR feedback.
Prevent integration tests from running when testing with -short
Skip AWS Test if no URI (#2102)
...
GODRIVER-3399
Summary
Add
TransientTransactionErrorlabel to `PoolClearedError and created unit test accordingly.Background & Motivation
(From DRIVERS-1815) In order for withTransaction to retry PoolClearedErrors, they need to have the TransientTransactionError label appended to them by the driver, similar to server selection errors.