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

Stop using signals to cache objects #137

Closed
jsocol opened this issue Nov 19, 2014 · 4 comments · Fixed by #211
Closed

Stop using signals to cache objects #137

jsocol opened this issue Nov 19, 2014 · 4 comments · Fixed by #211
Assignees
Milestone

Comments

@jsocol
Copy link
Collaborator

jsocol commented Nov 19, 2014

Using the signal handlers to cache objects and clear the cache is way too complicated and is even worse when you want to cache misses. It's also very hard to reason about and has been a source of some pain in the past.

The strategy I've been using internally lately is much, much simpler:

@classmethod
def get(cls, name):
    key = cls._cache_key(name)
    obj = cache.get(key)
    if obj == EMPTY:  # Some sentinel value
        return None  # or raise cls.DoesNotExist
    if obj:
        return obj
    try:
        obj = cls.objects.get(name=name)
        cache.add(key, obj)
    except cls.DoesNotExist:
        cache.add(key, EMPTY)
        return None  # or raise
    return obj

    def save(self, *a, **kw):
        cache.delete(self._cache_key(self.name))
        # ...

This works great when, like in waffle, most access is by one particular attribute, in this case the names. This doesn't impact the user API at all, only the internals of some methods.

It's a little more complex with the sub-keys for user and group lists on flags, but I think by being clever with the _cache_key and including the modified datetime, we can make that invalidation automatic and easier.

@jsocol
Copy link
Collaborator Author

jsocol commented Jan 11, 2015

I feel bad about this, but here's what I'm about to do.

In Flag.get(), what's actually going to get cached is an augmented copy of the instance .__dict__ with the IDs from the two many-to-many relationships as lists (e.g. _user_ids and _group_ids). Then the restore-from-cache path is f = Flag(); f.__dict__.update(cached_dict), and two new properties (e.g. Flag.user_ids) get added.

Then, for the user/group checks, we only need check that the user ID is in the flag.user_ids list, or that the intersection of the user's group IDs and flag.group_ids is not empty. I'll do something here to make sure that when we check multiple flags, we don't select the user group IDs every time.

The "all" caches will cache lists of these dicts, instead of whatever they have now.

Finally, signal handlers will be set up on post_save, pre_delete, post_delete, and m2m_changed, for the following:

  • post_save: update the instance cache and the all cache
  • pre_delete: update the instance cache with the EMPTY value
  • post_delete: update the all cache
  • m2m_changed: update the instance cache and the all cache

The assumption is that changing flags is very rare compared to checking them, so we'd much rather incur these queries predictably on change, instead of on access.

If any of that sends people screaming for the hills... Or if it seems I've forgotten anything here, let me know.

@jsocol
Copy link
Collaborator Author

jsocol commented Jan 11, 2015

I'll also update the admin to not use the queryset delete—or to at least overwrite all the caches if it does.

@mwarkentin
Copy link
Contributor

The assumption is that changing flags is very rare compared to checking them, so we'd much rather incur these queries predictably on change, instead of on access.

+1

@acangiani
Copy link

Is there any update on this feature? When this feature is complete the 0.10.2 should be released?

@jsocol jsocol modified the milestones: 0.11.x, 0.12 May 28, 2016
jsocol added a commit that referenced this issue May 28, 2016
- Removes all signal handlers, fixes #137.
- Adds Cls.get_all() methods, fixes #142.
- Moves caching globally into Cls.get() and Cls.get_all(), fixes #170,
  fixes #160, fixes #77.
- Adds managers to invalidate ALL_* cache keys on create.
- Adds a new delete admin action to invalidate cache.
- Adds Flag.is_active_for_user, fixes #186.
- Uses delete_many to flush cache, fixes #158.
- Fixes cache invalidation in override_*, fixes #163.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants