Skip to content

Add CSOT to synchronous GridFS operations. #1288

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
Merged
merged 24 commits into from
Jan 17, 2024
Merged

Add CSOT to synchronous GridFS operations. #1288

merged 24 commits into from
Jan 17, 2024

Conversation

@vbabanin
Copy link
Member

@vbabanin vbabanin commented Dec 29, 2023 •
edited
Loading

Description:

This PR updates the GridFS API to align with MongoDB's specifications on timeoutMS. Key changes include:

  • GridFS buckets now inherit timeoutMS from their parent MongoDatabase instance.
  • All GridFS Bucket API methods support the timeoutMS option with "withTimeout" method option.
  • For methods creating streams (e.g., open_upload_stream), timeoutMS caps the entire stream's lifetime, including operations during stream construction, reads/writes, and close/abort calls.
  • Methods interacting with user-provided streams (e.g., upload_from_stream) use timeoutMS for the entire operation's timeout.
  • Documentation updated to reflect potential timeout breaches for user-provided streams lacking timeout support.
  • All cursors created for GridFS API operations now set the timeoutMode option to CURSOR_LIFETIME.

For detailed spec, see: MongoDB Specification - GridFS API.

Performance tests for the recent changes compared with the mainline: Performance Test Results.
The results indicate that there are no significant deviations.

JAVA-5277

@vbabanin vbabanin requested a review from rozza January 5, 2024 08:17
@vbabanin
Account for RTT in tests.
9e8f75e 
JAVA-5277
@vbabanin
Fix test issues.
0da8b11 
JAVA-5277
@vbabanin
Check for minimum server version.
3d33e33 
JAVA-5277
@vbabanin
Merge branch 'CSOT' into JAVA-5277
811ce97 
# Conflicts:
#	driver-core/src/test/functional/com/mongodb/internal/connection/TestCommandListener.java
#	driver-sync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java
#	driver-sync/src/test/functional/com/mongodb/client/ClientSideOperationTimeoutProseTest.java
@vbabanin
Merge with CSOT branch.
1ba1bd2 
JAVA-5277
rozza
rozza requested changes Jan 8, 2024
View reviewed changes
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

Looking good a couple of nits and questions

@vbabanin
Merge branch 'CSOT' into JAVA-5277
e443f44 
# Conflicts:
#	driver-sync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java
@vbabanin
Rename classes.
d1a9449 
Add tests.

JAVA-5277
@vbabanin
Add TODO comments for dependencies.
f351d04 
JAVA-5277
@vbabanin
Add copyrights.
a5061d5 
JAVA-5277
@vbabanin vbabanin requested a review from rozza January 9, 2024 09:07
rozza
rozza requested changes Jan 9, 2024
View reviewed changes
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

Only a couple of minor nits - its looking good 👍

@vbabanin
Reduce bock time for download test.
a05cf56 
JAVA-5277
@vbabanin
Merge branch 'CSOT' into JAVA-5277
6a57207
@vbabanin
Merge branch 'CSOT' into JAVA-5277
06ebc67 
# Conflicts:
#	driver-reactive-streams/src/main/com/mongodb/reactivestreams/client/gridfs/GridFSFindPublisher.java
#	driver-scala/src/main/scala/org/mongodb/scala/gridfs/GridFSFindObservable.scala
#	driver-sync/src/main/com/mongodb/client/gridfs/GridFSFindIterable.java
#	driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedCrudHelper.java
@vbabanin
Fix merge conflicts with CSOT branch.
3025b3c 
JAVA-5277
@vbabanin
Add detailed message for unsupported GridFS test execution.
0d15fff 
JAVA-5277
@vbabanin
Copy link
Member Author

vbabanin commented Jan 13, 2024 •
edited
Loading

Note: All comments marked with a thumbs-up 👍 have been addressed as well. Thanks!

@vbabanin vbabanin requested a review from rozza January 13, 2024 02:16
@vbabanin
Fix tests.
66bee9b 
JAVA-5277
@rozza
Copy link
Member

rozza commented Jan 15, 2024

Any idea whats happening on the test tasks that are timing out?

@vbabanin vbabanin self-assigned this Jan 16, 2024
@vbabanin
Copy link
Member Author

vbabanin commented Jan 16, 2024

Any idea whats happening on the test tasks that are timing out?

it looks like changing from MongoExecutionTimeout to MongoOperationTimeoutException is causing testBlockingIterationMethodsChangeStream to run into an infinite loop due to isResumableError returning always true within this while loop: ChangeStreamBatchCursor.java#L187.

I'm thinking we should tackle this by implementing the ChangeStream part of the spec, as outlined in JAVA-4054. In the meantime, what do you think about temporarily disabling this test with a TODO? Let me know your thoughts

@rozza
Copy link
Member

rozza commented Jan 16, 2024 •
edited
Loading

Thats my bad - will fix in CSOT.

@rozza
Copy link
Member

rozza commented Jan 16, 2024

Fixed the changestream retryable error check in CSOT and merged - waiting for tests to run.

rozza
rozza approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM

@vbabanin
Copy link
Member Author

vbabanin commented Jan 17, 2024 •
edited
Loading

Fixed the changestream retryable error check in CSOT and merged - waiting for tests to run.

@rozza Tests passed – great fix! Thanks for your quick action!

@vbabanin vbabanin merged commit 17b05ad into mongodb:CSOT Jan 17, 2024
@vbabanin vbabanin deleted the JAVA-5277 branch January 17, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rozza rozza rozza approved these changes

Assignees

@vbabanin vbabanin

Labels

priority: TEST

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vbabanin @rozza