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

feat(NODE-6389): add support for timeoutMS in StateMachine.execute() #4243

Merged
merged 44 commits into from
Oct 7, 2024

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Sep 19, 2024

Description

StateMachine.execute and commands run against a mongocryptd server respect a timeoutMS.

What is changing?

  • StateMachine.execute method respects timeoutMS for the following operations:
    • listCollections
    • Fetching data keys
    • KMS requests
  • Implemented spec unit test relating to 'remaining timeoutMS value should apply to commands sent to KMS requests.'
  • mongocryptd commands now use remaining timeoutMS is used, and maxTimeMS is not appended to the command (ServerDescription.iscryptd field is used to determind server type)
  • Implemented Prose Test #2 from CSOT spec
Is there new documentation needed for these changes?

No

What is the motivation for this change?

CSOT

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@aditi-khare-mongoDB aditi-khare-mongoDB changed the base branch from main to NODE-6090 September 19, 2024 21:38
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title feat(NODE-6389): Add support for timeoutMS in StateMachine.execute() feat(NODE-6389): add support for timeoutMS in StateMachine.execute() Sep 20, 2024
): Promise<Array<DataKey>> {
const { db: dbName, collection: collectionName } =
MongoDBCollectionNamespace.fromString(keyVaultNamespace);

return client
.db(dbName)
.collection<DataKey>(collectionName, { readConcern: { level: 'majority' } })
.find(deserialize(filter))
.find(deserialize(filter), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of red in CI caused by this - timeoutMode can only be specified if timeoutMS is specified too. Otherwise cursors throw an error.

Also - lint is failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, changed it!

@baileympearson baileympearson merged commit d26a588 into NODE-6090 Oct 7, 2024
23 of 27 checks passed
@baileympearson baileympearson deleted the NODE-6389 branch October 7, 2024 18:37
baileympearson added a commit that referenced this pull request Oct 10, 2024
…4243)

Co-authored-by: Warren James <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
baileympearson added a commit that referenced this pull request Oct 10, 2024
…4243)

Co-authored-by: Warren James <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
nbbeeken added a commit that referenced this pull request Oct 14, 2024
…4243)

Co-authored-by: Warren James <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
baileympearson added a commit that referenced this pull request Oct 21, 2024
…4243)

Co-authored-by: Warren James <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
nbbeeken added a commit that referenced this pull request Nov 1, 2024
…4243)

Co-authored-by: Warren James <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
nbbeeken added a commit that referenced this pull request Nov 5, 2024
…4243)

Co-authored-by: Warren James <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
dariakp pushed a commit that referenced this pull request Nov 6, 2024
…4243)

Co-authored-by: Warren James <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants