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-3083): support aggregate writes on secondaries #3022

Merged
merged 7 commits into from Nov 5, 2021
Merged

Conversation

durran
Copy link
Member

@durran durran commented Nov 2, 2021

Description

Allows $merge and $out aggregate operations to execute on secondaries. The logic here is:

  1. If the server version is < 5.0, these operations always execute on the primary.
  2. If the server version is >= 5.0, these operations operate using the provided read preference or the primary if none is provided.

The spec specifically avoids multi-version clusters, so we can handle this at server selection by implementing our own server selection function for this case that does not need to scan twice.

Includes NODE-3684 to skip $out/$merge tests on serverless

What is changing?

Operations can be flagged with trySecondaryWrite to use the new server selector function. This function looks at the read preference and common wire version of the topology to potentially change the read preference used to select the server.

Is there new documentation needed for these changes?

Potentially if we want to let users know about this new feature in the node docs.

What is the motivation for this change?

DRIVERS-823

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

@durran durran force-pushed the NODE-3083 branch 2 times, most recently from bb6a774 to 8e64d3b Compare November 2, 2021 15:42
@durran durran requested a review from dariakp November 2, 2021 15:42
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 2, 2021
@durran durran removed the request for review from dariakp November 2, 2021 16:05
@durran durran removed the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 2, 2021
@durran durran force-pushed the NODE-3083 branch 2 times, most recently from 6f7ddb5 to 41ea187 Compare November 2, 2021 17:03
@durran durran force-pushed the NODE-3083 branch 5 times, most recently from 892dcf1 to 900cc19 Compare November 3, 2021 10:46
@durran durran requested a review from dariakp November 3, 2021 12:04
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 3, 2021
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.

Just a question to make sure I understand the implementation (I'll take a second pass at the newly added tests once I better understand the context, but I'll move it to team review)

src/operations/command.ts Show resolved Hide resolved
@dariakp dariakp requested a review from nbbeeken November 3, 2021 15:57
@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Nov 3, 2021
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.

LGTM

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.

Just a few small requests regarding the tests; I think we're all happy with the actual code approach, and thank you so much for adding the new unit tests!

test/unit/operations/aggregate.test.js Show resolved Hide resolved
test/unit/operations/aggregate.test.js Outdated Show resolved Hide resolved
test/unit/sdam/server_selection.test.js Outdated Show resolved Hide resolved
test/unit/sdam/server_selection.test.js Outdated Show resolved Hide resolved
@durran durran requested a review from dariakp November 4, 2021 09:28
test/unit/sdam/server_selection.test.js Outdated Show resolved Hide resolved
test/unit/sdam/server_selection.test.js Outdated Show resolved Hide resolved
Co-authored-by: Daria Pardue <daria.pardue@mongodb.com>
test/unit/sdam/server_selection.test.js Outdated Show resolved Hide resolved
test/unit/sdam/server_selection.test.js Outdated Show resolved Hide resolved
test/unit/sdam/server_selection.test.js Outdated Show resolved Hide resolved
@dariakp dariakp merged commit f696909 into main Nov 5, 2021
@dariakp dariakp deleted the NODE-3083 branch November 5, 2021 02:15
ljhaywar pushed a commit that referenced this pull request Nov 9, 2021
Co-authored-by: Daria Pardue <daria.pardue@mongodb.com>
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
3 participants