Skip to content
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

GODRIVER-3032 Don't retry writes on pre-4.4 mongos #1915

Merged
merged 2 commits into from
Jan 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions testdata/retryable-writes/unified/insertOne-serverErrors.json
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,99 @@
}
]
},
{
"description": "RetryableWriteError label is not added based on writeConcernError in pre-4.4 mongos response",
"runOnRequirements": [
{
"minServerVersion": "4.2",
"maxServerVersion": "4.2.99",
"topologies": [
"sharded"
]
}
],
"operations": [
{
"name": "failPoint",
"object": "testRunner",
"arguments": {
"client": "client0",
"failPoint": {
"configureFailPoint": "failCommand",
"mode": {
"times": 1
},
"data": {
"failCommands": [
"insert"
],
"writeConcernError": {
"code": 91,
"errmsg": "Replication is being shut down"
}
}
}
}
},
{
"name": "insertOne",
"object": "collection0",
"arguments": {
"document": {
"_id": 3,
"x": 33
}
},
"expectError": {
"errorLabelsOmit": [
"RetryableWriteError"
]
}
}
],
"expectEvents": [
{
"client": "client0",
"events": [
{
"commandStartedEvent": {
"command": {
"insert": "coll",
"documents": [
{
"_id": 3,
"x": 33
}
]
},
"commandName": "insert",
"databaseName": "retryable-writes-tests"
}
}
]
}
],
"outcome": [
{
"collectionName": "coll",
"databaseName": "retryable-writes-tests",
"documents": [
{
"_id": 1,
"x": 11
},
{
"_id": 2,
"x": 22
},
{
"_id": 3,
"x": 33
}
]
}
]
},
{
"description": "InsertOne succeeds after connection failure",
"operations": [
Expand Down
38 changes: 38 additions & 0 deletions testdata/retryable-writes/unified/insertOne-serverErrors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,44 @@ tests:
# writeConcernError doesn't prevent the server from applying the write
- { _id: 3, x: 33 }

- description: "RetryableWriteError label is not added based on writeConcernError in pre-4.4 mongos response"
runOnRequirements:
- minServerVersion: "4.2"
maxServerVersion: "4.2.99"
topologies: [ sharded ]
operations:
- name: failPoint
object: testRunner
arguments:
client: *client0
failPoint:
configureFailPoint: failCommand
# Trigger the fail point only once since a RetryableWriteError label
# will not be added and the write will not be retried.
mode: { times: 1 }
data:
failCommands: [ "insert" ]
writeConcernError:
code: 91 # ShutdownInProgress
errmsg: "Replication is being shut down"
- name: insertOne
object: *collection0
arguments:
document: { _id: 3, x: 33 }
expectError:
errorLabelsOmit: [ "RetryableWriteError" ]
expectEvents:
- client: *client0
events:
- commandStartedEvent: *insertCommandStartedEvent
outcome:
- collectionName: *collectionName
databaseName: *databaseName
documents:
- { _id: 1, x: 11 }
- { _id: 2, x: 22 }
# writeConcernError doesn't prevent the server from applying the write
- { _id: 3, x: 33 }
-
description: 'InsertOne succeeds after connection failure'
operations:
Expand Down
13 changes: 10 additions & 3 deletions x/mongo/driver/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (wce WriteCommandError) Error() string {
}

// Retryable returns true if the error is retryable
func (wce WriteCommandError) Retryable(wireVersion *description.VersionRange) bool {
func (wce WriteCommandError) Retryable(serverKind description.ServerKind, wireVersion *description.VersionRange) bool {
for _, label := range wce.Labels {
if label == RetryableWriteError {
return true
Expand All @@ -153,7 +153,7 @@ func (wce WriteCommandError) Retryable(wireVersion *description.VersionRange) bo
if wce.WriteConcernError == nil {
return false
}
return wce.WriteConcernError.Retryable()
return wce.WriteConcernError.Retryable(serverKind, wireVersion)
}

// HasErrorLabel returns true if the error contains the specified label.
Expand Down Expand Up @@ -186,7 +186,14 @@ func (wce WriteConcernError) Error() string {
}

// Retryable returns true if the error is retryable
func (wce WriteConcernError) Retryable() bool {
func (wce WriteConcernError) Retryable(serverKind description.ServerKind, wireVersion *description.VersionRange) bool {
if serverKind == description.ServerKindMongos && wireVersion.Max < 9 {
// For a pre-4.4 mongos response, we can trust that mongos will have already
// retried the operation if necessary. Drivers should not retry to avoid
// "excessive retrying".
return false
}

for _, code := range retryableCodes {
if wce.Code == int64(code) {
return true
Expand Down
2 changes: 1 addition & 1 deletion x/mongo/driver/operation.go
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,7 @@ func (op Operation) Execute(ctx context.Context) error {
}

connDesc := conn.Description()
retryableErr := tt.Retryable(connDesc.WireVersion)
retryableErr := tt.Retryable(connDesc.Kind, connDesc.WireVersion)
preRetryWriteLabelVersion := connDesc.WireVersion != nil && connDesc.WireVersion.Max < 9
inTransaction := op.Client != nil &&
!(op.Client.Committing || op.Client.Aborting) && op.Client.TransactionRunning()
Expand Down
Loading