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

Stop randomize_child_id when handoff #226

Closed

Conversation

kunna
Copy link

@kunna kunna commented Apr 7, 2021

resolves #207

Debug

Following the instruction from #207, I noticed that new processes are keep added to the DeltaCrdt.read(Conv.ConversationSupervisor.Crdt). Whenever a new process is handed off to another node, a duplicate record is created, with a new child.id

Change

By removing the randomize_child_id during the handoff process, I was able to fix this issue.

Question

I try to understand why we needed to add randomize_child_id and found this PR. #196
This PR says, "Deduplication will be the sole responsibility of Horde.Registry", but I was not able to find the logic from the Horde.Registry fixing the duplicated processes.

I think PR #196 was meant to remove Horde.ProcessesSupervisor.terminate_child_by_id. https://github.com/derekkraan/horde/pull/196/files#diff-36c0ba7f42365d4eeaa857f4b899986cb28d29ff94de730fd8c1934c872a0389L476

because this change stoped terminating the old process, we need a consistent id for Horde.Registry to prevent creating a duplicated process.

randomize_child_id adds a new (duplicated) process to DynamicSupervisor.DeltaCrdt
which results to a keep growing list of processes
@kunna kunna changed the title Remove randomize_child_id when handoff Stop randomize_child_id when handoff Apr 7, 2021
@kunna kunna closed this Feb 6, 2022
@bernardo-martinez
Copy link

this makes sense to solve #207 can i know why was this closed?

@arjan
Copy link
Contributor

arjan commented Nov 16, 2022

I agree.

without this change it is effectively not possible to run a Horde.DynamicSupervisor without a Horde.Registry...

@derekkraan
Copy link
Owner

I am not sure if it is possible to run a Horde.DynamicSupervisor without a Horde.Registry even with this change.

@arjan
Copy link
Contributor

arjan commented Nov 16, 2022

Not sure if I follow.. why would that be? Assuming that this change fixes the process duplications on membership changes..

@derekkraan
Copy link
Owner

@arjan because there are other situations in which you can end up with duplicate processes. A netsplit, for example.

@derekkraan
Copy link
Owner

In a previous version, Horde.DynamicSupervisor was deduping processes, but this resulted in race conditions with Horde.Registry, resulting in both processes being removed sometimes. That's why Horde.DynamicSupervisor doesn't dedupe processes anymore.

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.

Removing nodes creates multiple processes out of 1 process
4 participants