search

derekkraan / horde

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

Stop randomize_child_id when handoff #226

Closed kunna closed 2 years ago

kunna commented 3 years ago

resolves https://github.com/derekkraan/horde/issues/207

Debug

Following the instruction from https://github.com/derekkraan/horde/issues/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. https://github.com/derekkraan/horde/pull/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.

bernardo-martinez commented 2 years ago

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

arjan commented 2 years ago

I agree.

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

derekkraan commented 2 years ago

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

arjan commented 2 years ago

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

derekkraan commented 2 years ago

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

derekkraan commented 2 years ago

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.