-
-
Notifications
You must be signed in to change notification settings - Fork 720
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
Don't consider scheduler idle while executing Scheduler.update_graph
#8877
Changes from 6 commits
3cfbe01
d49f35d
b08a33e
dd92bc5
ed1d8bc
1150c41
40f3a35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -2389,6 +2389,34 @@ async def test_idle_timeout(c, s, a, b): | |||
pc.stop() | ||||
|
||||
|
||||
@gen_cluster(client=True) | ||||
async def test_idle_during_update_graph(c, s, a, b): | ||||
class UpdateGraphTrackerPlugin(SchedulerPlugin): | ||||
def start(self, scheduler): | ||||
self.scheduler = scheduler | ||||
self.idle_during_update_graph = None | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Non-blocking nit] This is totally fine as is but maybe don't even assign
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've slightly adjusted this such that |
||||
|
||||
def update_graph(self, *args, **kwargs): | ||||
self.idle_during_update_graph = self.scheduler.check_idle() is None | ||||
hendrikmakait marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
await c.register_plugin(UpdateGraphTrackerPlugin(), name="tracker") | ||||
plugin = s.plugins["tracker"] | ||||
# The cluster is idle because no work ever existed | ||||
assert s.check_idle() is not None | ||||
beginning = time() | ||||
assert s.idle_since < beginning | ||||
await c.submit(lambda x: x, 1) | ||||
# The cluster may be considered not idle because of the unit of work | ||||
s.check_idle() | ||||
# Now the cluster must be idle | ||||
assert s.check_idle() is not None | ||||
end = time() | ||||
assert beginning <= s.idle_since | ||||
assert s.idle_since <= end | ||||
# Ensure the cluster isn't idle while `Scheduler.update_graph` was being run | ||||
assert plugin.idle_during_update_graph is False | ||||
|
||||
|
||||
@gen_cluster(client=True, nthreads=[]) | ||||
async def test_idle_timeout_no_workers(c, s): | ||||
"""Test that idle-timeout is not triggered if there are no workers available | ||||
|
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.
Just noting that this
finally
block is being used to ensure we always decrement the_active_graph_updates
counter