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-4125): change stream resumability #3289

Merged
merged 31 commits into from
Jun 22, 2022
Merged

fix(NODE-4125): change stream resumability #3289

merged 31 commits into from
Jun 22, 2022

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Jun 13, 2022 •
edited
Loading

This PR address resumability issues in the iterator-based API for change streams.

As much as I tried to separate the refactor from the code changes, I ended up making a few refactors as a part of this PR.

  1. 44635ba makes the cursor property on the change streams always defined (non-optional). This was primarily to make tests that use the cursor easier to work with.
  2. 881dcf2 breaks apart the error handling for iterator and event emitter modes. Although this does result in some duplicated code, it did simply each method and allowed the promisifying of iterator-mode resume without modifying event emitter mode resuming.
  3. I made each iterator method (tryNext, next and hasNext) internally use async IIFEs. I did this for two reasons. First, it made the resume logic a bit simpler to read and write. Second, once we get to the async/await work, we can simply remove the maybePromise wrapper and the IIFE, and we should have the async functionality we need already written for us.

Relevant Commits

8820ea1 is the most important commit in this PR, as it actually fixes the resumability for iterator mode.

94a4853 makes a drive-by fix to remove an internal function in favor of server selection in the resume process.

Note

As it turns out, the stream based change stream API has never been resumable. I can file a follow up ticket to make our stream resumable, if we want to, but it seemed out of scope for this bug fix work.

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

@baileympearson baileympearson force-pushed the NODE-4125-misc-change-stream-cleanups-2 branch 3 times, most recently from 19ebf83 to 63f77b6 Compare June 15, 2022 15:15
@baileympearson
test: add resumabilty tests for change streams
2dceb56 
- adds resumability tests for the iterator based API (tryNext, hasNext and next)
- adds resumability tests for the event emitter-based API
- adds tests demonstrating that change streams do not resume on
   unresumable errors for iterator and event emitter based apis
- adds tests for resumability iterator mode on 3.6 servers
- adds resume options tests to resumability tests
@baileympearson
fix: use server selection in change stream resume process
94a4853 
This commit removes a custom function we had to wait until the topology
was reconnected in favor of performing server selection.  This is described
here: https://github.com/mongodb/specifications/blob/master/source/change-streams/change-streams.rst#resume-process
@baileympearson
refactor: break apart change stream error handling for iterator / eve…
881dcf2 
…ntemitter modes
@baileympearson
fix: fix change stream resumability for iterator mode
1a0d82b
@baileympearson
docs: add note clarifying that ChangeStream.stream does not resume
45a56d2
@baileympearson
fix: lint error
b7d2481
@baileympearson baileympearson force-pushed the NODE-4125-misc-change-stream-cleanups-2 branch from 63f77b6 to d57f78a Compare June 15, 2022 15:23
@baileympearson
refactor: prevent ChangeStream.cursor from ever being undefined
44635ba
@baileympearson
fix: cache maxWireVersion on cursor for resumability check
6ee61da
@baileympearson
test: prevent tests from running on standalone servers
f65cc73
@baileympearson
test: fix tryNext tests on sharded clusters
28e1b77 
	tryNext is not blocking and on sharded clusters we don't have control of when
	the actual change event will be ready on the change stream pipeline. This introduces
	a race condition, where sometimes we receive the change event and sometimes
	we don't when we call tryNext, depending on the timing of the sharded cluster.

	Since we really only care about the resumability, it's enough for this test to throw
	if tryNext ever throws and assert on the number of aggregate events.
@baileympearson baileympearson force-pushed the NODE-4125-misc-change-stream-cleanups-2 branch from d57f78a to 28e1b77 Compare June 15, 2022 16:24
@baileympearson baileympearson changed the title WIP fix: change stream resumability fix(NODE-4125): change stream resumability Jun 15, 2022
@baileympearson baileympearson marked this pull request as ready for review June 15, 2022 16:53
@durran durran self-assigned this Jun 16, 2022
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 16, 2022
durran
durran requested changes Jun 16, 2022
View reviewed changes
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

I removed all the if this.cursor checks on this PR and made this.cursor? lines just this.cursor and all change stream integration tests passed. Just want to make sure that's expected and that extra code could be removed (since we always have a cursor it seems). Would you like me to just push my changes to the branch or am I missing something where the cursor would not be there?

src/change_stream.ts Outdated Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
@durran
Copy link
Member

durran commented Jun 16, 2022

My patch run: https://spruce.mongodb.com/version/62ab44d632f417132f2a9a8a/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

durran
durran requested changes Jun 16, 2022
View reviewed changes
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

The refactor away from the resume queue is so much easier to understand. Nice work. Just the one nit left after the changes.

src/change_stream.ts Outdated Show resolved Hide resolved
durran
durran previously approved these changes Jun 16, 2022
View reviewed changes
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks!

@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 16, 2022
@baileympearson
chore: misc fixes
315343e 
- removes MongoClient.connect() call from new change stream tests
- awaits a promise in iterateUntilDocumentOrError in the unified runner
- removes an unused method in the change stream class
@baileympearson
chore: add comment explaining why shadowing is necessary in maybeProm…
d0d0641 
…ise call
@baileympearson
test: add better testing for maxwireVersion caching on cursor
51a2907
dariakp
dariakp reviewed Jun 17, 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.

the changes addressing my comments look good to me, I just have a suggestion on one of the tests

test/integration/change-streams/change_stream.test.ts Outdated Show resolved Hide resolved
nbbeeken
nbbeeken requested changes Jun 17, 2022
View reviewed changes
test/integration/change-streams/change_stream.test.ts Show resolved Hide resolved
test/integration/change-streams/change_stream.test.ts Outdated Show resolved Hide resolved
test/integration/change-streams/change_stream.test.ts Outdated Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
src/change_stream.ts Show resolved Hide resolved
nbbeeken
nbbeeken previously approved these changes Jun 17, 2022
View reviewed changes
nbbeeken
nbbeeken previously approved these changes Jun 21, 2022
View reviewed changes
@durran durran merged commit aa5f97e into main Jun 22, 2022
@durran durran deleted the NODE-4125-misc-change-stream-cleanups-2 branch June 22, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariakp dariakp dariakp approved these changes

@nbbeeken nbbeeken nbbeeken approved these changes

@durran durran Awaiting requested review from durran

Assignees
No one assigned
Labels
Team Review Needs review from team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@baileympearson @durran @nbbeeken @dariakp