Improved forking implementation #364
Conversation
bestie
requested review from
dasch,
leonmaia,
deepredsky,
bquorning and
HeyNonster
|
Would this replace the existing parallel workers setup? If not, why have two? |
|
I think it should replace it, but for now I went with the lowest friction option of complete separate feature. There are differences in behavior on error and when processes die unexpectedly, so I was imagining we could deprecate current 'parallel workers' implementation and remove it in 3.x. If we're up for messing with an 'experimental' in a minor release I do not object. |
|
Probably up to the owning team, but it should at least be documented that they're two implementations of the same feature, one of them experimental (for now). |
| pids.each do |pid| | ||
| unless worker_running?(pid) | ||
| $stdout.puts("A forker worker has exited. Shuttin' it down ...") | ||
| stop |
There was a problem hiding this comment.
Are we killing the ForkingRunner here and all of its children if one child exits? Maybe I'm following that incorrectly.
There was a problem hiding this comment.
Yes, it would unexpected for this to happen, so rather than try to recover from a potentially bad state the whole thing shuts down so Kubernetes can bring up a new replica.
I'm not against trying to recover and replace a lost worker process but that is quite a bit more complex.
* Set config value `forks` instead of `parallel_workers` * Supervisor process forks worker `n` processes * Supervisor terminates workers when it is stopped * Workers monitor supervisor and terminate themselves if supervisor exits to prevent zombie processes * Supervisor runs pre-fork hook before forking * Workers run post-fork hook immediately after forking
bestie
force-pushed
the
alternative-forking
branch
from
c8cff9e
to
7cc3dc4
Compare
bestie
changed the title
forksinstead ofparallel_workersnprocesses