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-3521): update session support checks #3151

Merged
merged 12 commits into from
Mar 3, 2022
Merged

fix(NODE-3521): update session support checks #3151

merged 12 commits into from
Mar 3, 2022

Conversation

durran
Copy link
Member

@durran durran commented Feb 24, 2022

Description

With the increase of the minimum wire version, server versions below 3.6 are no longer supported and thus the need to check if a topology supports sessions is no longer needed. This check also caused session mismatch errors in rare cases when a topology went unknown before a cursor's initial find command executed and came back before the subsequent getMore command was executed.

What is changing?

Removes all hasSessionSupport checks and tests that are no longer relevant.

Is there new documentation needed for these changes?

No

What is the motivation for this change?

NODE-3521 / NODE-4009

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>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

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.

Some open Qs

src/operations/execute_operation.ts Outdated Show resolved Hide resolved
src/operations/execute_operation.ts Outdated Show resolved Hide resolved
src/operations/execute_operation.ts Outdated Show resolved Hide resolved
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 25, 2022
@durran durran changed the title fix(NODE-3521): remove session support checks fix(NODE-3521): update session support checks Feb 28, 2022
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.

Q about the selection

src/cursor/abstract_cursor.ts Show resolved Hide resolved
nbbeeken
nbbeeken previously approved these changes Mar 1, 2022
@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 Mar 1, 2022
test/unit/cursor/find_cursor.test.js Outdated Show resolved Hide resolved
test/unit/cursor/aggregation_cursor.test.js Outdated Show resolved Hide resolved
test/unit/cursor/aggregation_cursor.test.js Outdated Show resolved Hide resolved
test/unit/cursor/aggregation_cursor.test.js Outdated Show resolved Hide resolved
test/unit/cursor/find_cursor.test.js Outdated Show resolved Hide resolved
dariakp
dariakp previously approved these changes Mar 2, 2022
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

I agree with Bailey's cleanup suggestions but otherwise LGTM

dariakp
dariakp previously approved these changes Mar 2, 2022
@dariakp dariakp self-requested a review March 2, 2022 22:55
@nbbeeken nbbeeken merged commit aaa453d into main Mar 3, 2022
@nbbeeken nbbeeken deleted the NODE-3521 branch March 3, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants