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.

Sign up for GitHub

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

Jump to bottom

feat(NODE-6275): Add CSOT support to GridFS #4246

Merged
merged 32 commits into from
Oct 4, 2024
Merged

feat(NODE-6275): Add CSOT support to GridFS #4246

merged 32 commits into from
Oct 4, 2024

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented Sep 24, 2024 •
edited by nbbeeken
Loading

Description

What is changing?

GridFSUploadStream
  • Add CSOT support by piping TimeoutContext down through all operations over lifetime of GridFSUploadStream
  • Migrate checkIndexes call from constructor to async construct() method to accomodate CSOT error handling
GridFSDownloadStream
  • Add CSOT support by piping TimeoutContext down through all operations over lifetime of GridFSDownloadStream
GridFSBucket
  • Add support for timeoutMS option to:
    • GridFSBucket.openUploadStream()
    • GridFSBucket.openUploadStreamWithId()
    • GridFSBucket.openDownloadStream()
    • GridFSBucket.openDownloadStreamByName()
    • GridFSBucket.find()
    • GridFSBucket.drop()
    • GridFSBucket.rename()
    • GridFSBucket.delete()
Testing
  • Unskip CSOT gridfs spec tests
  • Implement Prose test 6 and 7
  • Add int tests that verify that non-spec-required methods (GridFSBucket.openUploadStreamWithId() and GridFSBucket.openDownloadStreamByName()) properly handle timeoutMS
  • Add int test verifying that 'index' event is emitted exactly once by GridFSBucket when creating upload streams
Is there new documentation needed for these changes?

Yes, punt to NODE-5688, also see NODE-6275 for specifics.

What is the motivation for this change?

NODE-6275

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

@W-A-James
Copy link
Contributor Author

W-A-James commented Sep 25, 2024 •
edited
Loading

Note, tests currently failing since mongodb-legacy does not currently pass through the options for some methods that now have an options argument, being addressed here

W-A-James
W-A-James commented Sep 26, 2024
View reviewed changes
src/timeout.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James marked this pull request as ready for review September 26, 2024 15:12
@nbbeeken nbbeeken self-assigned this Sep 27, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 27, 2024
@nbbeeken nbbeeken self-requested a review September 27, 2024 18:27
nbbeeken
nbbeeken requested changes Sep 27, 2024
View reviewed changes
test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
test/integration/client-side-operations-timeout/node_csot.test.ts Outdated Show resolved Hide resolved
src/timeout.ts Outdated Show resolved Hide resolved
src/timeout.ts Outdated Show resolved Hide resolved
src/gridfs/upload.ts Show resolved Hide resolved
src/timeout.ts Outdated Show resolved Hide resolved
src/gridfs/upload.ts Outdated Show resolved Hide resolved
src/gridfs/index.ts Outdated Show resolved Hide resolved
src/gridfs/index.ts Outdated Show resolved Hide resolved
src/gridfs/download.ts Show resolved Hide resolved
@W-A-James
Copy link
Contributor Author

Re the question about leaking cursors on the server, you're right, it's not a new question for CSOT, but thought it might still be good to flag it here. Not sure if there's anything we want to or can do about it rn.

@W-A-James W-A-James requested a review from nbbeeken September 30, 2024 15:25
nbbeeken
nbbeeken requested changes Sep 30, 2024
View reviewed changes
test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
src/timeout.ts Outdated Show resolved Hide resolved
src/gridfs/download.ts Show resolved Hide resolved
@W-A-James W-A-James requested a review from nbbeeken September 30, 2024 19:05
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 30, 2024
baileympearson
baileympearson requested changes Sep 30, 2024
View reviewed changes
src/operations/find.ts Show resolved Hide resolved
src/gridfs/download.ts Show resolved Hide resolved
src/gridfs/download.ts Outdated Show resolved Hide resolved
src/gridfs/download.ts Outdated Show resolved Hide resolved
src/gridfs/upload.ts Show resolved Hide resolved
src/gridfs/upload.ts Outdated Show resolved Hide resolved
src/gridfs/upload.ts Show resolved Hide resolved
src/gridfs/index.ts Outdated Show resolved Hide resolved
src/gridfs/index.ts Outdated Show resolved Hide resolved
baileympearson
baileympearson requested changes Oct 2, 2024
View reviewed changes
src/gridfs/index.ts Outdated Show resolved Hide resolved
src/gridfs/index.ts Show resolved Hide resolved
@baileympearson baileympearson merged commit 1a06868 into NODE-6090 Oct 4, 2024
24 of 27 checks passed
@baileympearson baileympearson deleted the NODE-6275 branch October 4, 2024 14:52
baileympearson added a commit that referenced this pull request Oct 10, 2024
@W-A-James @nbbeeken @baileympearson
feat(NODE-6275): Add CSOT support to GridFS (#4246)
30e70d8 
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
@W-A-James @nbbeeken @baileympearson
feat(NODE-6275): Add CSOT support to GridFS (#4246)
3e2d32f 
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
@W-A-James @nbbeeken @baileympearson
feat(NODE-6275): Add CSOT support to GridFS (#4246)
c637ea8 
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
@W-A-James @nbbeeken @baileympearson
feat(NODE-6275): Add CSOT support to GridFS (#4246)
c0835d9 
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
@W-A-James W-A-James mentioned this pull request Oct 23, 2024
Merged
5 tasks
nbbeeken added a commit that referenced this pull request Nov 1, 2024
@W-A-James @nbbeeken @baileympearson
feat(NODE-6275): Add CSOT support to GridFS (#4246)
c71a450 
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
@W-A-James @nbbeeken @baileympearson
feat(NODE-6275): Add CSOT support to GridFS (#4246)
0623dc3 
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
@W-A-James @nbbeeken
@baileympearson @dariakp
feat(NODE-6275): Add CSOT support to GridFS (#4246)
3cb8187 
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
@github-actions github-actions bot mentioned this pull request Nov 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baileympearson baileympearson baileympearson requested changes

@nbbeeken nbbeeken nbbeeken approved these changes

Assignees

@nbbeeken nbbeeken

Labels
Team Review Needs review from team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@W-A-James @nbbeeken @baileympearson