feat(NODE-6296)!: remove cursor default batch size of 1000 #4729
Conversation
baileympearson
force-pushed
the
NODE-6296
branch
from
38a0927 to
ba84660
Compare
| // Generate docs | ||
| const docs = []; | ||
| for (let i = 0; i < 3500; i++) docs.push({ a: i }); | ||
|
|
There was a problem hiding this comment.
Before the changes in this PR, this test relied on a combination of server behavior and driver defaults to pass. getMore always set a batchSize of 1000, and local testing showed the server returning 101 documents for the aggregate. So, 100 documents for the aggregate + 1000 for each getMore results in 3 getMores to exhaust the cursor.
This test fails now that we don't have a default batchSize on getMores because cursor exhaustion is reached in fewer than 3 getMores. To make the test reliable and reproducible, I added a batchSize: 1000 argument to the aggregate. However, this also sets the batchSize for the initial aggregate, so we need a few more documents (1000ish more) in order for the test to pass.
| // TODO(DRIVERS-1448): Remove logic to enforce `limit` in the driver | ||
| let cleanup: () => void = noop; | ||
| const { batchSize } = this.cursorOptions; | ||
| if (batchSize != null && batchSize > remaining) { | ||
| this.cursorOptions.batchSize = remaining; | ||
|
|
||
| // After executing the final getMore, re-assign the batchSize back to its original value so that | ||
| // if the cursor is rewound and executed, the batchSize is still correct. | ||
| cleanup = () => { | ||
| this.cursorOptions.batchSize = batchSize; | ||
| }; | ||
| } | ||
|
|
||
| const response = await super.getMore(batchSize); | ||
| // TODO: wrap this in some logic to prevent it from happening if we don't need this support | ||
| this.numReturned = this.numReturned + response.batchSize; | ||
| try { | ||
| const response = await super.getMore(); | ||
|
|
||
| return response; | ||
| this.numReturned = this.numReturned + response.batchSize; | ||
|
|
||
| return response; | ||
| } finally { | ||
| cleanup?.(); |
There was a problem hiding this comment.
I like cleanup functions, and they naturally work well with explicit resource management (for whenever we can use resource management in the driver):
using stack = new DisposableStack();
stack.defer(() => ( this.cursorOptions.batchSize = batchSize ) );
But I'm happy to rework this if people prefer an alternative approach (i.e., manually assigning the value in a try-finally). Just lmk what the preference is.
durran
added
the
Primary Review
| // TODO(DRIVERS-1448): Remove logic to enforce `limit` in the driver | ||
| let cleanup: () => void = noop; | ||
| const { batchSize } = this.cursorOptions; | ||
| if (batchSize != null && batchSize > remaining) { |
There was a problem hiding this comment.
I'm curious about the reasoning for this. What is the harm of sending a batch size greater than the number of remaining documents? Can this block just be removed?
There was a problem hiding this comment.
Did you read the DRIVERS ticket?
Its here historically because the legacy wire protocol required it (it required drivers to enforce the limit). Then unified tests were added that enforce this behavior. It's not needed anymore but the unified tests still exist.
2 participants
Description
Summary of Changes
Cursors no longer provide a default batchSize of 1000 for getMores.
Notes for Reviewers
What is the motivation for this change?
Release Highlight
Cursors no longer provide a default batchSize of 1000 for getMores
In driver versions <7.0, the driver provides a default batchSize of 1000 for each getMore when iterating a cursor. This behavior is not ideal because the default is set regardless of the documents being fetched. For example, if a cursor fetches many small documents, the driver's default of 1000 can result in many round-trips to fetch all documents, when the server could fit all documents inside a single getMore if no batchSize were set.
Now, cursors no longer provide a default
batchSizewhen executing a getMore. AbatchSizewill only bet set ongetMorecommands if a batchSize has been explicitly configured for the cursor.Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript