Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added explanatory note to Horde.DynamicSupervisor #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gmdorf
Copy link

@gmdorf gmdorf commented Jun 23, 2024

@derekkraan This is a PR updating the docs to clarify the situation wrt using names vs pids (#269 (comment))

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
Copy link
Owner

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
Copy link
Author

gmdorf commented Jun 24, 2024

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
Copy link
Owner

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
Copy link
Author

gmdorf commented Jun 24, 2024

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
Copy link
Author

gmdorf commented Jun 24, 2024

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
Copy link
Owner

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
Copy link
Author

gmdorf commented Jun 24, 2024

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
Copy link
Owner

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
Copy link
Author

gmdorf commented Jun 24, 2024

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!

callahat added a commit to callahat/elixir-horde-swarm-exercises that referenced this pull request Jan 5, 2025
derekkraan/horde#271

Long story short, its not a good idea to nest Horde Supervisors in this way, probably a better approach
is have the "sub" processes attached to the main HordeSupervisor (via the project Module) and maybe
monitor that process in the "parent". Or just rethink needing them to be parent/child processes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants