django-cache-machine / django-cache-machine

Automatic caching and invalidation for Django models through the ORM.
https://cache-machine.readthedocs.org/en/latest/
BSD 3-Clause "New" or "Revised" License
875 stars 160 forks source link

fix failed invalidation across databases when using replication #105

Closed tobiasmcnulty closed 8 years ago

tobiasmcnulty commented 8 years ago

In 0.8, a regression was introduced (I believe by the changes in PR #29 or PR #36) that broke invalidation when an object previously cached from a slave database was later modified via the master database.

This test and change fixes that by removing the database from the flush key, thereby creating flush lists that are shared across databases. Objects are still cached per DB (so obj._state.db will be correct), but all cached objects/queries across DBs should be invalidated when one changes.

Getting this test working required a few changes to the way multi-DB tests are run, namely:

tobiasmcnulty commented 8 years ago

@Bpless @tgross @jbalogh I believe the Multi DB changes in PR #29 / PR #36 caused a regression described by the test in this PR (https://github.com/django-cache-machine/django-cache-machine/pull/105/files#diff-7053fe2d125e8267cc59b02ee849855bR569).

I can think of a few ways to fix this but am not sure what would be best:

  1. Include the DB key in the cache key, but not the flush key (not a small change, since pretty much everything that generates a flush key does so based on a cache key). This would still allow objects from different DBs to be cached separately, but would (I think) make sure they're all invalidated when a related object or query changes.
  2. Revert the original change entirely, and advise fixing it on the app side by changing allow_relation in the appropriate DB router to allow relations between objects in the master & slave DBs (if the use case here is really a master/slave setup, it shouldn't matter what DB the object(s) came from): https://docs.djangoproject.com/en/1.8/topics/db/multi-db/#allow_relation
  3. Number 1 above, plus add a new setting that indicates whether the DB should be included in the cache/flush keys or not (not a big fan of this option).
  4. Revert the original change, and forcibly overwrite _state.db with a DB key to match the one of the original request, using a method similar to this: https://gist.github.com/tobiasmcnulty/2906c06b7498b3978867 (not sure this would work either)

Would appreciate any feedback!

Thanks.

tobiasmcnulty commented 8 years ago

@Bpless @tgross @jbalogh I took a stab at option 1; would appreciate any feedback.

tobiasmcnulty commented 8 years ago

Also @kmtracey ^^

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.4%) to 93.629% when pulling d4113683a6febefacaa92c113f444b9e4e3118b2 on master-slave-invalidation into a99bfcdacc2bd4e7380324614da506cd686702c1 on master.