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

fix failed invalidation across databases when using replication #105

Merged
merged 7 commits into from
Oct 22, 2015

Conversation

tobiasmcnulty
Copy link
Member

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:

  • use Django's TEST_MIRROR setting rather than simply duplicating the test data across DBs (without this, there's no way to remove an object from one DB without invalidating it on that DB)
  • use TransactionTestCase (undocumented, but required by TEST_MIRROR setting)
  • use Postgres (also undocumented, but TEST_MIRROR also does not appear to work with sqlite)

@tobiasmcnulty
Copy link
Member Author

@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.

…B while allowing invalidation of both master & slave cached objects when only one is modified or deleted
@tobiasmcnulty
Copy link
Member Author

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

@tobiasmcnulty
Copy link
Member Author

Also @kmtracey ^^

@tobiasmcnulty tobiasmcnulty changed the title test showing failed invalidation when using database replication fix failed invalidation across databases when using replication Oct 17, 2015
…ave flushed the whole cache, or there was some odd connection between completely unrelated objects that shared no attributes)
tobiasmcnulty added a commit that referenced this pull request Oct 22, 2015
…dation

fix failed invalidation across databases when using replication
@tobiasmcnulty tobiasmcnulty merged commit f43be8b into master Oct 22, 2015
@tobiasmcnulty tobiasmcnulty deleted the master-slave-invalidation branch October 22, 2015 21:25
@coveralls
Copy link

Coverage Status

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

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

3 participants