-
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
Fix suite info on daemonize #2044
Fix suite info on daemonize #2044
Conversation
1b37e2b
to
fe9b4e6
Compare
fe9b4e6
to
cdfb0f9
Compare
@@ -2060,7 +2064,6 @@ def will_pause_at(self): | |||
|
|||
def command_trigger_tasks(self, items): | |||
"""Trigger tasks.""" | |||
print "Trigger", items |
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.
Why is this being removed?
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 left behind debug print
statement. The command and its arguments should already be logged by the process_command_queue
method.
Review 2 Looks OK to me. No problems found from the test-battery in my environment. |
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.
One question; otherwise looks good and tests as working.
# Poll for suite log to be populated | ||
suite_pid = None | ||
suite_port = None | ||
while suite_pid is None or suite_port is None: |
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.
Should this loop time out after a while in case something stops the log file being written?
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.
Very unlikely, as the log file should at least have some errors in the 1st line - which would result in the loop exiting with an error.
However, just be on the safe side, I have now added a timeout after 5 minutes.
cylc run/restart command will now poll for suite log to be populated with the start up line so it can get the information on PID and port number and print them out correctly to the user. Incidentally, this also allows the suite to exit 1 if the suite does not start up correctly due to suite configuration errors.
e643593
to
e6f1f79
Compare
Branch re-based. |
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.
Good, no problems found.
cylc run/restart command will now poll for suite log to be populated
with the start up line so it can get the information on PID and port number and print them out correctly to the user.
Added suite PID to start up line in the suite log.
Incidentally, this also allows the suite to exit 1 if the suite does not
start up correctly due to suite configuration errors, etc.
Also migrated remaining useful logic in
cylc.suite_output
intocylc.daemonize
.Fix #2041. Fix #1720.