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-3515): do proper opTime merging in bulk results #3012

Merged
merged 5 commits into from Oct 20, 2021
Merged

Conversation

durran
Copy link
Member

@durran durran commented Oct 19, 2021

Description

This is a forward port to 4.1 branch for NODE-3515.

Fixes the merging of results in a bulk write inside a transaction to properly compare and set the opTime value in the result.

What is changing?

Per various specifications, lastOp and opTime are optional fields in results that can be either a Timestamp, or an object with a ts and t property of Timestamp and Long respectively. In some cases they are both Longs. Our comparison logic on these values could get into a hot mess when trying to compare values of different types. This changes the opTime value in the bulk write result to always be an object with the ts and t fields and never sets it as a Timestamp value anymore.

Is there new documentation needed for these changes?

No

What is the motivation for this change?

This was reported as a bug in HELP-28460 and related to NODE-3515, although the user provided patch did not solve all potential cases.

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 requested a review from nbbeeken October 19, 2021 22:55
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 19, 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.

Pending assertions from the 3.x branch

@nbbeeken nbbeeken requested a review from dariakp October 20, 2021 14:38
@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 Oct 20, 2021
@durran durran requested a review from nbbeeken October 20, 2021 18:41
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 need to update the test case names to match the assertions

test/unit/bulk/common.test.js Outdated Show resolved Hide resolved
test/unit/bulk/common.test.js Outdated Show resolved Hide resolved
test/unit/bulk/common.test.js Outdated Show resolved Hide resolved
test/unit/bulk/common.test.js Outdated Show resolved Hide resolved
test/unit/bulk/common.test.js Outdated Show resolved Hide resolved
@durran durran requested a review from dariakp October 20, 2021 19:09
test/unit/bulk/common.test.js Outdated Show resolved Hide resolved
@dariakp dariakp merged commit 43300c3 into 4.1 Oct 20, 2021
@dariakp dariakp deleted the NODE-3515-4.1 branch October 20, 2021 20:41
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