Processes construct their own supervision - #522
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
663cb89 to
2a5e575
Compare
2a5e575 to
5c1f0cc
Compare
5c1f0cc to
8c2033c
Compare
8c2033c to
9e9809f
Compare
danth
left a comment
There was a problem hiding this comment.
This looks like a good cleanup, just a few small tweaks 😄
9e9809f to
fdec3cd
Compare
f0e336a to
84780eb
Compare
…into feature/process-makes-supervision
32d14e6 to
1a33227
Compare
| const std::size_t supervised_components) | ||
| : m_recovery_client(recovery_client), m_config(config) | ||
| { | ||
| initResult = init(supervised_components); |
There was a problem hiding this comment.
It doesn't look like that AliveMonitorImpl can recover from initial init error, so why not just move the init() method to the constructor and let it throw exceptions?
There was a problem hiding this comment.
Hmm, I think we should avoid throwing exceptions where possible, I'd have to add throw statements to make this throw on some errors I think. I think all the create/construct/init methods in alive monitor should be removed/reworked, but this relates to #488 and is a larger change.
| buffer_(std::make_shared<SupervisionBufferType>()), | ||
| supervisionManager{std::make_unique<factory::FlatCfgFactory>()}, | ||
| processStateReader{std::move(f_observable_event_receiver)} | ||
| processStateReader{buffer_} |
There was a problem hiding this comment.
I am wondering if this variable is named correctly. Technical it is an ObservableEventReader, so observableEventReader_ would be more proper. From a semantic point of view it is a supervisionStateReader_, which I would like even more.
There was a problem hiding this comment.
Ah, that one must have missed a rename. I've named it as you suggested cefd51c
| /// @brief For fixed time-step execution during the cyclic execution | ||
| CycleTimer cycleTimer; | ||
|
|
||
| std::shared_ptr<SupervisionBufferType> buffer_; |
There was a problem hiding this comment.
Documentation is missing. BTW, the whole naming schema for members is currently a little bit mixed (member_variable vs. member_variable_). I do not think this needs to be fixed in this PR, but we should take time for that. Should I open an issue?
There was a problem hiding this comment.
Added a doc in cefd51c. Yeah that's worth opening an issue for I think, we need to decide on a format and stick to it (with a clang-tidy rule)
| /// @brief Set up alive supervision for the identified process. Alive supervision is not started until the publisher | ||
| /// is notified. |
There was a problem hiding this comment.
This took me a while until I understood that this is meant:
| /// @brief Set up alive supervision for the identified process. Alive supervision is not started until the publisher | |
| /// is notified. | |
| /// @brief Set up alive supervision for the identified process. Alive supervision is not started until the | |
| /// ISupervisionEventPublisher is notified via reportActivation(..) |
However I am also not sure if ISupervisionEventPublisher is the correct word for this interface at all.
| { | ||
| SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG_MESSAGE( | ||
| !supervisionManager.full(), "More alive supervisions than expected were constructed"); | ||
| supervisionManager.constructWorker(id, config, uid, recoveryClient, processStateReader); |
There was a problem hiding this comment.
Is it okay to ignore the return value here?
There was a problem hiding this comment.
Ah, thanks for spotting that, that is a problem! Handled properly in d590b6d
| const AliveSupervisionConfig& m_config; | ||
| std::thread alive_monitor_thread_{}; | ||
| std::atomic_bool stop_thread_{false}; | ||
| saf::daemon::EInitCode initResult{saf::daemon::EInitCode::kNotInitialized}; |
There was a problem hiding this comment.
Rename to ìnitResult_, also to not shadow initResultinAliveMonitorImpl::init(..)`
| saf::daemon::EInitCode initResult{saf::daemon::EInitCode::kNotInitialized}; | |
| saf::daemon::EInitCode initResult_{saf::daemon::EInitCode::kNotInitialized}; |
There was a problem hiding this comment.
Member has now been removed
| OsClock m_osClock{}; | ||
| UptrISupervisionControlReceiver m_observable_event_receiver; | ||
| const Config& m_config; | ||
| const AliveSupervisionConfig& m_config; |
There was a problem hiding this comment.
For new code it would be nice to stick to one naming schema.
| const AliveSupervisionConfig& m_config; | |
| const AliveSupervisionConfig& config_; |
| @@ -44,8 +44,9 @@ class ProcessInfoNode final : public IComponent | |||
| /// @brief Constructs a ProcessInfoNode. | |||
| /// @param config Configuration for the OS process. | |||
| /// @param index The process index within its process group. | |||
| /// @param index The process index within its process group. | ||
| /// @param ready_condition Whether this process is considered ready when running or when terminated. | ||
| /// @param process_handling The interfaces used to start, stop and report on the OS process. | ||
| /// @param supervision_factory Temporary reference to a factory this node can use to construct its supervision if |
There was a problem hiding this comment.
This interface does not describe an API which publishes anything. I need to think about what it is actually defining. Something like IProcessState? If you have better ideas, please go ahead.
There was a problem hiding this comment.
Renamed to "supervision state reporter", what do you think? 2290950
| LM_LOG_DEBUG() << "Setting up alive supervision for" << identifier_; | ||
|
|
||
| config_.deployment_config.environmental_variables.add( | ||
| "LCM_ALIVE_INTERFACE_PATH", aliveInterfacePath(identifier_)); |
There was a problem hiding this comment.
Could we remove the env var? Should maybe the constructSupervision just take it as a param?
There was a problem hiding this comment.
I don't think I can, the library still need to read the supervision path from somewhere. In #571 the factory will return the path to use though
…into feature/process-makes-supervision
Continuation of the refactoring started in #477. This PR moves alive supervision construction to the ProcessInfoNode constructor.
In addition, it:
SupervisionHandle, this is no longer neededgetTimeForReportsupervision_control_clientTODO
PhmDaemon::constructmethod. This really just needs to reserve the size of a vector and LM should calculate what size to reserve