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

prevents workers to stale when redis server went away #243

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

prevents workers to stale when redis server went away #243

wants to merge 4 commits into from

Conversation

jesusch
Copy link

@jesusch jesusch commented Apr 23, 2015

fix for #180

unfortunatelly I have no clue, why the first access on redis()->ping() does not throw the exception
anyhow this will prevent worker from getting useless after a network failure or redis restart

@danhunsaker
Copy link
Contributor

Throwing a CredisException instead of duplicating the error handling code would probably be a good idea.

That said, it's interesting that Credis itself doesn't handle this... The reason we don't is it's supposed to do it for us.

@jesusch
Copy link
Author

jesusch commented Apr 24, 2015

I've updated the pull according to your comment

@danhunsaker
Copy link
Contributor

Looks good to me.

I still find it odd that Credis doesn't handle that on its own, though. I wonder if it's handled correctly in a more recent version of the library...

@jesusch
Copy link
Author

jesusch commented May 19, 2015

any chance that this pull requests gets merged?

@jesusch
Copy link
Author

jesusch commented Jun 16, 2015

I had to update the commit due to missing constructor message on CredisException

@danhunsaker
Copy link
Contributor

👍

@chrisboulton
Copy link
Owner

I think in theory this should also be covered by #229, which was just merged down - essentially, the next operation against Redis on an instance that's gone away will cause the worker to throw an exception (Resque_RedisException)

danhunsaker added a commit to resque/php-resque that referenced this pull request Dec 11, 2018
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.

3 participants