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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 18 commits into from

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Jun 17, 2024 •
edited
Loading

Description

On pre-4.4 Mongosservers, no longer add a RetryableWriteLabel to WriteConcernError due to its code property.

What is changing?

Sync some retryable write spec tests and adhere to spec guidelines.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

Spec compliance.

Release Highlight

On pre-4.4 sharded servers, the node driver uses the 'writeConcernError.code` field determine retryability

This fix was made to comply with spec guidelines.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review June 17, 2024 21:20
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as draft June 18, 2024 14:44
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-5720/clarify-retryable-fields branch 2 times, most recently from fb7495d to dcef868 Compare June 20, 2024 19:06
@baileympearson baileympearson self-requested a review June 28, 2024 20:02
@baileympearson baileympearson self-assigned this Jun 28, 2024
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 28, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review July 1, 2024 14:03
baileympearson
baileympearson requested changes Jul 1, 2024
View reviewed changes
src/error.ts Outdated Show resolved Hide resolved
test/integration/retryable-writes/retryable_writes.spec.test.ts Outdated Show resolved Hide resolved
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title test(NODE-5720): sync retryable write spec tests feat(NODE-5720): Clarify which tests are retryable on <4.4 Mongos and Mongod servers Jul 9, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title feat(NODE-5720): Clarify which tests are retryable on <4.4 Mongos and Mongod servers fix(NODE-5720): on pre-4.4 Mongos servers, the node driver uses WriteConcernError.code to determine retryability Jul 9, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5720): on pre-4.4 Mongos servers, the node driver uses WriteConcernError.code to determine retryability fix(NODE-5720): on pre-4.4 Mongos servers, the node driver uses WriteConcernError.code to add RetryableWriteLabel Jul 9, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5720): on pre-4.4 Mongos servers, the node driver uses WriteConcernError.code to add RetryableWriteLabel fix(NODE-5720): on pre-4.4 Mongos servers, the node driver uses WriteConcernError.code to determine retryability Jul 9, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-5720/clarify-retryable-fields branch from 25a4196 to 231fb80 Compare July 9, 2024 19:10
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5720): on pre-4.4 Mongos servers, the node driver uses WriteConcernError.code to determine retryability fix(NODE-5720): on pre-4.4 sharded servers, the node driver uses WriteConcernError.code to determine retryability Jul 9, 2024
@aditi-khare-mongoDB
remove stray only
ad5b150 
clear up logic

lint fix

clean up

lint fix 2
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-5720/clarify-retryable-fields branch from 370b1a9 to ad5b150 Compare July 9, 2024 21:28
baileympearson
baileympearson reviewed Jul 10, 2024
View reviewed changes
src/error.ts Outdated
@@ -1237,11 +1242,13 @@ export function needsRetryableWriteLabel(error: Error, maxWireVersion: number):
}

if (error instanceof MongoWriteConcernError) {
return RETRYABLE_WRITE_ERROR_CODES.has(error.result?.code ?? error.code ?? 0);
return serverType === 'Mongos' && maxWireVersion < 9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right still - the spec says that for <4.4 mongoses, we do check the top level code, we just don't check writeConcernError.code. As written, I think that this will always return false if a writeConcernError is returned with a top-level retryable error code, when instead we should be hitting the if-statement on line 1250.

Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if statement in like 1250 checks for error.code and if the error is a writeConcernError it will check for writeConcernError.code. Because of this we wouldn't a <4.4 sharded writeConcernError getting to line 1250.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'm saying that I think it's supposed to reach line 1250 but for a pre-4.4 mongos writeConcernError, we will always return false here.

baileympearson
baileympearson reviewed Jul 10, 2024
View reviewed changes
test/integration/retryable-writes/retryable_writes.spec.test.ts Outdated
if (
this.configuration.topologyType === 'Sharded' &&
lt(this.configuration.version, '4.4.0')
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to check the description, right? we don't want to skip all tests on <4.4 mongoses.

Also - we usually attach a skip reason:

this.currentTest.skipReason = ...
this.skip();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attached the skip reason and moved the server type, server version, and test.description check to all in one place!

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5720): on pre-4.4 sharded servers, the node driver uses WriteConcernError.code to determine retryability fix(NODE-5720): on pre-4.4 sharded servers, the node driver uses error.writeConcern.code to determine retryability Jul 11, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-5720/clarify-retryable-fields branch from 4085658 to a57fb1d Compare July 11, 2024 17:33
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-5720/clarify-retryable-fields branch from a57fb1d to ac247a7 Compare July 11, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baileympearson baileympearson Awaiting requested review from baileympearson

Requested changes must be addressed to merge this pull request.

Assignees

@baileympearson baileympearson

Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Milestone
No milestone
2 participants
@aditi-khare-mongoDB @baileympearson