-
Notifications
You must be signed in to change notification settings - Fork 183
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
Test #371
Test #371
Conversation
Signed-off-by: Margulanz <[email protected]>
Signed-off-by: Margulanz <[email protected]>
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.
This changes what we put into the connection pool dead
set, which in a way changes an interface. Probably not the best solution, so let's think about it a bit more. What's the actual problem? Are you able to explain why the test fails (why TypeError
occurs) only on Windows?
Add Windows CI into this PR?
@@ -187,7 +187,7 @@ def mark_dead(self, connection, now=None): | |||
dead_count = self.dead_count.get(connection, 0) + 1 | |||
self.dead_count[connection] = dead_count | |||
timeout = self.dead_timeout * 2 ** min(dead_count - 1, self.timeout_cutoff) | |||
self.dead.put((now + timeout, connection)) | |||
self.dead.put((now + timeout,id(connection),connection)) |
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.
add a space before ,
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #371 +/- ##
===========================================
- Coverage 72.01% 55.07% -16.94%
===========================================
Files 77 74 -3
Lines 7190 6803 -387
===========================================
- Hits 5178 3747 -1431
- Misses 2012 3056 +1044
... and 49 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Hey @margulanz, Could you kindly answer the questions mentioned above? Thanks :) |
@margulanz, Could you please respond to the above comments when you have a moment? Thank you! |
This seems to have been a red herring, fixed windows CI in #569. |
Description
Updated connection_pool.py, so TypeError does not occur. Updated tests accordingly. I am not sure if this is the best solution, so I am waiting for the advices if there is something wrong with my solution
Issues Resolved
Partially Closes 347.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.