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

feat(NODE-6312): add error transformation for server timeouts #4192

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Aug 7, 2024

Description

What is changing?

  • When the server returns a max time error we turn it into a client side timeout error.
Is there new documentation needed for these changes?

Not for this PR. General CSOT documentation should cover the cases where a timeout error is encountered

What is the motivation for this change?

When CSOT is enabled the goal is to provide a consistent mechanism for timing out operations. Ususally a client side timeout should expire before a server is able to, but depending on the variance in RTT calculation it is possible for the maxTimeMS setting that is sent to the server to be shorter than what remains on the client side. In this case, if we get a server timeout we want to throw the same error that would normally occur in CSOT circumstances.

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

@nbbeeken nbbeeken changed the title feat(NODE-5687): add error transformation for server timeouts feat(NODE-6312): add error transformation for server timeouts Aug 7, 2024
@W-A-James W-A-James self-assigned this Aug 8, 2024
@W-A-James W-A-James self-requested a review August 8, 2024 19:32
@W-A-James W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 8, 2024
src/cmap/wire_protocol/responses.ts Outdated Show resolved Hide resolved
@tom-selander tom-selander added Primary Review In Review with primary reviewer, not yet ready for team's eyes and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 9, 2024
@nbbeeken nbbeeken force-pushed the NODE-5687-csot-error-transformations branch from 4caa93a to 728ab9e Compare August 9, 2024 18:31
@tom-selander tom-selander added Primary Review In Review with primary reviewer, not yet ready for team's eyes and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 9, 2024
src/cmap/connection.ts Outdated Show resolved Hide resolved
@tom-selander tom-selander added Primary Review In Review with primary reviewer, not yet ready for team's eyes and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 9, 2024
src/cmap/connection.ts Show resolved Hide resolved
src/cmap/wire_protocol/responses.ts Show resolved Hide resolved
src/cmap/wire_protocol/responses.ts Show resolved Hide resolved
@aditi-khare-mongoDB
Copy link
Contributor

aditi-khare-mongoDB commented Aug 12, 2024

Bookkeeping: I'm taking over Warren's review while he's OOO.

src/cmap/wire_protocol/responses.ts Show resolved Hide resolved
src/cmap/connection.ts Show resolved Hide resolved
@aditi-khare-mongoDB aditi-khare-mongoDB merged commit 6b78962 into NODE-6090 Aug 12, 2024
22 of 26 checks passed
@aditi-khare-mongoDB aditi-khare-mongoDB deleted the NODE-5687-csot-error-transformations branch August 12, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants