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

Support forking of workers (rake jobs:work NUM_PROCESSES=3) #565

Closed
wants to merge 1 commit into from

Conversation

wkonkel
Copy link

@wkonkel wkonkel commented Aug 21, 2013

Especially useful for running multiple workers on a single Heroku dyno worker.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 6419e25 on wkonkel:master into 812f10b on collectiveidea:master.

@albus522
Copy link
Member

albus522 commented Sep 4, 2013

Unfortunately it is not quite that simple. What happens when 1 or all of the workers die? Are signals properly handled in the forked processes?
The master process probably needs to continue running or heroku will think it finished and kill the dyno.

@wkonkel
Copy link
Author

wkonkel commented Sep 6, 2013

The master process has a "sleep" in it (line 21), so Heroku doesn't think it finished. However, your other points are valid. If I have some free time, I'll improve this pull request with:

  • Master process should periodically check that child processes are still around and if not, re-fork.
  • Master process should gracefully respond to SIGTERM. Currently the TERM is passed down to children who gracefully exit but the master process exits abruptly with a stack trace.

That said, we're successfully using this patch in our production environment and it's been working very well for us. We have the same worker throughput with 1/4th the dynos.

@dgobaud
Copy link

dgobaud commented Jan 9, 2014

This is really helpful. I added code to monitor and re-fork the children #615

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.

4 participants