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-6329): client bulk write happy path #4206

Merged
merged 8 commits into from
Sep 25, 2024
Merged

feat(NODE-6329): client bulk write happy path #4206

merged 8 commits into from
Sep 25, 2024

Conversation

durran
Copy link
Member

@durran durran commented Aug 22, 2024

Description

Implements the "happy path" for the new client bulk write in 8.0

What is changing?

  • Fixes issue in document sequence serialization to account properly for cstring names and the document length.
  • Adds handling of bulk write document sequences in command monitoring.
  • Adds a new client bulk write cursor response to handle top level metadata.
  • Adds a new client bulk write cursor that is always exhausted when used.
  • Implements MongoClient#bulkWrite and all associated model and return types.
  • Fixes the command builder to handle comments and collations on updates properly.
  • Adds _id generation to inserts in the command builder.
  • Handles both acknowledged and unacknowledged client bulk writes.
  • Implements client bulk write in the unified runner and runs happy path unified tests.
Is there new documentation needed for these changes?

None. Note release note highlights will be added on last ticket.

What is the motivation for this change?

NODE-6329

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

@durran durran force-pushed the NODE-6329 branch 3 times, most recently from bbc3701 to 77789ff Compare August 23, 2024 14:12
@durran durran force-pushed the NODE-6329 branch 2 times, most recently from 5c65c07 to c74fcea Compare September 2, 2024 20:17
@durran durran force-pushed the NODE-6329 branch 4 times, most recently from 99506fa to f6b2bf2 Compare September 13, 2024 12:31
@durran durran force-pushed the NODE-6329 branch 12 times, most recently from b19b0f1 to 5f3d4f1 Compare September 18, 2024 19:46
@durran durran marked this pull request as ready for review September 18, 2024 22:25
@nbbeeken nbbeeken self-assigned this Sep 19, 2024
@nbbeeken nbbeeken self-requested a review September 19, 2024 14:51
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 19, 2024
src/operations/client_bulk_write/executor.ts Show resolved Hide resolved
src/operations/client_bulk_write/executor.ts Outdated Show resolved Hide resolved
src/operations/client_bulk_write/command_builder.ts Outdated Show resolved Hide resolved
src/operations/client_bulk_write/command_builder.ts Outdated Show resolved Hide resolved
src/operations/client_bulk_write/results_merger.ts Outdated Show resolved Hide resolved
src/mongo_client.ts Show resolved Hide resolved
src/mongo_client.ts Show resolved Hide resolved
test/tools/unified-spec-runner/match.ts Show resolved Hide resolved
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

@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 Sep 24, 2024
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

two small comments

src/cursor/client_bulk_write_cursor.ts Outdated Show resolved Hide resolved
src/error.ts Outdated Show resolved Hide resolved
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.

There's a small lint failure, we need to add the cursor to the list of public APIs. Quick question about didUpsert too

src/operations/client_bulk_write/results_merger.ts Outdated Show resolved Hide resolved
baileympearson
baileympearson previously approved these changes Sep 24, 2024
@durran
Copy link
Member Author

durran commented Sep 24, 2024

There's a small lint failure, we need to add the cursor to the list of public APIs. Quick question about didUpsert too

Yes that's fixed in the PR now.

@nbbeeken nbbeeken merged commit 3d3da40 into main Sep 25, 2024
30 checks passed
@nbbeeken nbbeeken deleted the NODE-6329 branch September 25, 2024 14:28
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