-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(NODE-4413): set maxTimeMS on getMores when maxAwaitTimeMS is specified #3319
Changes from 11 commits
4f2ebd6
afb5b56
1d159d3
3483519
12ca746
9ad9ce9
0c0136a
6f88573
053b194
586b3ed
83c1b56
e1dc674
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1111,7 +1111,7 @@ describe('Change Streams', function () { | |
changeStream.next((err, doc) => { | ||
expect(err).to.exist; | ||
expect(doc).to.not.exist; | ||
expect(err.message).to.equal('ChangeStream is closed'); | ||
expect(err?.message).to.equal('ChangeStream is closed'); | ||
changeStream.close(() => client.close(done)); | ||
}); | ||
}); | ||
|
@@ -1372,23 +1372,98 @@ describe('Change Streams', function () { | |
) | ||
.run(); | ||
|
||
UnifiedTestSuiteBuilder.describe('entity.watch() server-side options') | ||
.runOnRequirement({ | ||
topologies: ['replicaset', 'sharded-replicaset', 'sharded', 'load-balanced'], | ||
minServerVersion: '4.4.0' | ||
}) | ||
.createEntities([ | ||
{ client: { id: 'client0', observeEvents: ['commandStartedEvent'] } }, | ||
{ database: { id: 'db0', client: 'client0', databaseName: 'watchOpts' } }, | ||
{ collection: { id: 'collection0', database: 'db0', collectionName: 'watchOpts' } } | ||
]) | ||
.test( | ||
TestBuilder.it('should use maxAwaitTimeMS to send maxTimeMS on getMore commands') | ||
.operation({ | ||
object: 'collection0', | ||
name: 'createChangeStream', | ||
saveResultAsEntity: 'changeStreamOnClient', | ||
arguments: { maxAwaitTimeMS: 5000 } | ||
}) | ||
.operation({ | ||
name: 'insertOne', | ||
object: 'collection0', | ||
arguments: { document: { a: 1 } }, | ||
ignoreResultAndError: true | ||
}) | ||
.operation({ | ||
object: 'changeStreamOnClient', | ||
name: 'iterateUntilDocumentOrError', | ||
ignoreResultAndError: true | ||
}) | ||
.expectEvents({ | ||
client: 'client0', | ||
events: [ | ||
{ commandStartedEvent: { commandName: 'aggregate' } }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we assert absence of the prop on the getMore in the 2nd case, but not on the initial aggregate here? what's the actual expected behavior from these options in the two cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an (not) existence check, and clarified the titles |
||
{ commandStartedEvent: { commandName: 'insert' } }, | ||
{ commandStartedEvent: { commandName: 'getMore', command: { maxTimeMS: 5000 } } } | ||
] | ||
}) | ||
.toJSON() | ||
) | ||
.test( | ||
TestBuilder.it('should send maxTimeMS on aggregate command') | ||
.operation({ | ||
object: 'collection0', | ||
name: 'createChangeStream', | ||
saveResultAsEntity: 'changeStreamOnClient', | ||
arguments: { maxTimeMS: 5000 } | ||
}) | ||
.operation({ | ||
name: 'insertOne', | ||
object: 'collection0', | ||
arguments: { document: { a: 1 } }, | ||
ignoreResultAndError: true | ||
}) | ||
.operation({ | ||
object: 'changeStreamOnClient', | ||
name: 'iterateUntilDocumentOrError', | ||
ignoreResultAndError: true | ||
}) | ||
.expectEvents({ | ||
client: 'client0', | ||
ignoreExtraEvents: true, // Sharded clusters have extra getMores | ||
events: [ | ||
{ commandStartedEvent: { commandName: 'aggregate', command: { maxTimeMS: 5000 } } }, | ||
{ commandStartedEvent: { commandName: 'insert' } }, | ||
{ | ||
commandStartedEvent: { | ||
commandName: 'getMore', | ||
command: { maxTimeMS: { $$exists: false } } | ||
dariakp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
] | ||
}) | ||
.toJSON() | ||
) | ||
.run(); | ||
|
||
describe('BSON Options', function () { | ||
let client: MongoClient; | ||
let db: Db; | ||
let collection: Collection; | ||
let cs: ChangeStream; | ||
|
||
beforeEach(async function () { | ||
client = await this.configuration.newClient({ monitorCommands: true }).connect(); | ||
db = client.db('db'); | ||
collection = await db.createCollection('collection'); | ||
}); | ||
|
||
afterEach(async function () { | ||
await db.dropCollection('collection'); | ||
await cs.close(); | ||
await client.close(); | ||
client = undefined; | ||
db = undefined; | ||
collection = undefined; | ||
}); | ||
|
||
context('promoteLongs', () => { | ||
|
@@ -1452,7 +1527,7 @@ describe('Change Streams', function () { | |
it('does not send invalid options on the aggregate command', { | ||
metadata: { requires: { topology: '!single' } }, | ||
test: async function () { | ||
const started = []; | ||
const started: CommandStartedEvent[] = []; | ||
|
||
client.on('commandStarted', filterForCommands(['aggregate'], started)); | ||
const doc = { invalidBSONOption: true }; | ||
|
@@ -1473,7 +1548,7 @@ describe('Change Streams', function () { | |
it('does not send invalid options on the getMore command', { | ||
metadata: { requires: { topology: '!single' } }, | ||
test: async function () { | ||
const started = []; | ||
const started: CommandStartedEvent[] = []; | ||
|
||
client.on('commandStarted', filterForCommands(['aggregate'], started)); | ||
const doc = { invalidBSONOption: true }; | ||
|
@@ -1503,7 +1578,7 @@ describe('ChangeStream resumability', function () { | |
const changeStreamResumeOptions: ChangeStreamOptions = { | ||
fullDocument: 'updateLookup', | ||
collation: { locale: 'en', maxVariable: 'punct' }, | ||
maxAwaitTimeMS: 20000, | ||
maxAwaitTimeMS: 2000, | ||
batchSize: 200 | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have tests that test for errors being thrown when execute is called on a zero-ed cursor or if the server is undefined? If not, can we add them to demonstrate this behavior still exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added! :)