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

PYTHON-4784 - Add tests to confirm async parallelism #1886

Merged
merged 9 commits into from
Oct 9, 2024

Conversation

NoahStapp
Copy link
Contributor

No description provided.


class TestAsyncConcurrency(AsyncIntegrationTest):
async def _task(self, client):
await client.db.test.find_one({"$where": delay(1)})
Copy link
Member

Choose a reason for hiding this comment

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

This 1 second delay makes the test take 6 seconds which is too long. Can we lower the sleep to something more like at most 50ms?

Copy link
Contributor Author

@NoahStapp NoahStapp Oct 2, 2024

Choose a reason for hiding this comment

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

Very short sleeps like 50ms don't have a very large time delta due to our 500ms poll timeout. With a 50ms delay, the concurrent version is consistently only 0-10% faster and highly variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My testing with #1870 confirms that this is due to our blocking of the asyncio loop. If we wait to merge that PR before this one, I can lower the timeout to around 10ms.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's wait for #1870

blink1073
blink1073 previously approved these changes Oct 3, 2024
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

@NoahStapp
Copy link
Contributor Author

Does this test pass consistently on macos and windows?

Looks like it doesn't pass consistently on linux: spruce.mongodb.com/task/mongo_python_driver_tests_python_version_rhel8_test_ssl__platformrhel8_auth_sslauth_ssl_python_version3.10_coveragecoverage_test_3.6_replica_set_patch_b111cbf5d5dab906a94d2c4b2a209cfde2971a94_66feef335bc7120007d6bca4_24_10_03_19_23_36?execution=0&sortBy=STATUS&sortDir=ASC

Perhaps raise the timeout to 100m or 200ms and limit it to only linux?

It was passing consistently for me, but could be the difference between local and remote latency. I'll up the timeout and lower the threshold a bit.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@NoahStapp NoahStapp merged commit 5fa4380 into mongodb:master Oct 9, 2024
35 checks passed
@NoahStapp NoahStapp deleted the PYTHON-4784 branch October 9, 2024 14:44
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.

3 participants