-
Notifications
You must be signed in to change notification settings - Fork 94
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
Implement suite stalled handler. #1848
Conversation
@arjclark - this looks good, and the logic is simpler than my suggestion here #1286 (comment) 👍 |
@hjoliver - I think this now has everything needed, please review 1. |
@@ -339,6 +340,19 @@ \subsection{[cylc]} | |||
\item {\em default:} True | |||
\end{myitemize} | |||
|
|||
\paragraph[abort on stalled]{[cylc] \textrightarrow [[event hooks]] \textrightarrow abort on stalled} | |||
|
|||
If a this is set is set to True this will cause the suite to abort with |
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.
If this is set to ...?
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.
corrected
for itask in self.get_tasks(): | ||
if (itask.state.status in [TASK_STATUS_QUEUED, | ||
TASK_STATUS_READY] or | ||
itask.state.status in TASK_STATUSES_ACTIVE): |
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.
Is it just Github or does this indentation look funny?
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.
The indentation is funny in order to pass the pep8 test - complains if the row doesn't start exactly there...
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.
It is probably best to use an extra 4 spaces?
Looks OK 2. Test battery OK. Is it worth having a test for abort on stalled? (given that it is one of the main use cases.) |
@matthewrmshin - feedback addressed. |
Looks OK 2. Tests OK in my environment. |
@@ -858,6 +858,10 @@ \subsection{[cylc]} | |||
|
|||
\subparagraph[abort on timeout]{[cylc] \textrightarrow [[event hooks]] \textrightarrow abort on timeout} | |||
|
|||
\subparagraph[startup handler]{[cylc] \textrightarrow [[event hooks]] \textrightarrow stalled handler} | |||
|
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.
The first bit here should be "stalled handler".
Review 1: good, just a few doc typos to be fixed. |
@hjoliver - corrections made. |
Close #1286
First pass attempt at implementing a suite stalled handler.
This takes a look at the current task pool and if no tasks are active, submitting (ready), queued or awaiting a clock trigger then the suite is deemed to have stalled. Given the way the scheduler loop runs, the "am I stalled" check needs to indicate a stalled state for two consecutive loops in order to fire off the stalled handler/abort on stalled. This captures where a state has updated to succeeded/failed but the follow on task has yet to trigger (happening in the next scheduler loop).
@hjoliver - can you take an initial look through and see if you think I've missed anything? If you're happy then I'll start writing tests, documenting etc.