Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

rescue Redis errors when reading and writing cache #150

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

opahk commented Oct 7, 2012

Hi there,

I've been bitten pretty badly with some redis connection errors other than ECONNREFUSED being raised for simple cache calls. IMHO these errors should be caught, as done in this pull request.

Any thoughts on this? I'm new to redis, so I'm unsure whether this is the right / idiomatic way to handle these errors.

Contributor

mhoran commented Oct 7, 2012

Redis::TimeoutError and Redis::CannotConnectError seem OK to rescue, but Redis::CommandError seems dangerous, and I wouldn't expect it under normal circumstances. It'd also be good to see some tests around this stuff as it's a bit brittle.

opahk commented Oct 12, 2012

Thanks for your comments - I'll look into the testing this. Redis::CommandError in my case was raised because of an invalid password. I agree that this shouldn't usually happen, but at the same time, it shouldn't to a 500 just because of a failing cache call.

Owner

jodosha commented Oct 12, 2012

Cache is part of the architecture, like a database is. You may want to be informed when something doesn't work, instead of having the illusion that it does. Plus, that exception is shared by all the others commands that we run against Redis, so catch it, is a really WRONG thing to do.

opahk commented Oct 17, 2012

Hi jodosha, thanks for your comment. I agree that it's important to be informed about these kind of errors, and there are various ways to do it, including monitoring or maybe using instrumentation. Maybe this is something that should be configurable?

Isn't it a reasonable requirement that the site shouldn't be down if the cache isn't available? In my setup at least, the cache is not a strictly necessary component. I'd rather have a slower site than no site at all. I'm wondering what the right way to handle this is then - I clearly can't wrap all my calls from within my app, that would be a great mess.

Any more thoughts?

Owner

jodosha commented Oct 18, 2012

Sorry, but it doesn't sound reasonable. Monitoring is something you should apply directly to your server, instead of having it in your cache code.

Cache is certainly part of the architecture but, in my practice at least, it is ancillary to the main functions of the application/environment. The environment can preform without it, slower, but it does not have to die. If my cache goes down I should still be able to provide service while working to get the cache reinstated. This is not about monitoring at the application level, you can still monitor at any level and the cache could go down all the same. This is about the application being able to easily survive in the event the connection to the cache breaks.

I like the idea of this being an option.

@jodosha jodosha closed this Jan 18, 2013

@mltsy mltsy referenced this pull request May 29, 2013

Closed

Redis Failure #175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment