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

fix(NODE-5720): on pre-4.4 sharded servers, the node driver uses error.writeConcern.code to determine retryability #4155

Merged
merged 24 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6fd82f8
sync spec tests - no other changes needed
aditi-khare-mongoDB Jun 17, 2024
242c12d
remove stray only
aditi-khare-mongoDB Jun 17, 2024
a8f67d6
sharded <4.4 support
aditi-khare-mongoDB Jun 18, 2024
41e8587
remove typo
aditi-khare-mongoDB Jun 18, 2024
96b5c95
bugs fixed?
aditi-khare-mongoDB Jun 18, 2024
dcef868
remove stray only
aditi-khare-mongoDB Jun 18, 2024
f468ae4
skip outdated tests
aditi-khare-mongoDB Jun 20, 2024
e205a77
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jun 20, 2024
b22466e
skip tests
aditi-khare-mongoDB Jun 28, 2024
9eb063e
remove stray only
aditi-khare-mongoDB Jun 28, 2024
231fb80
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jun 28, 2024
e7037e9
skip tests with comment
aditi-khare-mongoDB Jul 9, 2024
440bac7
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 9, 2024
ad5b150
remove stray only
aditi-khare-mongoDB Jul 9, 2024
60ad9ac
fix logic
aditi-khare-mongoDB Jul 10, 2024
a7b760d
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 10, 2024
985dc03
changes to consider top-level code, doesnt pass tests
aditi-khare-mongoDB Jul 11, 2024
ac247a7
delete extraneous folder
aditi-khare-mongoDB Jul 11, 2024
10578d2
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 19, 2024
021e118
lint fix
aditi-khare-mongoDB Jul 19, 2024
fca53fe
Merge branch 'main' into NODE-5720/clarify-retryable-fields
aditi-khare-mongoDB Jul 29, 2024
7a1964b
post node-6276 update
aditi-khare-mongoDB Jul 29, 2024
a0bdcf4
post node-6276 update 2
aditi-khare-mongoDB Jul 29, 2024
5f8020e
requested changes
aditi-khare-mongoDB Jul 29, 2024
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
2 changes: 1 addition & 1 deletion src/cmap/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export async function performInitialHandshake(
} catch (error) {
if (error instanceof MongoError) {
error.addErrorLabel(MongoErrorLabel.HandshakeError);
if (needsRetryableWriteLabel(error, response.maxWireVersion)) {
if (needsRetryableWriteLabel(error, response.maxWireVersion, conn.description.type)) {
error.addErrorLabel(MongoErrorLabel.RetryableWriteError);
}
}
Expand Down
19 changes: 15 additions & 4 deletions src/error.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Document } from './bson';
import type { ServerType } from './sdam/common';
import type { TopologyVersion } from './sdam/server_description';
import type { TopologyDescription } from './sdam/topology_description';

Expand Down Expand Up @@ -1226,7 +1227,11 @@ const RETRYABLE_READ_ERROR_CODES = new Set<number>([
// see: https://github.com/mongodb/specifications/blob/master/source/retryable-writes/retryable-writes.rst#terms
const RETRYABLE_WRITE_ERROR_CODES = RETRYABLE_READ_ERROR_CODES;

export function needsRetryableWriteLabel(error: Error, maxWireVersion: number): boolean {
export function needsRetryableWriteLabel(
error: Error,
maxWireVersion: number,
serverType: ServerType
): boolean {
// pre-4.4 server, then the driver adds an error label for every valid case
// execute operation will only inspect the label, code/message logic is handled here
if (error instanceof MongoNetworkError) {
Expand All @@ -1246,11 +1251,17 @@ export function needsRetryableWriteLabel(error: Error, maxWireVersion: number):
}

if (error instanceof MongoWriteConcernError) {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
return RETRYABLE_WRITE_ERROR_CODES.has(error.result.writeConcernError.code ?? error?.code ?? 0);
if (serverType === 'Mongos' && maxWireVersion < 9) {
// use original top-level code from server response
return RETRYABLE_WRITE_ERROR_CODES.has(error.result.code ?? 0);
}
return RETRYABLE_WRITE_ERROR_CODES.has(
error.result.writeConcernError.code ?? Number(error.code) ?? 0
);
}

if (error instanceof MongoError && typeof error.code === 'number') {
return RETRYABLE_WRITE_ERROR_CODES.has(error.code);
if (error instanceof MongoError) {
return RETRYABLE_WRITE_ERROR_CODES.has(Number(error.code));
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
}

const isNotWritablePrimaryError = LEGACY_NOT_WRITABLE_PRIMARY_ERROR_MESSAGE.test(error.message);
Expand Down
2 changes: 1 addition & 1 deletion src/sdam/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ export class Server extends TypedEventEmitter<ServerEvents> {
} else {
if (
(isRetryableWritesEnabled(this.topology) || isTransactionCommand(cmd)) &&
needsRetryableWriteLabel(error, maxWireVersion(this)) &&
needsRetryableWriteLabel(error, maxWireVersion(this), this.description.type) &&
!inActiveTransaction(session, cmd)
) {
error.addErrorLabel(MongoErrorLabel.RetryableWriteError);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ describe('Retryable Writes (unified)', function () {
runUnifiedSuite(loadSpecTests(path.join('retryable-writes', 'unified')), ({ description }) => {
return clientBulkWriteTests.includes(description)
? `TODO(NODE-6257): implement client-level bulk write.`
: description ===
'RetryableWriteError label is not added based on writeConcernError in pre-4.4 mongos response'
? 'TODO(NODE-5720)'
: false;
});
});