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-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 •
edited

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

nbbeeken
nbbeeken reviewed Feb 24, 2022
View reviewed changes
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
nbbeeken
nbbeeken reviewed Feb 28, 2022
View reviewed changes
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
@durran durran requested a review from nbbeeken March 1, 2022 13:04
nbbeeken
nbbeeken previously approved these changes Mar 1, 2022
View reviewed changes
@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
dariakp
dariakp requested changes Mar 1, 2022
View reviewed changes
test/unit/cursor/find_cursor.test.js Outdated Show resolved Hide resolved
test/unit/cursor/aggregation_cursor.test.js Outdated Show resolved Hide resolved
@durran durran requested a review from dariakp March 2, 2022 10:32
baileympearson
baileympearson requested changes Mar 2, 2022
View reviewed changes
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
View reviewed changes
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

@durran @baileympearson
Update test/unit/cursor/aggregation_cursor.test.js
68365bf 
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
durran and others added 2 commits March 2, 2022 23:05
@durran @baileympearson
Update test/unit/cursor/aggregation_cursor.test.js
dd594f6 
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
@durran @baileympearson
Update test/unit/cursor/find_cursor.test.js
05ad2d2 
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
dariakp
dariakp previously approved these changes Mar 2, 2022
View reviewed changes
@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
Reviewers

@jyemin jyemin jyemin left review comments

@baileympearson baileympearson baileympearson approved these changes

@nbbeeken nbbeeken nbbeeken left review comments

@dariakp dariakp Awaiting requested review from dariakp

Assignees
No one assigned
Labels
Team Review Needs review from team
Projects
None yet
Milestone
No milestone
5 participants
@durran @jyemin @nbbeeken @baileympearson @dariakp