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

RUST-1713 Bulk Write #1034

Merged
merged 80 commits into from
May 14, 2024
Merged

RUST-1713 Bulk Write #1034

merged 80 commits into from
May 14, 2024

Conversation

isabelatkinson
Copy link
Contributor

@isabelatkinson isabelatkinson commented Feb 21, 2024

Implement support for the new bulkWrite command. This PR can be reviewed in tandem with the specification changes (mongodb/specifications#1534).

I've left some things out of this PR to minimize its size and ensure that it will be a digestible reference implementation. Additional tickets can be found in RUST-1282.

@isabelatkinson isabelatkinson changed the title RUST-1282 Bulk Write POC RUST-1282 Bulk Write Feb 29, 2024
@isabelatkinson isabelatkinson changed the title RUST-1282 Bulk Write RUST-1713 Bulk Write Feb 29, 2024
src/action/bulk_write.rs Show resolved Hide resolved
src/client/executor.rs Show resolved Hide resolved
src/operation.rs Outdated Show resolved Hide resolved
options: BulkWriteOptions,
}

impl<'de> Deserialize<'de> for WriteModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

serde doesn't allow configuring different values for whether an enum should be "tagged" for serialization and deserialization, which requires a manual impl here

src/test/spec.rs Show resolved Hide resolved
src/operation.rs Outdated Show resolved Hide resolved
src/test/bulk_write.rs Outdated Show resolved Hide resolved
src/client/options/bulk_write.rs Show resolved Hide resolved
src/client/options/bulk_write.rs Show resolved Hide resolved
src/results/bulk_write.rs Show resolved Hide resolved
@isabelatkinson isabelatkinson marked this pull request as ready for review March 4, 2024 22:13
src/client/options/bulk_write.rs Show resolved Hide resolved
src/coll.rs Outdated Show resolved Hide resolved
src/operation.rs Outdated Show resolved Hide resolved
src/operation.rs Outdated Show resolved Hide resolved
src/operation.rs Outdated Show resolved Hide resolved
src/action/bulk_write.rs Outdated Show resolved Hide resolved
src/results/bulk_write.rs Show resolved Hide resolved
src/serde_util.rs Outdated Show resolved Hide resolved
src/test/spec.rs Show resolved Hide resolved
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM!

src/operation.rs Outdated Show resolved Hide resolved
src/operation.rs Outdated Show resolved Hide resolved
@@ -75,6 +81,12 @@ const MAX_ENCRYPTED_WRITE_SIZE: usize = 2_097_152;
// The amount of overhead bytes to account for when building a document sequence.
const COMMAND_OVERHEAD_SIZE: usize = 16_000;

/// Context about the execution of the operation.
pub(crate) struct ExecutionContext<'a> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future-proofing against the annoyance of having to add a new parameter to every implementation of handle_response...

description: impl AsRef<str>,
) -> std::result::Result<(), String> {
let description = description.as_ref();
pub(crate) fn verify_result(&self, error: &Error, description: impl AsRef<str>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might just be personal preference, but returning string errors here rather than performing assertions was making these errors feel very opaque because a) they require us to write errors ourselves rather than use the standard format of assertion and panic error messages and b) we lose info about the exact line where the error occurred. We were just unwrapping the error returned from this method anyway so I don't think we were getting much benefit from this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a surface-level refactor of this because I thought the old implementation was causing me problems (I was actually just forgetting to store the FailPointGuard being returned, added #[must_use] on that type). In retrospect this PR is probably not the place for extra stuff so I can pull this out into another PR it's too distracting 🙂

@isabelatkinson isabelatkinson requested a review from abr-egn May 3, 2024 23:00
src/test/bulk_write.rs Show resolved Hide resolved
src/test/bulk_write.rs Show resolved Hide resolved
src/operation/create_indexes/test.rs Show resolved Hide resolved
src/operation/aggregate.rs Show resolved Hide resolved
src/test/spec/retryable_reads.rs Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/operation/insert.rs Outdated Show resolved Hide resolved
src/operation/bulk_write.rs Outdated Show resolved Hide resolved
src/operation/insert.rs Outdated Show resolved Hide resolved
src/operation/bulk_write.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

the lint failure is unrelated, I'll fix in a separate PR

src/error.rs Show resolved Hide resolved
src/operation/bulk_write.rs Outdated Show resolved Hide resolved
src/operation/create_indexes/test.rs Show resolved Hide resolved
@isabelatkinson isabelatkinson requested a review from kevinAlbs May 10, 2024 14:57
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM with a possible change to the serialized name of message. Great work!

src/error.rs Show resolved Hide resolved
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM! I solemnly swear ✋ that I'm not rubber-stamping this, it's just that it's such a well put together PR that I can't find anything to comment on!

@isabelatkinson
Copy link
Contributor Author

@abr-egn thank you 🙂

@isabelatkinson isabelatkinson merged commit f5d7c4c into mongodb:main May 14, 2024
9 of 12 checks passed
isabelatkinson added a commit to isabelatkinson/mongo-rust-driver that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants