-
Notifications
You must be signed in to change notification settings - Fork 73
[HZ-5278] Asyncio module soak tests #755
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
Conversation
- Fixed a bug where an asyncio Lock was tried to be acquired without a release - Store asyncio tasks in order not to lose them - Removed start/shutdown from AsyncioReactor
* Removed unnecessary client components * Converted partition service to async * Ported compact compatibility test
# Conflicts: # examples/asyncio/cloud-discovery/hazelcast_cloud_discovery_example.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #755 +/- ##
==========================================
- Coverage 94.57% 94.55% -0.02%
==========================================
Files 393 393
Lines 25074 25072 -2
==========================================
- Hits 23713 23707 -6
- Misses 1361 1365 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "Programming Language :: Python", | ||
| "Programming Language :: Python :: 3", | ||
| "Programming Language :: Python :: 3 :: Only", | ||
| "Programming Language :: Python :: 3.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come this was not caught before? Dont we have a test to verify all supported runtimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't test for these.
These are just annotations.
| pids+=("$pid") | ||
| done | ||
|
|
||
| for i in {1..5}; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there used to be 10 clients all same type. Now you mıxed asyncio and non-asyncio usage. But this is not an expected usage from users. I would keep 10 clients and have 2 different tests for asynci and asyncore. We need to make sure both clients work independently in their own cluster without a problem. Mixed usage is another use case which is less interest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users can use both asyncio and asyncore clients together without problems even in the same process.
In the soak tests, asyncio and asyncore clients run in different processes, so there's even better isolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not do such mixed tests for other clients which is also possible. If you want to change the soak test in this way, we better evaluate changing it for all clients. Original soak test did not mean to test this scenario.
AsyncioReactor._create_connectionto be compatible withuvloop.