Re-architecture of Alice's supervision tree structure #85
Conversation
adamzaninovich
changed the title
adamzaninovich
changed the title
| @@ -0,0 +1,15 @@ | |||
| defmodule Alice.Adapter.Supervisor do | |||
| @moduledoc """ | |||
| Supervises any Alice.Adapter process that are started. | |||
There was a problem hiding this comment.
processes instead of process. 😃
| if Process.alive?(pid) do | ||
| %{adapters: adapters, handlers: handlers} = :sys.get_state(pid) | ||
| for adapter <- adapters do | ||
| Supervisor.terminate_child(Alice.Adapter.Supervisor, Process.whereis(adapter)) |
There was a problem hiding this comment.
Could Supervisor.stop (docs here) with Alice.Adapter.Supervisorbe used to avoid the loop?
There was a problem hiding this comment.
I'll look into it, that would be much better. This is what I had to do to get rid of that error messages after the tests run, and I'm all for simplifying it.
| Supervisor.terminate_child(Alice.Adapter.Supervisor, Process.whereis(adapter)) | ||
| end | ||
| for handler <- handlers do | ||
| Supervisor.terminate_child(Alice.Handler.Supervisor, Process.whereis(handler)) |
There was a problem hiding this comment.
Similar question here for the handler supervisor.
|
|
||
| def start_link(_) do | ||
| Supervisor.start_link([ | ||
| worker(Alice.Adapter, [], restart: :transient) |
There was a problem hiding this comment.
We may want to consider converting this to a module based supervisor, [per the docs here] (https://hexdocs.pm/elixir/Supervisor.html#module-module-based-supervisors) particularly since this is a simple_one_for_one supervisor. This will be important in particular if we want to support hot code-reloading as a deployment strategy (and I think we do). This change seems pretty simple, as it means moving most things to the init/1 callback rather than start_link/2.
There was a problem hiding this comment.
Since I wasn't doing anything in init for now I used the simplified version, but I think you're right. It will leave it more open for change and doesn't add much, if any, extra complexity.
| def start_link(_) do | ||
| Supervisor.start_link([ | ||
| worker(Alice.Bot, [], restart: :transient) | ||
| ], strategy: :simple_one_for_one, name: __MODULE__) |
There was a problem hiding this comment.
Same comment here about going with a module based supervisor. I also don't think we need a simple_one_for_one supervisor here. We may have discussed why a straight one_to_one wouldn't work, but i don't remember why that is.
There was a problem hiding this comment.
I left it SOFO because the bot process is started by the user's bot project after the entire Alice tree starts. Using SOFO, we don't have to double-supervise the bot process.
There was a problem hiding this comment.
However, having said that, I'm not 100% happy with having to start the bot there kind of manually, so maybe that's something we still have to figure out.
|
I feel like all the process manipulation you're doing in the test is indicative of a larger problem in the setup of the supervision trees. Perhaps it's just a thing where the tests need to run synchronously. Either way, that process manipulation feels like a smell but I also don't have a good way to counteract it ATM, so it might as well stay for now. |
|
@mattr- thanks for the review! I agree about the process manipulation for the tests. It feels a bit dirty. I think I'm ok with A2 being rough around the edges in general at this stage as long as we're mindful about improving as we iterate on it before release. I feel pretty confident that we can find ways to keep improving this. |
Same. I just wanted to make sure I said something about it explicitly vs. letting it rot away. 😃 |
Tests are passing now
This change restructures the OTP supervision tree of the Alice application. You can now have multiple adapters. Also, your Bot process is inserted into the main Alice supervision tree without the need to double-supervise it in your bot application.
Before
After