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

DRIVERS-2868 Adjust getMore maxTimeMS Calculation for tailable awaitData Cursors #1749

Merged
merged 20 commits into from
Mar 20, 2025

Conversation

prestonvasquez
Copy link
Contributor

@prestonvasquez prestonvasquez commented Jan 29, 2025

DRIVERS-2868

Please complete the following before merging:

@prestonvasquez prestonvasquez requested a review from a team as a code owner January 29, 2025 22:50
@prestonvasquez prestonvasquez changed the title DRIVERS-2869 Adjust getMore maxTimeMS Calculation for tailable awaitData Cursors DRIVERS-2868 Adjust getMore maxTimeMS Calculation for tailable awaitData Cursors Jan 29, 2025
@prestonvasquez prestonvasquez requested a review from vbabanin March 7, 2025 02:31
Copy link
Member

@vbabanin vbabanin left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShaneHarvey
Copy link
Member

ShaneHarvey commented Mar 7, 2025

@vbabanin can you confirm this test passes in .net Java on both linux and windows?

@vbabanin
Copy link
Member

vbabanin commented Mar 13, 2025

@ShaneHarvey @prestonvasquez

With the changes introduced by this spec update and the added unified tests, all tests pass in Java across all topologies. However, I observed some flakiness over multiple runs in apply remaining timeoutMS if less than maxAwaitTimeMS, likely because the first getMore sometimes completes too quickly, causing lte: 99 ms to fail. In my earlier testing, this issue did not appear, but after running more iterations, I observed an assertion failure couple of times.

I also tested using a failPoint to delay the first getMore, which made the test more reliable.

I suggest adding a failPoint here to reduce raciness. Here’s a reference comment in the Java driver: mongodb/mongo-java-driver#1650 (comment).

What do you think?


- description: "apply maxAwaitTimeMS if less than remaining timeout"
operations:
- name: failPoint
Copy link
Member

Choose a reason for hiding this comment

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

According to @vbabanin this failpoint should be moved to the "apply remaining timeoutMS if less than maxAwaitTimeMS" test.

Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

@vbabanin could you run the new tests and confirm the flakes are gone?

@vbabanin
Copy link
Member

vbabanin commented Mar 20, 2025

@vbabanin could you run the new tests and confirm the flakes are gone?

I re-ran the tests in this Java driver PR: mongodb/mongo-java-driver#1650 that includes tailable-awaitData.json changes. All tests passed with no flakiness observed.

Here are the relevant details:
Evergreen patch changes: : https://evergreen.mongodb.com/filediff/67d351fec9bb0a0007cd5b53/?file_name=driver-core%2Fsrc%2Ftest%2Fresources%2Funified-test-format%2Fclient-side-operation-timeout%2Ftailable-awaitData.json&patch_number=0&commit_number=1
Successful patch execution: https://spruce.mongodb.com/version/67d351fec9bb0a0007cd5b53/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

@prestonvasquez prestonvasquez merged commit 24b5318 into mongodb:master Mar 20, 2025
5 checks passed
@prestonvasquez prestonvasquez deleted the DRIVERS-2868 branch March 20, 2025 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants