Support suite UUID in suite/task event handlers #2902
Conversation
|
@matthewrmshin - thanks for implementing this! Taking a look now... |
There was a problem hiding this comment.
Code looks good! +1 Going to try to test it now in my environment.
Maybe later we could review the object dependency in Cylc...
A SuiteEventsManager now has a strong dependency to TaskEventsManager. i.e. if you write a SuiteEventsManager, your need a TaskEventsManager object first.
But SuiteEventsManager actually needs the SuiteProcPool and Suite UUID (task_events_mgr.task_remote_mgr.uuid_str). This makes writing tests - especially unit tests - harder (and sometimes forcing you to use mock unnecessarily).
But also makes re-factoring / debugging a bit more complicated. In Java there's an OO principle, which I can't recall the name. And it tells you to look for a pattern like:
class ServiceA {
void doSomething(ServiceB serviceB) {
String value = serviceB.serviceC.value;
}
}And kill the dependency between ServiceA and ServiceB, and instead leave only ServiceA and ServiceC, or only ServiceA and the member value that you need.
Normally this principle is used an introduction to dependency injection, as by following this pattern, it becomes much easier to decouple classes and use DI.
End of digression (and maybe of the Java-ism) 😬
lib/cylc/suite_events.py
Outdated
| @@ -37,7 +37,7 @@ class SuiteEventError(Exception): | |||
| ["event", "reason", "suite", "owner", "host", "port"]) | |||
|
|
|||
|
|
|||
| class SuiteEventHandler(object): | |||
| class SuiteEventsManager(object): | |||
| """Suite event handler.""" | |||
There was a problem hiding this comment.
"""Suite event manager."""
? as in task_events_mgr.py and task_job_mgr.py
|
(Yes, our "manager" classes have multiplied since first introduced, and have become a bit entangled.) |
There was a problem hiding this comment.
Tested with the following suite:
#file: uuid1/suite.rc
[cylc]
[[events]]
handlers = echo 'My UUID is %(suite_uuid)s'
handler events = startup
[scheduling]
[[dependencies]]
graph=t1
After registering, a cylc run uuid1 produced the following log:
2018-12-13T16:20:02+13:00 INFO - Suite server: url=http://kinow-VirtualBox:43035/ pid=6128
2018-12-13T16:20:02+13:00 INFO - Run: (re)start=0 log=1
2018-12-13T16:20:02+13:00 INFO - Cylc version: 7.8.0-3-gc7e72
2018-12-13T16:20:02+13:00 INFO - Run mode: live
2018-12-13T16:20:02+13:00 INFO - Initial point: 1
2018-12-13T16:20:02+13:00 INFO - Final point: 1
2018-12-13T16:20:02+13:00 INFO - Cold Start 1
2018-12-13T16:20:02+13:00 INFO - [t1.1] -submit-num=1, owner@host=kinow-VirtualBox
2018-12-13T16:20:02+13:00 INFO - [('suite-event-handler-00', 'startup') cmd] echo 'My UUID is 01ed0ea0-ebad-49ca-9447-4bac59d49f9e'
[('suite-event-handler-00', 'startup') ret_code] 0
[('suite-event-handler-00', 'startup') out] My UUID is 01ed0ea0-ebad-49ca-9447-4bac59d49f9e
2018-12-13T16:20:03+13:00 INFO - [t1.1] -(current:ready) submitted at 2018-12-13T16:20:02+13:00
2018-12-13T16:20:03+13:00 INFO - [t1.1] -health check settings: submission timeout=None
2018-12-13T16:20:03+13:00 INFO - [t1.1] -(current:submitted)> started at 2018-12-13T16:20:02+13:00
2018-12-13T16:20:03+13:00 INFO - [t1.1] -health check settings: execution timeout=None
2018-12-13T16:20:04+13:00 INFO - [t1.1] -(current:running)> succeeded at 2018-12-13T16:20:03+13:00
2018-12-13T16:20:04+13:00 INFO - Suite shutting down - AUTOMATIC
2018-12-13T16:20:04+13:00 INFO - DONE
Which includes the UUID from the sqlite DB table suite_params.
👏 🎉
|
@kinow You are right about the dependency issue. (I should have known better!) I'll reimplement this change without introducing extra big dependency. (Testing in my local environment now.) |
|
@matthewrmshin added that comment more as a side note, as I think the code is really good and ready to be merged (unless you'd like to fix the comment). I only noticed the dependency because I stared (literally gawked) at the code for over 15 minutes thinking "If I had to implement it... could I do it now?"... alas I could understand how to test it after reading the tests (and I think it's related to a presentation of someone from Melbourne that needed it?) 😞 but wouldn't know how to implement it yet. But eventually I will be able to implement changes like this and help more! 🎉 |
c7e7223 to
e4220cc
Compare
|
Should all be good. |
|
That does look cleaner! Merging... |
Close #2901.