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

INTPYTHON-451 Add support for database caching #253

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented Feb 7, 2025

The MongoDB cache uses two different indexes:
Unique for the key and TTL index for the expiration date.

The Mongo's demon that deletes data from the collection is called every 60 seconds (if I am not mistaken) we have to deal with virtually deleted records (just filter if the record is expired)
So an extra exclude or include expired query are necessary.
Some scenarios to take in account:

  1. counting avoiding expired thing
  2. Do a set over an expired thing (must work as if the item isn't in the DB)
  3. When Cull (capping routine is called) remove by expiration date.

fixes #229

@WaVEV WaVEV requested review from timgraham and Jibola February 7, 2025 06:16
@WaVEV WaVEV force-pushed the add-support-for-database-caching branch from c7cbf04 to 1ee2a04 Compare February 7, 2025 16:07
@WaVEV WaVEV force-pushed the add-support-for-database-caching branch from 5f356db to 5be1867 Compare February 14, 2025 02:38
@WaVEV WaVEV marked this pull request as ready for review February 14, 2025 02:38

class MongoDBCache(BaseDatabaseCache):
def create_indexes(self):
self.collection.create_index("expires_at", expireAfterSeconds=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the expiry set to 0 here? I know we talked about doing the filtering afterwards, but this is basically saying "expire immediately" is it not?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's getting deleted almost instantly, but we also want to manually do the culling, wouldn't this just be better as a regular sorted index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it expires when the time touch the expires_at value, I got the idea from here:
https://www.mongodb.com/docs/manual/tutorial/expire-data/#expire-documents-at-a-specific-clock-time

We also do culling. look at the method _cull

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea is to handle different expiration times, so the set the expiration after 0 so we can chose whether we expires data

@WaVEV WaVEV force-pushed the add-support-for-database-caching branch from 2fb2337 to b2c664b Compare February 25, 2025 11:40
@timgraham
Copy link
Collaborator

timgraham commented Feb 27, 2025 via email

@WaVEV
Copy link
Collaborator Author

WaVEV commented Feb 27, 2025

It's not just the cache tests in Django's test suite that may need the table. If a Django project is using the Django test runner, what creates the table?

Message ID: </pull/253/review/2646403770@ github.com>

Ah ok, understood. I thought it was only used by unit test. Given that, yes, I should add it after the super

Copy link
Collaborator

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

You might compare the implementation with https://github.com/django-nonrel/mongodb-cache/blob/develop/django_mongodb_cache/backend.py. One thing I noticed is that your code doesn't consult router.db_for_write().

@WaVEV
Copy link
Collaborator Author

WaVEV commented Feb 27, 2025

You might compare the implementation with https://github.com/django-nonrel/mongodb-cache/blob/develop/django_mongodb_cache/backend.py. One thing I noticed is that your code doesn't consult router.db_for_write().

Yeah, my bad. I created the method get_collection and forgot to handle the collection to write. I think I have to add the route test to the unit test.

@WaVEV WaVEV force-pushed the add-support-for-database-caching branch from 91d0e58 to eae823d Compare February 28, 2025 03:49
@timgraham timgraham force-pushed the add-support-for-database-caching branch from 210c542 to 361df71 Compare March 4, 2025 02:42
@WaVEV WaVEV force-pushed the add-support-for-database-caching branch from 361df71 to 39a468e Compare March 4, 2025 03:50
@WaVEV WaVEV force-pushed the add-support-for-database-caching branch from f859a29 to 9ea8dc6 Compare March 13, 2025 22:36
@timgraham timgraham changed the title Add support for database caching INTPYTHON-451 Add support for database caching Mar 15, 2025
@timgraham timgraham force-pushed the add-support-for-database-caching branch from 9ea8dc6 to 7453c14 Compare March 15, 2025 01:53
@timgraham timgraham force-pushed the add-support-for-database-caching branch from 7453c14 to 64b1c10 Compare March 15, 2025 02:50
Comment on lines +28 to +33
Unlike Django's built-in database cache backend, this backend supports
automatic culling of expired entries at the database level.

In addition, the cache is culled based on ``CULL_FREQUENCY`` when ``add()``
or ``set()`` is called, if ``MAX_ENTRIES`` is exceeded. See
:ref:`django:cache_arguments` for an explanation of these two options.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two paragraphs replaced this one which was copied from the Django docs:

Unlike other cache backends, the database cache does not support automatic
culling of expired entries at the database level. Instead, expired cache
entries are culled each time add(), set(), or touch() is called.

@timgraham timgraham merged commit 64b1c10 into mongodb:main Mar 18, 2025
9 of 10 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.

add support for database caching
4 participants