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

Fix flaky test_oversubscribing_leases #3726

Merged
merged 2 commits into from
Apr 18, 2020

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Apr 18, 2020

This should finally fix #3717 by introducing two changes

  • A new method get_value to observe the state. This eliminates any timing based assumption on the oversubscription but allows us to wait until that condition is given for certain
  • For some reason, the PeriodicCallback start/stop within the test did not work as expected. I got stop the callback, I could start it again but for whatever reason it was not doing anything. That left the entire test in more or less random state and I'm wondering why it worked in the first place. To eliminate the randomness I control the periodic callback via the refresh_leases variable now instead of start-/stopping it. Also, I explicitly verify that the internals do what they are supposed to do by capturing the logs. Has anybody else encountered a PC behaviour like this before? Is start/stop/start something we are not supposed to do with PCs?

I couldn't reproduce the race condition in the first point but I am now more confident that the test is doing what it was intended to do when running it locally and I don't see any more room for race conditions atm.

Sorry for the flaky test, I hope this fixes the issue now

cc @mrocklin

@fjetter fjetter changed the title Semaphore/get value Fix flaky test_oversubscribing_leases Apr 18, 2020
@mrocklin mrocklin merged commit 5c027a6 into dask:master Apr 18, 2020
@mrocklin
Copy link
Member

Thank you for following up on this @fjetter ! I appreciate it.

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.

Test failure in test_oversubscribing_leases
2 participants