Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ tests:
namespace: *namespace
filter: { _id: 3 }
update: { $set: { x: 333 } }
ordered: false
expectResult:
insertedCount:
$$unsetOrMatches: 0
Expand Down Expand Up @@ -89,7 +90,7 @@ tests:
command:
bulkWrite: 1
errorsOnly: true
ordered: true
ordered: false
ops:
- insert: 0
document: { _id: 4, x: 44 }
Expand Down
12 changes: 12 additions & 0 deletions source/crud/bulk-write.md
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,11 @@ returned. This field is optional and defaults to false on the server.
value for `verboseResults`, drivers MUST define `errorsOnly` as the opposite of `verboseResults`. If the user did not
specify a value for `verboseResults`, drivers MUST define `errorsOnly` as `true`.

Drivers MUST return a client-side error if `verboseResults` is true with an unacknowledged write concern containing the
following message:

> Cannot request unacknowledged write concern and verbose results

### `ordered`

The `ordered` field defines whether writes should be executed in the order in which they were specified, and, if an
Expand All @@ -622,6 +627,11 @@ server. Drivers MUST explicitly define `ordered` as `true` in the `bulkWrite` co
`BulkWriteOptions`. This is required to avoid inconsistencies between server and driver behavior if the server default
changes in the future.

Drivers MUST return a client-side error if `ordered` is true (including when default is applied) with an unacknowledged
Copy link
Member

Choose a reason for hiding this comment

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

What should drivers that already released this api do? Should we continue supporting it until the next major version? Drop it in a minor version? We usually try to never do the latter but I'd be willing to make an expection here since unack'd writes are so niche and the fix is simple, just add ordered=False.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should drivers that already released this api do?

I think this only currently applies to Python and C (Rust does not implement unacknowledged writes). I was not planning to wait for a major release to fix in C.

Arguably, the previous behavior of allowing ordered+w:0 is a bug, since the behavior does not match the description:

writes will stop executing if an individual write fails

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, Python does not have that bug. We send w:1 so we can stop if a batch fails.

Copy link
Member

Choose a reason for hiding this comment

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

@ShaneHarvey: what is the bug you're referring to? I'm a bit confused how PyMongo sending w:1 relates to the user specifying w:0 with ordered:true. Does that mean PyMongo is already disregarding the user's original write concern in order to catch failed batches?

Copy link
Member

@ShaneHarvey ShaneHarvey Oct 9, 2024

Choose a reason for hiding this comment

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

Yes that is what drivers are supposed to do. With ordered:true + w:0, all but the last batch use w:1 and only the final batch uses w:0. I'm not sure if this is specified anywhere but that's how pymongo's collection.bulk_write works.

That said, I'm fine with dropping support for this in our next release (for client.bulk_write only).

write concern containing the following message:

> Cannot request unacknowledged write concern and ordered writes

### Size Limits

The server reports a `maxBsonObjectSize` in its `hello` response. This value defines the maximum size for documents that
Expand Down Expand Up @@ -925,6 +935,8 @@ batch-splitting to standardize implementations across drivers and simplify batch

## **Changelog**

- 2024-10-07: Error if `w:0` is used with `ordered=true` or `verboseResults=true`.

- 2024-10-01: Add sort option to `replaceOne` and `updateOne`.

- 2024-09-30: Define more options for modeling summary vs. verbose results.
Expand Down
46 changes: 44 additions & 2 deletions source/crud/tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,8 @@ InsertOne: {

Construct as list of write models (referred to as `models`) with the one `model`.

Call `MongoClient.bulkWrite` with `models` and `BulkWriteOptions.writeConcern` set to an unacknowledged write concern.
Call `MongoClient.bulkWrite` with `models`. Pass `BulkWriteOptions` with `ordered` set to `false` and `writeConcern` set
to an unacknowledged write concern.

Expect a client-side error due the size.

Expand All @@ -415,7 +416,8 @@ ReplaceOne: {

Construct as list of write models (referred to as `models`) with the one `model`.

Call `MongoClient.bulkWrite` with `models` and `BulkWriteOptions.writeConcern` set to an unacknowledged write concern.
Call `MongoClient.bulkWrite` with `models`. Pass `BulkWriteOptions` with `ordered` set to `false` and `writeConcern` set
to an unacknowledged write concern.

Expect a client-side error due the size.

Expand Down Expand Up @@ -693,3 +695,43 @@ maxTimeMS value of 2000ms for the `explain`.

Obtain the command started event for the explain. Confirm that the top-level explain command should has a `maxTimeMS`
value of `2000`.

### 15. `MongoClient.bulkWrite` with unacknowledged write concern uses `w:0` for all batches

This test must only be run on 8.0+ servers. This test must be skipped on Atlas Serverless.

If testing with a sharded cluster, only connect to one mongos. This is intended to ensure the `countDocuments` operation
uses the same connection as the `bulkWrite` to get the correct connection count. (See
[DRIVERS-2921](https://jira.mongodb.org/browse/DRIVERS-2921)).

Construct a `MongoClient` (referred to as `client`) with
[command monitoring](../../command-logging-and-monitoring/command-logging-and-monitoring.md) enabled to observe
CommandStartedEvents. Perform a `hello` command using `client` and record the `maxWriteBatchSize` value contained in the
response.

Construct a `MongoCollection` (referred to as `coll`) for the collection "db.coll". Drop `coll`.

Use the `create` command to create "db.coll" to workaround [SERVER-95537](https://jira.mongodb.org/browse/SERVER-95537).

Construct the following write model (referred to as `model`):

```javascript
InsertOne: {
"namespace": "db.coll",
"document": { "a": "b" }
}
```

Construct a list of write models (referred to as `models`) with `model` repeated `maxWriteBatchSize + 1` times.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this test execute faster if it used 5 16MB documents, rather than 100,001 small ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Testing the C driver with a sharded 8.0 cluster locally shows the median of 3 runs:

With many small docs to exceed `maxWriteBatchSize`  : 0.65s
With few large docs to exceed `maxMessageSizeBytes` : 0.35s

Updated prose test to use few large docs.


Call `client.bulkWrite` with `models`. Pass `BulkWriteOptions` with `ordered` set to `false` and `writeConcern` set to
an unacknowledged write concern. Assert no error occurred. Assert the result indicates the write was unacknowledged.

Assert that two CommandStartedEvents (referred to as `firstEvent` and `secondEvent`) were observed for the `bulkWrite`
command. Assert that the length of `firstEvent.command.ops` is `maxWriteBatchSize`. Assert that the length of
`secondEvent.command.ops` is 1. If the driver exposes `operationId`s in its CommandStartedEvents, assert that
`firstEvent.operationId` is equal to `secondEvent.operationId`. Assert both commands include `writeConcern: {w: 0}`.

To force completion of the `w:0` writes, execute `coll.countDocuments` and expect the returned count is
`maxWriteBatchSize + 1`. This is intended to avoid incomplete writes interfering with other tests that may use this
collection.
58 changes: 58 additions & 0 deletions source/crud/tests/unified/client-bulkWrite-errors.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 29 additions & 0 deletions source/crud/tests/unified/client-bulkWrite-errors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,32 @@ tests:
verboseResults: true
expectError:
isClientError: true
- description: "Requesting unacknowledged write with verboseResults is a client-side error"
operations:
- name: clientBulkWrite
object: *client0
arguments:
models:
- insertOne:
namespace: *namespace
document: { _id: 10 }
verboseResults: true
ordered: false
writeConcern: { w: 0 }
expectError:
isClientError: true
errorContains: "Cannot request unacknowledged write concern and verbose results"
- description: "Requesting unacknowledged write with ordered is a client-side error"
operations:
- name: clientBulkWrite
object: *client0
arguments:
models:
- insertOne:
namespace: *namespace
document: { _id: 10 }
# Omit `ordered` option. Defaults to true.
writeConcern: { w: 0 }
expectError:
isClientError: true
errorContains: "Cannot request unacknowledged write concern and ordered writes"
Loading