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

A robust, hopefully production-ready caching implementation #31

Merged
merged 12 commits into from
Jun 14, 2013

Conversation

fancyremarker
Copy link
Contributor

to: @dblock
cc: @joeyAghion

Sorry for the big PR. I promise the next one will be more concise. The big change here was replacing the CacheKey strategy with a more robust SafeCacheKey strategy (better name suggestions welcome 😉) which provides the same (or better) performance than Garner 0.3.3. The CacheKey strategy was OK — in fact, it fully mirrored the caching performance of Rails 4 — but it suffered from two critical flaws:

  • The timestamp in default cache keys only has second precision. (Can you believe this?) In other words, if a document is cache-bound and then written to within a single second, the cache result will always be stale. This pull request adds nanosecond precision when using the SafeCacheKey strategy.
  • Mongoid (and ActiveModel) classes don't have cache_key defined. So you can't bind to a class. This pull request solves that by proxying Model.cache_key to Model.latest.cache_key.

Some more minor, or structural changes:

  • Separated callback invalidations from explicit invalidations via invalidate_garner_caches. Motivation: In the specific case of the Touch invalidation strategy, it's redundant and even possibly harmful for us to call touch on documents during Mongoid callbacks.
  • Added a spec for the potential race condition brought up by @dblock in Proposed Garner 0.4.0 (a.k.a 1.0.0-alpha) #28. Because SafeCacheKey always uses the actual database state as truth, it probably wasn't an issue here, but could very well be an issue for other strategies, like the IdentityMap strategy I'm working on. So thanks again for bringing up the corner case.
  • Rewrote all strategies as classes, not models, inheriting from respective Base classes. There's a minor need now (see apply_on_callback?), and conceivably a major need later, for inheritable behavior in strategy groups.
  • Fixed Coveralls, and made it only run in CI.

Pending any changes you guys suggest, I think this is ready for production. (Famous last words.) Let's :shipit:!

@ghost ghost assigned dblock Jun 14, 2013
@dblock
Copy link
Contributor

dblock commented Jun 14, 2013

This one change that fixes the two issues you describe is extremely blog-worthy. It clarifies in a major way why the next version of Garner is so much better than the previous version of Garner.

You could call SafeCacheKey something like SRTCacheKey. That's Sphincter Reaction Time, "the small amount of time between the urge to pass gas and the determination that there could be solid or liquid in said gas passage", according to Urban Dictionary on nanosecond.

dblock added a commit that referenced this pull request Jun 14, 2013
A robust, hopefully production-ready caching implementation
@dblock dblock merged commit f405bf5 into artsy:master Jun 14, 2013
@mzikherman
Copy link
Contributor

Wow, re: SRT -
screen shot 2013-05-24 at 12 58 53 am

@joeyAghion
Copy link
Contributor

I like SafeCacheKey, although @dblock's argument is compelling.

The latest method is very generic-sounding, considering the scope of classes it will impact. As a specific example, I'm pretty sure it will conflict with a similar method in our code base (so may or may not function as designed, depending on module-inclusion order).

@fancyremarker
Copy link
Contributor Author

@joeyAghion: Sorry, latest was pseudocode. I didn't actually define that method.

@dblock: I wasn't sure about SRT at first, but I think that once again, @mzikherman's ever-relevant GIF has me convinced.

@dblock
Copy link
Contributor

dblock commented Jun 14, 2013

The big question is whether to spell SRT or Srt.

@fancyremarker
Copy link
Contributor Author

@joeyAghion: Actually, I did define that method, incredibly. I'll send a quick pull request to revert that.

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.

None yet

4 participants