Implement new control API - #489
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
| LM_LOG_DEBUG() << "Graphs initialized"; | ||
| } | ||
|
|
||
| void ProcessGroupManager::offerService() |
There was a problem hiding this comment.
I would recommend to eventually move all the interaction handling with lmcontrol service and all the mw::com parts to a separate class and create an interface for the ProcessGroupManager class.
In case we want to exchange the underlying IPC implementation at some point it will be good if this is cleanly isolated in the code and e.g. the ProcessGroupManager does not need to be changed.
| auto instance = std::move(instance_result).value(); | ||
|
|
||
| const auto register_result = instance.activate_run_target.RegisterHandler( | ||
| [this](ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) { |
There was a problem hiding this comment.
I think all the code in Graph should only run on main thread to avoid the complexity of multi threaded code there.
One idea would be to post an event in the event queue when a request arrives so it can be processed on the main thread.
Then the result can be provided back to some "control" class that implements the mw::com communication
| } | ||
| else | ||
| { | ||
| event->activation_source = RunTargetActivationSource::kStateManagerRequest; |
There was a problem hiding this comment.
Hi @danth, nice to see the daemon side of ActivationResult publishing land.
One thing I noticed while reading through: RunTargetActivationSource::kRecoveryAction doesn't seem to be set anywhere. Graph::finalizeTransitionSuccess() only branches on is_initial_state_transition_, so a transition triggered by ProcessGroupManager::processGroupHandler()'s recovery path (pg.startTransition(recovery_state.pg_state_name_)) would currently report as kStateManagerRequest, even though no State Manager actually requested it.
Was this intentional for this pass (e.g. deferred to a follow-up), or is it worth threading a "recovery-triggered" signal from that call site into Graph before this merges — maybe a dedicated entry point similar to startInitialTransition()? Happy to help if useful.
There was a problem hiding this comment.
Was this intentional for this pass
Yes, I am still working on this. It will be implemented before the PR is marked as ready. :))
This pull request adapts the launch manager (and tests) to use the new control API based on
mw::com, and removes the old API.Fixes #481.