Set enable_cleanup_closed by default #1469
Conversation
|
Actually I think this only fixes the async case, not the sync one. I'll fix it and add an unit test. |
|
@elasticmachine run tests please |
1 similar comment
|
@elasticmachine run tests please |
This is now the default in Elasticsearch's Python client and leaving it false causes memory leaks in Rally for long-running benchmarks.
Timing tests are by nature flaky, especially in CI, see python-trio/trio#1851 for example.
pquentin
force-pushed
the
enable-cleanup-closed
branch
from
e2b4fd0 to
7d7f8d4
Compare
Actually async only is fine because
I have a test currently running to make sure our nightly benchmarks also perform the same way. I also had to fight a bit with a timings issue that failed in CI because we sleeped a bit more than expected. I bumped the tolerance to address that. This is now ready for review. |
| @@ -5795,13 +5795,13 @@ async def test_adds_request_timings(self): | |||
| assert len(timings) == 3 | |||
|
|
|||
| assert timings[0]["operation"] == "initial-call" | |||
| assert math.isclose(timings[0]["service_time"], 0.1, rel_tol=0.05) | |||
There was a problem hiding this comment.
Do you suppose the change in enable_cleanup_closed somehow affected the RequestContextHolder's APIs' responsiveness? I don't have an issue in general with adding tolerance to approximation unit tests, but wondering if we're going to expect anything else to happen in service_times even for short-lived benchmarks.
For context, I haven't seen this test fail before
There was a problem hiding this comment.
No, RequestContextHolder is purely self-contained. I think it's just that when moving to pytest I made the test stricter.
Co-authored-by: Rick Boyd <[email protected]>
|
Should we add a note in |
Since this is phrased as a question: I don't think so as it's a bug fix like any other. |
|
My nightly test using nyc_taxis was succesful. This PR did not degrade performance (nor did it improve it). Thanks for the reviews, merging! |
This is now the default in Elasticsearch's Python client and leaving it
false causes memory leaks in Rally for long-running benchmarks.
Closes #1466