feat: sub agent use new supervisor [NR-497494]#2077
Conversation
f291c20 to
f813eb4
Compare
|
Needs conflict resolution (probably because we both added mocks to the supervisor module xD) |
Refactors the code with no behavior changes in order to make the following behavior change as easy as possible
f813eb4 to
39f75e8
Compare
|
That was it! Thanks for pointing out |
39f75e8 to
a5b8454
Compare
a5b8454 to
a865d50
Compare
There was a problem hiding this comment.
@DavSanchez I opted for the incoming implementation for the mocks because sub_agent_new.rs is already using them. They were mostly the same! But I can switch names or whatever if you think we should to 🙂
gsanchezgavier
left a comment
There was a problem hiding this comment.
I ended up doing a diff between sub_agent and sub_agent_new files locally 😅 .
Haven't check the tests yet but changes look good . I left a doubt
DavSanchez
left a comment
There was a problem hiding this comment.
Looking good! Some nits now that I'm getting to look at this code again. Feel free to not address any at all xD
Let me have another shot at the tests before approving.
| type AgentSupervisorStarter<B> = <B as SupervisorBuilder>::Starter; | ||
|
|
||
| type AgentSupervisor<B> = <AgentSupervisorStarter<B> as SupervisorStarter>::Supervisor; |
gsanchezgavier
left a comment
There was a problem hiding this comment.
great! i did a diff between the new and old sub-agents and foucus the review on trying to catch any behave issue but it looks all good to me! , i saw we could reuse most of the testing scenarios with some adaptation :) 🚀
Summary
This PR introduces a refactored version of
sub_agent.rsin a new temporary modulesub_agent_new.rs. The refactoring adopts the newSupervisortrait that unifiesSupervisorStopperfunctionality and adds support delegating to an existing supervisor the application of new configuration via theapply()method.Changes
Supervisor apply() usage
Previous behavior:
New behavior:
Supervisor::apply()apply()fails, theRemoteConfigis reported as failed and no supervisor keeps running (agents stop).Supervisor Lifecycle
Previous behavior:
New behavior:
Supervisor::apply()method is responsible for handling any necessary internal restarts.Small refactors and improvements
SupervisorCreationErroris kept oninit_supervisoronly.Notes for reviewers
Because the new behavior is introduced in a different module, this PR is best reviewed commit-by-commit.
The new module is not yet used and will replace
sub_agent.rsonce other dependent changes are ready.When the supervisors are ready we need to:
sub_agent.rswithsub_agent_new.rs