Added fault tolerant caching and fixed a couple bugs. #81

Closed
wants to merge 6 commits into
from

Projects

None yet

5 participants

@clabrunda

@mszenher , @fosdev , @pmavinkurve-mdsol

Added fault tolerance and a spec to test it.

...................................................................................................................................................................................................................................................................
Finished in 0.456608 seconds.

259 tests, 917 assertions, 0 failures, 0 errors

@fosdev

I would bracket the end as well if you feel it is important to reference this.

I left it in by accident, it was just to help me find my new test while I was working on it.

@fosdev

Do we really want to add this dependency? Seems to me that if you are modifying errors that can be caught, it would be better to just add some kind of configuration of that, if it can be done.

Yeah, we can't assume this'll be used in a faraday context as this gem can be used for rack-middleware as well as faraday middleware.

@fosdev

Do you need to include the zero. If headers['Age'] is nil that will give a zero and in the logic before, if there is a 'Age' header, it wins regardless and presumbably the 0 corresponses to nil.

Also, you don't need max.to_i since everything will already be an integer.

The 'Age' header will generally be zero (assuming no intermediate caches), so it usually won't win. The zero is for the case where our client computer's time is not in sync to the server's, and we get a date header from the server that is in the future. It seemed like a bad idea to have a negative age.

@fosdev

Suggested: whitespace +=

@fosdev

Indentation here and below

@mszenher mszenher commented on an outdated diff Apr 18, 2013
@@ -24,6 +24,8 @@ caching solution for small to medium sized deployments. More sophisticated /
high-performance caching systems (e.g., Varnish, Squid, httpd/mod-cache) may be
more appropriate for large deployments with significant throughput requirements.
+This Medidata fork of Rack::Cache adds fault tolerant caching, so that stale cached results can be returned if the downstream service is unavailable.
@mszenher
mszenher Apr 18, 2013

Remove mention of "Medidata fork" from README

@mszenher mszenher commented on an outdated diff Apr 18, 2013
test/context_test.rb
@@ -349,6 +349,42 @@
cache.trace.should.include :store
end
+ #New from clabrunda
@mszenher
mszenher Apr 18, 2013

Remove this comment

@mszenher mszenher commented on the diff Apr 18, 2013
lib/rack/cache/context.rb
@@ -159,7 +160,9 @@ def invalidate
# found and is fresh, use it as the response without forwarding any
# request to the backend. When a matching cache entry is found but is
# stale, attempt to #validate the entry with the backend using conditional
- # GET. When no matching cache entry is found, trigger #miss processing.
+ # GET. If validation fails due to a timeout or connection error, serve the
+ # stale cache entry anyway. When no matching cache entry is found, trigger
@mszenher
mszenher Apr 18, 2013

I think the consumer of this middleware should decide (ideally for each request) whether or not they want to fail-over to stale cache.

@fosdev
fosdev commented Apr 18, 2013

@clabrunda Somehow I ended up commenting on commit in a PR here: mdsol@0b3add8

@mszenher What do you think about this: mdsol@0b3add8#commitcomment-3040372

Sorry, for the confusion.

@mjobin-mdsol

anyone to review this for inclusion in main rack-cache repo ?

@mjobin-mdsol

@rtomayko would you please review and consider merging?

@mszenher

@clabrunda Please close this PR as I've made a PR for another, more recent version of our code. Thanks.

@grosser
Collaborator
grosser commented Oct 8, 2015

closing sine another PR includes this and more

@grosser grosser closed this Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment