-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Fix leak in test_localcluster_get_client
#6817
Fix leak in test_localcluster_get_client
#6817
Conversation
Note: We may want to add an |
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ±0 15 suites ±0 6h 49m 29s ⏱️ + 22m 11s For more details on these failures and errors, see this check. Results for commit 4489298. ± Comparison against base commit 9267a21. |
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 looks like a good change to me. But I'm not sure how it would affect other tests besides test_localcluster_get_client
? Are you saying it would address the RuntimeError: cannot schedule new futures after shutdown
issue from #6745 (comment)? Because I'm not sure how it would affect #6796 (but I may be missing something).
Going to merge this now regardless since |
To be honest, my assumption was based on correlation rather than an understanding of causality. What I observed was an oddly behaving test that kept the test suite busy for a while after passing and then returned a pretty long error output as well as a sudden uptick in CI failures once this test had been merged into |
Fixes issue mentioned in #6745 (comment).
Likely fixes:
distributed.deploy.tests.test_spec_cluster::test_adaptive_killed_worker
#6794distributed.deploy.tests.test_spec_cluster::test_restart
#6795assert False, (bad_thread, call_stacks)
- Worker executor thread still running afterNanny.kill
#6796which started occurring once #6745 was merged.
pre-commit run --all-files