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-6387): Add CSOT support to change streams #4256

Merged
merged 22 commits into from
Oct 29, 2024
Merged

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Sep 27, 2024

Description

What is changing?

ChangeStream
  • Added support for CSOT to change stream iterator mode methods (ChangeStream.next(), ChangeStream.hasNext(), ChangeStream.tryNext())
  • Added support for CSOT to change stream emitter mode
    • Change streams will now emit an error event with a MongoOperationTimeoutError when the internal cursor iteration times out
Connection
Testing
  • Unskip and fix change stream prose test
  • Unskip change stream tests
  • Add integration tests to verify that CSOT behviour for ChangeStream emitter mode
Is there new documentation needed for these changes?

Yes, punted to NODE-5688

What is the motivation for this change?

NODE-6387

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
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

Base automatically changed from NODE-6305 to NODE-6090 October 7, 2024 17:07
@baileympearson baileympearson force-pushed the NODE-6090 branch 2 times, most recently from 3591368 to a645d9f Compare October 10, 2024 20:40
@W-A-James W-A-James marked this pull request as ready for review October 10, 2024 21:44
@nbbeeken nbbeeken self-assigned this Oct 11, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 11, 2024
@W-A-James W-A-James force-pushed the NODE-6387 branch 2 times, most recently from 2820e4d to e5e443d Compare October 11, 2024 20:25
@W-A-James W-A-James changed the base branch from NODE-6090 to NODE-6412-socket-timeout-v2 October 11, 2024 20:26
src/cursor/abstract_cursor.ts Outdated Show resolved Hide resolved
@@ -489,7 +493,7 @@ export abstract class AbstractCursor<
await this.fetchBatch();
} while (!this.isDead || (this.documents?.length ?? 0) !== 0);
} finally {
if (this.cursorOptions.timeoutMode === CursorTimeoutMode.ITERATION && this.cursorId != null) {
if (shouldRefresh) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this line is after fetchBatch the value at cursorId could have changed right? so is the stored condition still the same one we want to refresh on?

Copy link
Contributor Author

@W-A-James W-A-James Oct 14, 2024

Choose a reason for hiding this comment

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

Yes because the only time that cursorId is null is when the cursor is uninitialized , but to guard against any changes we make to that logic, we can check for existence of timeoutContext instead, because they're effectively testing the same thing.

Actually, since we introduced the potential use of an external timeout context, that might not be true. Checking this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know what you find.

It sounds correct; my thinking is that if we've hit an error we should still have a null id because the operation didn't give us one, maybe we still set it to 0 somewhere or this logic runs in either case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only two times we set the cursorId to null are in

  • AbstractCursor#constructor()
  • AbstractCursor.rewind()

Anywhere else, we are setting the cursorId from the server response (which is never null, provided that nothing went wrong server-side), so regardless of where we check the cursorId, in AbstractCursor.next()/.tryNext()/.hasNext(), it will still yield the correct behaviour of only calling timeoutContext.refresh()/.clear() when the cursor is already initialized and in iteration mode.

If it's initially null, then we'd still want to clear the clear the timeout if the cursor gets killed (id set to zero).

src/cursor/change_stream_cursor.ts Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
Base automatically changed from NODE-6412-socket-timeout-v2 to NODE-6090 October 11, 2024 20:44
An error occurred while trying to automatically change base from NODE-6412-socket-timeout-v2 to NODE-6090 October 11, 2024 20:44
src/cmap/connection.ts Outdated Show resolved Hide resolved
src/cmap/connection.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James requested a review from nbbeeken October 14, 2024 20:32
@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 14, 2024
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, just request me back when we can resolve the two threads. Moved to team review

@W-A-James W-A-James requested a review from nbbeeken October 15, 2024 16:58
src/cursor/abstract_cursor.ts Show resolved Hide resolved
src/cursor/change_stream_cursor.ts Show resolved Hide resolved
src/cursor/change_stream_cursor.ts Outdated Show resolved Hide resolved
src/change_stream.ts Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
src/change_stream.ts Show resolved Hide resolved
src/change_stream.ts Outdated Show resolved Hide resolved
@baileympearson baileympearson force-pushed the NODE-6090 branch 2 times, most recently from c3f31da to 4fd4b24 Compare October 21, 2024 20:52
@W-A-James W-A-James force-pushed the NODE-6387 branch 2 times, most recently from 93a148b to fee5ac4 Compare October 22, 2024 15:40
@nbbeeken nbbeeken merged commit d92ebb2 into NODE-6090 Oct 29, 2024
23 of 27 checks passed
@nbbeeken nbbeeken deleted the NODE-6387 branch October 29, 2024 18:09
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