sorentwo / readthis

:newspaper: Pooled active support compliant caching with redis
MIT License
504 stars 40 forks source link

Disable caching when redis is down #26

Closed dmitrypol closed 8 years ago

dmitrypol commented 8 years ago

I am using readthis for Rails caching with shared ElasticCache Redis instance in AWS. I'd like to be able to serve uncached content if Redis goes down (better slow site than dead one).

Any suggestions on how to do it?

sorentwo commented 8 years ago

I've been thinking about this a bit for the past couple of days. There are numerous ways to hack/monkey patch Readthis to operate if Redis goes down, but that is far from ideal. Really this should be a first-class feature of the library (for fetch/write type operations at least).

I'll keep you informed as this is worked on.

dmitrypol commented 8 years ago

Thank you very much. I implemented a monkey patch recommended for redis-store. But it would be really nice to have a proper implementation.

sorentwo commented 8 years ago

@dmitrypol Would you mind taking a look at #30?

dmitrypol commented 8 years ago

Hi Parker

Thank you very much for implementing this fix. I pointed my Gemfile to commit fed72c7eb30dafc80579ac8c7a1491e21acc6cf7.
Here is my production.rb: config.cache_store = :readthis_store, { expires_in: 1.hour.to_i, namespace: 'amplo', redis: { host: config.redis_host, port: 6379, db: 0 }, driver: :hiredis }

In local dev I simply shutdown redis and it worked great. In production I did several things to test this.

When I changed redis connection string I got this exception: getaddrinfo: Name or service not known. I think that's reasonable.

But when I removed AWS Security Group firewall port 6379 and flushed keys in redis DB I would sometimes get: cannot load such file -- o: ActiveSupport::Cache::Entry: @valuef 10849:@created_atf1449685419.7098632:@expires_inf 3.6e3 in my method cache calls I have these: Rails.cache.fetch("#{cache_key}/#{method}", expires_in: 1.hour) do

Looking at your PR I see rescue Redis::BaseConnectionError => error. But the error message I got above feels like CommandError. Could that be impacted by the fault_tolerant flag?

Thanks, Dmitry

sorentwo commented 8 years ago

@dmitrypol From the redis errors file it looks like BaseConnectionError covers everything except for CommandError and ProtocolError, which are probably reasonable to catch as well. Granted that the command set Readthis uses is limited, it won't swallow unintentional/unrelated runtime errors.

sorentwo commented 8 years ago

@dmitrypol I pushed a change to the fault tolerance branch, would you mind checking again? That's about as loose as I can make the error handling.

dmitrypol commented 8 years ago

Wow, talk about a fast response. Yes, I see your change and I agree that going to Redis::BaseError is looser but can present additional risks. I will deploy this fix to our prod and let you know how it goes tomorrow.

dmitrypol commented 8 years ago

No issues so far in prod. Thank you again for implementing the fix and for writing your blog. Lots of great ideas.

sorentwo commented 8 years ago

Awesome, thanks for the thorough testing! I'm going to merge the PR then.