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 Cache Misses None Values #20

Merged
merged 10 commits into from
May 1, 2024

Conversation

anthonyp97
Copy link

@anthonyp97 anthonyp97 commented Apr 30, 2024

Overview

  • We currently have a bug in our caching repo where if the value we pull from our cache is None, we will always attempt to re evaluate the underlying function instead of just simply returning the None value in our cache.
  • This is impacting certain tasks such as our category period fund flows, which makes intensive queries to our DB and was missing our cache for ~1300 funds.
  • Fix this by separating out the case where we set a value to be None in the cache vs the django cache actually returning None due to an expired cache key or nonexistent cache key
  • See relevant django cache docs here: https://docs.djangoproject.com/en/4.2/topics/cache/#django.core.cache.cache.get

How to test

  • I tested this locally by copying over this new decorators.py file to a docker container via the path /usr/local/lib/python3.8/dist-packages/cache_helper/decorators.py and then going into a django shell in that container and confirming this works as intended.
  • Added tests for static, class, and instance cached method decorators to test None return. Tests will fail if you don't include changes made in decorators.py here.

python-version: [ "3.7", "3.8", "3.9", "3.10" ]
django-version: [ "3.2" ]
python-version: [ "3.8", "3.9", "3.10" ]
django-version: [ "4.2" ]
Copy link
Author

@anthonyp97 anthonyp97 Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating these versions to align with what we use in main repo while I'm here. Python 3.7 no longer supported by Django 4.2+ so removed that from matrix.

@anthonyp97
Copy link
Author

@Jakeway going to hold off until after code freeze to merge this but once this is merged I'm going to follow these instructions to build a new release in pypi: https://github.com/ycharts/django_cache_helper/blob/master/DEVELOPMENT.md

Though I probably don't have the permission to do the twine upload so might need to sync with @kdhamoycharts on doing that.

@anthonyp97
Copy link
Author

Also once this is merged and we create version 1.0.5, will need to create a PR that updates our requirements file in ycharts repo to reflect the new version and then we should be good.

For some reason can't add any reviewers or assigners to the PR I'm guessing need to update some setting for this repo to enable that for me.

@anthonyp97
Copy link
Author

@kdhamoycharts Looks like I also need an approval from an admin to merge this in and then we'll need to do these steps: https://github.com/ycharts/django_cache_helper/blob/master/DEVELOPMENT.md. Not urgent can do this tomorrow / later in week, but once we do that I can open a PR in ycharts to point to version 1.0.5 so these changes will take effect in our pipeline.

@kdhamoycharts kdhamoycharts merged commit d4d1b32 into ycharts:master May 1, 2024
4 checks passed
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.

3 participants