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-6377): remove noResponse option #4240

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

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Sep 16, 2024

Description

Now that unacknowledged writes are supported through writeConcern.w = 0, remove the buggy noResponse option internally and in public APIs.

What is changing?

  • Remove noResponse from public CommandOperationOptions API
  • Update client.close() and RunAdminCommandOperation to use writeConcern instead
Is there new documentation needed for these changes?

None, except removing the option from the Public API.

What is the motivation for this change?

NODE-6060.

Release Highlight

Remove CommandOperationOptions.noResponse option

Setting the noResponse option on CommandOperationOptions would result in indeterminate socket behavior leading to either silent or thrown errors on any following operations. Since this option is broken and causes bugs, we're removing it to call attention to any code that may have been using it inadvertently.

In order to correctly request that the driver recieves 'no response' from the server (usually to perform an unacknowledged write) setting the WriteConcern.w property to 0 is the recommended migration.

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 changed the title fix(NODE-6377): Remove noResponse option fix(NODE-6377): remove noResponse option Sep 16, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the base branch from main to NODE-6060/fire-and-forget September 16, 2024 19:58
Base automatically changed from NODE-6060/fire-and-forget to main September 17, 2024 13:40
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review September 19, 2024 22:12
@nbbeeken nbbeeken self-assigned this Sep 20, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 20, 2024
@nbbeeken nbbeeken self-requested a review September 20, 2024 16:30
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Just some quick comment fixes

src/cmap/connection.ts Outdated Show resolved Hide resolved
test/integration/node-specific/mongo_client.test.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 24, 2024
src/mongo_client.ts Show resolved Hide resolved
durran
durran previously approved these changes Sep 24, 2024
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

CI needs a closer look 👁️

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Still seeing tasks timeout ⏲️

@dariakp dariakp added the wip label Sep 27, 2024
@nbbeeken nbbeeken added Blocked Blocked on other work and removed Team Review Needs review from team labels Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked on other work wip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants