TurtleAI / derive

An Event Sourcing and CQRS solution.
0 stars 1 forks source link

Including a Registry inside of a DynamicSupervisor #7

Closed venkatd closed 2 years ago

venkatd commented 2 years ago

Hi @TurtleAI/eds

I've implemented a MapSupervisor https://github.com/TurtleAI/derive/blob/master/lib/derive/map_supervisor.ex#L24-L44

It's used to start or lookup processes for a given key.

However, if you notice, I'm passing in a registry as the 2nd argument. I'd rather encapsulate the Registry within the MapSupervisor so I don't have to think about declaring a registry elsewhere each time.

I guess I could setup a Supervisor as I'm seeing at the link below: https://elixir-lang.org/getting-started/mix-otp/dynamic-supervisor.html#supervision-trees

But it seems like a lot of boilerplate just for bundling a registry.

Would you recommend going this route, or are there better ways of doing this?

josevalim commented 2 years ago

Your option is correct! You could have a supervisor and you will define both MapSupervisor and the Registry as children of this supervisor.

There is another option which is to add a supervision tree to the derive application (run mix new foo --sup for a template or check this chapter). In your app supervision tree you can define a single registry and reuse it for all MapSupervisors. This should be fine given registries are scalable!

venkatd commented 2 years ago

Ok great! I went the Supervisor route (was hoping to keep the process tree self-contained) and things work as expected.

Does the implementation here look god to you? https://github.com/TurtleAI/derive/blob/master/lib/derive/map_supervisor.ex

Also I'm not sure if this is considered a hack, but I've forced the MapSupervisor to accept a name so I can reference Registry and DynamicSupervisor: https://github.com/TurtleAI/derive/blob/master/lib/derive/map_supervisor.ex#L64-L68

What are your thoughts on doing it this way?

josevalim commented 2 years ago

Yup, this is perfect.

There is one trick you can use which is to use :name for the Registry. This way you don’t need to build names during start_child on regular lookups. You only need to build the name for the dynamic supervisor when lookup fails and you need to start something.

You can even store the dynamic supervisor name under the :meta key of the registry to avoid building names on start_child altogether.

venkatd commented 2 years ago

Great, I've went ahead with your suggestions so the names are built once at the beginning:

  def init(opts) do
    name = Keyword.fetch!(opts, :name)
    dynamic_supervisor = :"#{name}.DynamicSupervisor"

    children = [
      {Registry, keys: :unique, name: name, meta: [dynamic_supervisor: dynamic_supervisor]},
      {DynamicSupervisor, strategy: :one_for_one, name: dynamic_supervisor}
    ]

    Supervisor.init(children, strategy: :one_for_all)
  end

Then later on...

        {:ok, dynamic_supervisor} = Registry.meta(registry, :dynamic_supervisor)
        via = {:via, Registry, {registry, key}}
        opts = Keyword.put(opts, :name, via)

        case DynamicSupervisor.start_child(dynamic_supervisor, {mod, opts}) do
          {:ok, pid} -> pid
          {:error, {:already_started, pid}} -> pid
        end

Things are much simpler now!