Skip to content

Commit b7bf0e0

Browse files
author
Isabella Siu
authored
GODRIVER-1603 don't add RetryableWriteError label in transactions (#434)
1 parent 9272366 commit b7bf0e0

File tree

3 files changed

+182
-11
lines changed

3 files changed

+182
-11
lines changed

‎data/transactions/error-labels.json

Lines changed: 105 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@
134134
"TransientTransactionError"
135135
],
136136
"errorLabelsOmit": [
137+
"RetryableWriteError",
137138
"UnknownTransactionCommitResult"
138139
]
139140
}
@@ -223,6 +224,7 @@
223224
"TransientTransactionError"
224225
],
225226
"errorLabelsOmit": [
227+
"RetryableWriteError",
226228
"UnknownTransactionCommitResult"
227229
]
228230
}
@@ -312,6 +314,7 @@
312314
"TransientTransactionError"
313315
],
314316
"errorLabelsOmit": [
317+
"RetryableWriteError",
315318
"UnknownTransactionCommitResult"
316319
]
317320
}
@@ -462,7 +465,7 @@
462465
}
463466
},
464467
{
465-
"description": "add TransientTransactionError label to connection errors",
468+
"description": "add TransientTransactionError label to connection errors, but do not add RetryableWriteError label",
466469
"failPoint": {
467470
"configureFailPoint": "failCommand",
468471
"mode": {
@@ -497,6 +500,7 @@
497500
"TransientTransactionError"
498501
],
499502
"errorLabelsOmit": [
503+
"RetryableWriteError",
500504
"UnknownTransactionCommitResult"
501505
]
502506
}
@@ -512,6 +516,7 @@
512516
"TransientTransactionError"
513517
],
514518
"errorLabelsOmit": [
519+
"RetryableWriteError",
515520
"UnknownTransactionCommitResult"
516521
]
517522
}
@@ -534,6 +539,7 @@
534539
"TransientTransactionError"
535540
],
536541
"errorLabelsOmit": [
542+
"RetryableWriteError",
537543
"UnknownTransactionCommitResult"
538544
]
539545
}
@@ -550,6 +556,7 @@
550556
"TransientTransactionError"
551557
],
552558
"errorLabelsOmit": [
559+
"RetryableWriteError",
553560
"UnknownTransactionCommitResult"
554561
]
555562
}
@@ -1098,6 +1105,103 @@
10981105
}
10991106
}
11001107
},
1108+
{
1109+
"description": "do not add RetryableWriteError label to writeConcernError ShutdownInProgress that occurs within transaction",
1110+
"failPoint": {
1111+
"configureFailPoint": "failCommand",
1112+
"mode": {
1113+
"times": 1
1114+
},
1115+
"data": {
1116+
"failCommands": [
1117+
"insert"
1118+
],
1119+
"writeConcernError": {
1120+
"code": 91,
1121+
"errmsg": "Replication is being shut down"
1122+
}
1123+
}
1124+
},
1125+
"operations": [
1126+
{
1127+
"name": "startTransaction",
1128+
"object": "session0",
1129+
"arguments": {
1130+
"options": {
1131+
"writeConcern": {
1132+
"w": "majority"
1133+
}
1134+
}
1135+
}
1136+
},
1137+
{
1138+
"name": "insertOne",
1139+
"object": "collection",
1140+
"arguments": {
1141+
"session": "session0",
1142+
"document": {
1143+
"_id": 1
1144+
}
1145+
},
1146+
"result": {
1147+
"errorLabelsContain": [],
1148+
"errorLabelsOmit": [
1149+
"RetryableWriteError",
1150+
"TransientTransactionError",
1151+
"UnknownTransactionCommitResult"
1152+
]
1153+
}
1154+
},
1155+
{
1156+
"name": "abortTransaction",
1157+
"object": "session0"
1158+
}
1159+
],
1160+
"expectations": [
1161+
{
1162+
"command_started_event": {
1163+
"command": {
1164+
"insert": "test",
1165+
"documents": [
1166+
{
1167+
"_id": 1
1168+
}
1169+
],
1170+
"ordered": true,
1171+
"readConcern": null,
1172+
"lsid": "session0",
1173+
"txnNumber": {
1174+
"$numberLong": "1"
1175+
},
1176+
"startTransaction": true,
1177+
"autocommit": false
1178+
},
1179+
"command_name": "insert",
1180+
"database_name": "transaction-tests"
1181+
}
1182+
},
1183+
{
1184+
"command_started_event": {
1185+
"command": {
1186+
"abortTransaction": 1,
1187+
"lsid": "session0",
1188+
"txnNumber": {
1189+
"$numberLong": "1"
1190+
},
1191+
"startTransaction": null,
1192+
"autocommit": false
1193+
},
1194+
"command_name": "abortTransaction",
1195+
"database_name": "admin"
1196+
}
1197+
}
1198+
],
1199+
"outcome": {
1200+
"collection": {
1201+
"data": []
1202+
}
1203+
}
1204+
},
11011205
{
11021206
"description": "add UnknownTransactionCommitResult label to writeConcernError WriteConcernFailed",
11031207
"failPoint": {

‎data/transactions/error-labels.yml

Lines changed: 68 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ tests:
8686
result:
8787
# Note, the server will return the errorLabel in this case.
8888
errorLabelsContain: ["TransientTransactionError"]
89-
errorLabelsOmit: ["UnknownTransactionCommitResult"]
89+
errorLabelsOmit: ["RetryableWriteError", "UnknownTransactionCommitResult"]
9090
- name: abortTransaction
9191
object: session0
9292

@@ -143,7 +143,7 @@ tests:
143143
result:
144144
# Note, the server will return the errorLabel in this case.
145145
errorLabelsContain: ["TransientTransactionError"]
146-
errorLabelsOmit: ["UnknownTransactionCommitResult"]
146+
errorLabelsOmit: ["RetryableWriteError", "UnknownTransactionCommitResult"]
147147
- name: abortTransaction
148148
object: session0
149149

@@ -200,7 +200,7 @@ tests:
200200
result:
201201
# Note, the server will return the errorLabel in this case.
202202
errorLabelsContain: ["TransientTransactionError"]
203-
errorLabelsOmit: ["UnknownTransactionCommitResult"]
203+
errorLabelsOmit: ["RetryableWriteError", "UnknownTransactionCommitResult"]
204204
- name: abortTransaction
205205
object: session0
206206

@@ -295,7 +295,7 @@ tests:
295295
collection:
296296
data: []
297297

298-
- description: add TransientTransactionError label to connection errors
298+
- description: add TransientTransactionError label to connection errors, but do not add RetryableWriteError label
299299

300300
failPoint:
301301
configureFailPoint: failCommand
@@ -315,7 +315,10 @@ tests:
315315
_id: 1
316316
result: &transient_label_only
317317
errorLabelsContain: ["TransientTransactionError"]
318-
errorLabelsOmit: ["UnknownTransactionCommitResult"]
318+
# While a connection error would normally be retryable, these are not because
319+
# they occur within a transaction; ensure the driver does not add the
320+
# RetryableWriteError label to these errors.
321+
errorLabelsOmit: ["RetryableWriteError", "UnknownTransactionCommitResult"]
319322
- name: find
320323
object: collection
321324
arguments:
@@ -670,6 +673,66 @@ tests:
670673
data:
671674
- _id: 1
672675

676+
- description: do not add RetryableWriteError label to writeConcernError ShutdownInProgress that occurs within transaction
677+
678+
failPoint:
679+
configureFailPoint: failCommand
680+
mode: { times: 1 }
681+
data:
682+
failCommands: ["insert"]
683+
writeConcernError:
684+
code: 91
685+
errmsg: Replication is being shut down
686+
687+
operations:
688+
- name: startTransaction
689+
object: session0
690+
arguments:
691+
options:
692+
writeConcern:
693+
w: majority
694+
- name: insertOne
695+
object: collection
696+
arguments:
697+
session: session0
698+
document:
699+
_id: 1
700+
result:
701+
errorLabelsContain: []
702+
errorLabelsOmit: ["RetryableWriteError", "TransientTransactionError", "UnknownTransactionCommitResult"]
703+
- name: abortTransaction
704+
object: session0
705+
706+
expectations:
707+
- command_started_event:
708+
command:
709+
insert: *collection_name
710+
documents:
711+
- _id: 1
712+
ordered: true
713+
readConcern:
714+
lsid: session0
715+
txnNumber:
716+
$numberLong: "1"
717+
startTransaction: true
718+
autocommit: false
719+
command_name: insert
720+
database_name: *database_name
721+
- command_started_event:
722+
command:
723+
abortTransaction: 1
724+
lsid: session0
725+
txnNumber:
726+
$numberLong: "1"
727+
startTransaction:
728+
autocommit: false
729+
command_name: abortTransaction
730+
database_name: admin
731+
732+
outcome:
733+
collection:
734+
data: []
735+
673736
- description: add UnknownTransactionCommitResult label to writeConcernError WriteConcernFailed
674737

675738
failPoint:

‎x/mongo/driver/operation.go

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -387,9 +387,10 @@ func (op Operation) Execute(ctx context.Context, scratch []byte) error {
387387
connDesc := conn.Description()
388388
retryableErr := tt.Retryable(connDesc.WireVersion)
389389
preRetryWriteLabelVersion := connDesc.WireVersion != nil && connDesc.WireVersion.Max < 9
390-
391-
// If retry is enabled, add a RetryableWriteError label for retryable errors from pre-4.4 servers
392-
if retryableErr && preRetryWriteLabelVersion && retryEnabled {
390+
inTransaction := !(op.Client.Committing || op.Client.Aborting) && op.Client.TransactionRunning()
391+
// If retry is enabled and the operation isn't in a transaction, add a RetryableWriteError label for
392+
// retryable errors from pre-4.4 servers
393+
if retryableErr && preRetryWriteLabelVersion && retryEnabled && !inTransaction {
393394
tt.Labels = append(tt.Labels, RetryableWriteError)
394395
}
395396

@@ -462,8 +463,11 @@ func (op Operation) Execute(ctx context.Context, scratch []byte) error {
462463
if op.Type == Write {
463464
retryableErr = tt.RetryableWrite(connDesc.WireVersion)
464465
preRetryWriteLabelVersion := connDesc.WireVersion != nil && connDesc.WireVersion.Max < 9
465-
// If retryWrites is enabled, add a RetryableWriteError label for network errors and retryable errors from pre-4.4 servers
466-
if retryEnabled && (tt.HasErrorLabel(NetworkError) || (retryableErr && preRetryWriteLabelVersion)) {
466+
inTransaction := !(op.Client.Committing || op.Client.Aborting) && op.Client.TransactionRunning()
467+
// If retryWrites is enabled and the operation isn't in a transaction, add a RetryableWriteError label
468+
// for network errors and retryable errors from pre-4.4 servers
469+
if retryEnabled && !inTransaction &&
470+
(tt.HasErrorLabel(NetworkError) || (retryableErr && preRetryWriteLabelVersion)) {
467471
tt.Labels = append(tt.Labels, RetryableWriteError)
468472
}
469473
} else {

0 commit comments

Comments
 (0)