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

DOCSP-41769: Improved bulk write API #590

Merged

Conversation

mayaraman19
Copy link
Collaborator

@mayaraman19 mayaraman19 commented Nov 11, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - DOCSP-41769
Staging:

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for docs-java ready!

Name Link
🔨 Latest commit 605cf7d
🔍 Latest deploy log https://app.netlify.com/sites/docs-java/deploys/67634a866293c30008f2c8b9
😎 Deploy Preview https://deploy-preview-590--docs-java.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mayaraman19 mayaraman19 changed the title initial checkpoint DOCSP-41769: Improved bulk write API Nov 11, 2024
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

good start! There are a lot of comments, so let me know if you have any questions

source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/whats-new.txt Outdated Show resolved Hide resolved
source/whats-new.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

eep sorry for more comments but it is for sure shaping up!! great work

source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/whats-new.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

no blocking changes, but pointed out some last issues and suggestions!

source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
@mayaraman19 mayaraman19 requested review from a team and katcharov and removed request for a team November 14, 2024 21:36
@stIncMale stIncMale requested review from stIncMale and removed request for katcharov November 15, 2024 21:04
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
source/fundamentals/crud/write-operations/bulk.txt Outdated Show resolved Hide resolved
@stIncMale
Copy link
Member

Note that I have neither compiled nor run the examples.

@mayaraman19 mayaraman19 requested a review from stIncMale December 4, 2024 02:11
@stIncMale
Copy link
Member

@mayaraman19 could you please not resolve discussions started by a reviewer, as it complicates the review process: the list of unresolved discussions represent the items the reviewer has to re-review, and by resolving them the PR author hides those items from the reviewer. It should be up to the reviewer to decide whether a concern raised by him is resolved.

@mayaraman19
Copy link
Collaborator Author

@stIncMale Thanks for the feedback. I have unresolved comments that I initially resolved, if you want to take another look at those.

@stIncMale
Copy link
Member

I have unresolved comments that I initially resolved, if you want to take another look at those.

:trollface: 😭 This resulted in even more confusion, because I have reviewed all of them, including the resolved ones. But now I will have to re-review them again.

Copy link
Contributor

@mongoKart mongoKart left a comment

Choose a reason for hiding this comment

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

a lot of small suggestions and a couple changes for completeness and clarity. nice work!

Comment on lines 24 to 25
document, you can use the ``insertOne()`` and ``replaceOne()`` methods. In this
case, the ``MongoClient`` makes a call to the database for each operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: clarify

Suggested change
document, you can use the ``insertOne()`` and ``replaceOne()`` methods. In this
case, the ``MongoClient`` makes a call to the database for each operation.
document, you can use the ``insertOne()`` and ``replaceOne()`` methods. When you
use these methods, the ``MongoClient`` makes one call to the database for each operation.

Comment on lines 27 to 28
Generally, you can reduce the number of calls to the database by using bulk write
operations. You can perform bulk write operations at the following levels:
Copy link
Contributor

Choose a reason for hiding this comment

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

S: 'generally' is a little vague. we can just state a fact instead

Suggested change
Generally, you can reduce the number of calls to the database by using bulk write
operations. You can perform bulk write operations at the following levels:
By using a bulk write operation, you can perform multiple write operations
in a single database call. You can perform bulk write operations at the following levels:

Generally, you can reduce the number of calls to the database by using bulk write
operations. You can perform bulk write operations at the following levels:

- :ref:`Collection Level <java-sync-coll-bulk-write>`: You can use the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- :ref:`Collection Level <java-sync-coll-bulk-write>`: You can use the
- :ref:`Collection <java-sync-coll-bulk-write>`: You can use the

operations in the same call, but makes two calls to the database for an insert
operation and a replace operation.

- :ref:`Client Level <java-sync-client-bulk-write>`: When running {+mdb-server+}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- :ref:`Client Level <java-sync-client-bulk-write>`: When running {+mdb-server+}
- :ref:`Client <java-sync-client-bulk-write>`: When running {+mdb-server+}

Comment on lines 32 to 33
single collection. This method groups write operations together by the kind
of operation. For example, ``MongoCollection.bulkWrite()`` puts multiple update
Copy link
Contributor

Choose a reason for hiding this comment

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

S:

Suggested change
single collection. This method groups write operations together by the kind
of operation. For example, ``MongoCollection.bulkWrite()`` puts multiple update
single collection. In this method, each kind of write operation requires one
database call. For example, ``MongoCollection.bulkWrite()`` puts multiple update

Comment on lines 326 to 327
to multiple databases and collections in the same cluster. The ``MongoClient.bulkWrite()``
method does not split different kinds of operations into different batches.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: if someone jumps straight to this section, they might not see that this line is intended to compare client bulk writes to collection bulk writes.

Suggested change
to multiple databases and collections in the same cluster. The ``MongoClient.bulkWrite()``
method does not split different kinds of operations into different batches.
to multiple databases and collections in the same cluster. The ``MongoClient.bulkWrite()``
method performs all write operation in a single batch.

Comment on lines 329 to 334
In the examples in preceding sections of this page, the ``MongoCollection.bulkWrite()`` method
takes a list of ``WriteModel`` instances in which the specified subclass of
``WriteModel`` represents the corresponding write operation. For example, an
instance of ``InsertOneModel`` represents an operation to insert one document.

Similarly, the ``MongoClient.bulkWrite()`` method takes a
Copy link
Contributor

Choose a reason for hiding this comment

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

S: unfortunately, i think it's worth repeating the info from earlier in the page just to users don't have to read earlier sections if they don't need to. if you have a lot of repeated content, you can always put it in an includes directive 😎

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

honestly, I'm not sure how much this information is relevant so I will remove it 👍

For example, an instance of ``ClientNamespacedInsertOneModel`` represents an
operation to insert one document.

You can construct instances of ``ClientNamespacedWriteModel`` using the following
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can construct instances of ``ClientNamespacedWriteModel`` using the following
You can construct instances of the ``ClientNamespacedWriteModel`` class by using the following

You can construct instances of ``ClientNamespacedWriteModel`` using the following
methods:

- ``insertOne()``
Copy link
Contributor

Choose a reason for hiding this comment

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

S: would use this pattern for each method and delete the following paragraph

Suggested change
- ``insertOne()``
- ``ClientNamespacedWriteModel.insertOne()``: Constructs a ``ClientNamespacedInsertOneModel`` instance.

Comment on lines 360 to 363
database and collection to write to. Some, such as ``insertOne()``, can take a
``Document`` instance that defines information about the write operation.
Some other methods, such as ``updateOne()`` and ``replaceOne()``, take a filter
object that defines the subset of documents to be updated or replaced.
Copy link
Contributor

Choose a reason for hiding this comment

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

for comprehensiveness and clarity, we probably want to specify which methods take what. you could give each method its own subsection, or add a parameter list/table for each one

Copy link
Contributor

@mongoKart mongoKart left a comment

Choose a reason for hiding this comment

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

a few things to change but LGTM otherwise! lmk if you have questions or want to pair up.

Comment on lines 53 to 54
The ``MongoCollection.bulkWrite()`` method performs each kind of write
operations in a separate call. For example, when you pass ``DeleteOneModel``,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The ``MongoCollection.bulkWrite()`` method performs each kind of write
operations in a separate call. For example, when you pass ``DeleteOneModel``,
The ``MongoCollection.bulkWrite()`` method performs each kind of write
operation in a separate database call. For example, when you pass ``DeleteOneModel``,

Comment on lines 55 to 56
``DeleteManyModel``, and ``ReplaceOneModel`` objects to the method, it creates
two calls: one for the delete operations and one for the replace operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably say 'creates two batches' or 'performs two calls', but 'creates two calls' is confusing

Suggested change
``DeleteManyModel``, and ``ReplaceOneModel`` objects to the method, it creates
two calls: one for the delete operations and one for the replace operation.
``DeleteManyModel``, and ``ReplaceOneModel`` objects to the method, it performs
two calls: one for the delete operations and one for the replace operation.


.. note::

When the client splits operations into calls, it might reorder operations for
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When the client splits operations into calls, it might reorder operations for
When the client splits operations into separate database calls, it might reorder operations for

Comment on lines 355 to 357
- ``namespace``: defines which database and collection to write to

``document``: defines the document to insert
Copy link
Contributor

Choose a reason for hiding this comment

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

For all of these parameters, removes 'defines which' or 'defines a' and then capitalize the first letter (e.g., 'Database and collection to write to')

https://www.mongodb.com/docs/meta/style-guide/style/lists/#list-items


* - ``ClientNamespacedUpdateOneModel``
- ``updateOne()``
- Creates a model to update at most one document in the ``namespace``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Creates a model to update at most one document in the ``namespace``
- Creates a model to update the first document in the ``namespace``


``options``: (optional) defines options to apply when updating document

One of ``update`` or ``updatePipeline`` must be specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
One of ``update`` or ``updatePipeline`` must be specified.
You must pass a value for either the ``update`` or ``updatePipeline`` parameter.


``options``: (optional) defines options to apply when updating documents

One of ``update`` or ``updatePipeline`` must be specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
One of ``update`` or ``updatePipeline`` must be specified.
You must pass a value for either the ``update`` or ``updatePipeline`` parameter.


* - ``ClientNamespacedReplaceOneModel``
- ``replaceOne()``
- Creates a model to replace at most one document in the ``namespace`` that
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Creates a model to replace at most one document in the ``namespace`` that
- Creates a model to replace the first document in the ``namespace`` that

- ``updateMany()``
* - ``ClientNamespacedDeleteOneModel``
- ``deleteOne()``
- Creates a model to delete at most one document in the ``namespace`` that
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Creates a model to delete at most one document in the ``namespace`` that
- Creates a model to delete the first document in the ``namespace`` that

@mayaraman19 mayaraman19 merged commit 2702594 into mongodb:master Dec 18, 2024
5 checks passed
@mayaraman19 mayaraman19 deleted the DOCSP-41769-improved-bulk-write branch December 18, 2024 22:46
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.

4 participants