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-2843): implement sessions advanceClusterTime method #2920

Merged
merged 5 commits into from
Aug 2, 2021

Conversation

dariakp
Copy link
Contributor

@dariakp dariakp commented Jul 28, 2021

Description

NODE-2843

What changed?

Implemented the advanceClusterTime session method, refactored internal helper function for naming consistency

@dariakp dariakp added the wip label Jul 28, 2021
test/unit/core/sessions.test.js Outdated Show resolved Hide resolved
src/sdam/common.ts Show resolved Hide resolved
@dariakp dariakp added Primary Review In Review with primary reviewer, not yet ready for team's eyes and removed wip labels Jul 30, 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

@dariakp dariakp marked this pull request as ready for review July 30, 2021 22:15
@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 Jul 30, 2021
@dariakp dariakp requested review from emadum and durran July 30, 2021 22:16
!clusterTime.signature ||
clusterTime.signature.hash?._bsontype !== 'Binary' ||
(typeof clusterTime.signature.keyId !== 'number' &&
clusterTime.signature.keyId?._bsontype !== 'Long') // apparently we decode the key to number?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nbbeeken Just bringing your attention to this change

Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I do not block this PR: looks correct to me.

Remind me to tell you the deeper issue here / file a ticket next week, has to do with BSON promote* options I think.

@nbbeeken nbbeeken merged commit 1fd0244 into 4.0 Aug 2, 2021
@nbbeeken nbbeeken deleted the NODE-2843/advance_cluster_time branch August 2, 2021 19:08
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.

3 participants