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

Child batch #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

kballenegger
Copy link

Workers will now accept a PERCHILD env variable. This will process multiple jobs in each child to reduce the amount of forks and drastically improve performance. Not setting the variable, or having a value of 1, is equivalent to the previous behaviors.

Did not write unit tests, but did document in comments and did adhere to code style used.

Deployed this in production in our (chartboost.com) high-traffic (1k jobs per second) environment and is running smoothly so far. Will report back tomorrow if anything is new.

@jbrower
Copy link

jbrower commented Jan 14, 2013

Wouldn't this compromise the safety provided by forking all of the job processing into another thread? I'd be fairly nervous about losing the graceful handling of failed jobs. Am I misunderstanding this?

@danhunsaker
Copy link
Contributor

That's what I'm seeing here, too. Hey, @kballenegger - how far are we from the mark on this one?

@kballenegger
Copy link
Author

You're not wrong, it trades safety for scalability. Forks are extremely expensive.

I don't remember the specifics of how I'd implemented this though; it has been over a year. There may be better ways of doing this though internal acks via IPC or ZMQ sockets.

@Dynom
Copy link

Dynom commented Aug 31, 2013

This PR, unfortunately, won't apply cleanly anymore :-(

@kballenegger
Copy link
Author

@Dynom - I am not that surprised… it is over 2 years old!

@danhunsaker
Copy link
Contributor

I'd love to see this get pulled in to the new repo, resque/php-resque, if you wouldn't mind a rebase/rebuild? It would be good, IMO, to give users this option.

@kballenegger
Copy link
Author

Given I wrote this code almost 7 years ago now, and I haven't written a line of PHP since 2014, I'll opt out from working on this further. There didn't seem to be much willingness to merge this anyways in the previous 7 years.

@danhunsaker
Copy link
Contributor

Fair enough. I'll create an issue on the new repo (where things are moving forward quite rapidly at the moment) to get this implemented by someone else. Thanks for the original code, and the reply to my inquiry!

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