search

derekkraan / horde

Horde is a distributed Supervisor and Registry backed by DeltaCrdt
MIT License
1.32k stars 106 forks source link

added explanatory note to Horde.DynamicSupervisor #271

Open gmdorf opened 5 months ago

gmdorf commented 5 months ago

@derekkraan This is a PR updating the docs to clarify the situation wrt using names vs pids (https://github.com/derekkraan/horde/issues/269#issuecomment-2178012113)

Additional relevant discussion has been added to this old thread: https://elixirforum.com/t/horde-dynamicsupervisor-terminate-child-is-not-working/61558/12

I am still curious to know if it is possible to have Horde.DynamicSupervisor children started at runtime as children of a parent Horde.DynamicSupervisor, and have each parent and child supervisor behave correctly as expected?

derekkraan commented 5 months ago

Thanks for this. I want to improve a bit on this before it goes into main, so I'll hold off on merging it until I have time to make the required changes.

gmdorf commented 5 months ago

Yeah, I was thinking that it would also be good to clarify the status of :via tuples. AFAIK they don't work?

My boss and I are also scratching our heads trying to figure out if it's possible to nest Horde.DynamicSupervisor. No luck so far.

derekkraan commented 5 months ago

It doesn't make sense from my perspective to nest Horde.DynamicSupervisor. My advice would be that if you are considering that, you should likely head back to the drawing board.

I'm not sure what the status is on :via tuples for Horde.DynamicSupervisor, but they should work for children of Horde.DynamicSupervisor. There is the caveat that lookups are eventually consistent, until we figure out a way to fix that for local lookups. (See the people struggling with this in #267).

If you're asking about :via tuples for naming Horde.DynamicSupervisor, it sounds like this might be related to nesting, and so my advice would be the same as above: consider if this is really the best solution to your problem.

gmdorf commented 5 months ago

Yeah, we've reached the same conclusion on our own.

Can you elaborate more on the reasons why it doesn't make sense to nest Horde.DynamicSupervisor unlike the stdlib Supervisor and DynamicSupervisor?

Thinking it might be good to add to the docs so that future users don't get confused.

gmdorf commented 5 months ago

Otherwise, do you see any issue with dynamically adding and removing Horde.DynamicSupervisors to a cluster at runtime if they are not added as children of a Horde.DynamicSupervisor?

derekkraan commented 5 months ago

Horde.DynamicSupervisor is spreading your processes (or sub-trees of processes) evenly across your nodes. What could be the benefit of doing this twice?

Re: adding and removing dynamically at runtime, I would likely just run all of them all the time. An idling Horde.DynamicSupervisor doesn't take up much resources.

gmdorf commented 5 months ago

Horde.DynamicSupervisor is spreading your processes (or sub-trees of processes) evenly across your nodes. What could be the benefit of doing this twice?

Ah, I see.

Re: adding and removing dynamically at runtime, I would likely just run all of them all the time. An idling Horde.DynamicSupervisor doesn't take up much resources.

It's not about conserving resources. It's about not knowing which Horde.DynamicSupervisors will be needed. We have an interface by which the user of our custom IDE can add new Horde.DynamicSupervisors as desired.

derekkraan commented 5 months ago

In this case I would see if it would also solve your problem to run it all under a single Horde.DynamicSupervisor.

The issue with starting and stopping them at runtime is that in order to have an actual benefit from Horde.DynamicSupervisor, you need it to be running on multiple nodes. If you do this dynamically, then you add a bunch of issues. What happens when a new node joins the cluster for example? Or leaves and then comes back, how do you ensure that the list of Horde.DynamicSupervisors are coordinated correctly?

Nesting Horde.DynamicSupervisor doesn't help you here, since each child Horde.DynamicSupervisor would be run on a single node. As I mentioned above, it needs to run on multiple nodes, otherwise you would be better just nesting a regular DynamicSupervisor as a child to your Horde.DynamicSupervisor.

gmdorf commented 5 months ago

just nesting a regular DynamicSupervisor as a child to your Horde.DynamicSupervisor.

This seems to make the most sense for my use case.

Thanks Derek! You've been incredibly helpful and made a groundbreaking piece of technology free to use besides!