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

Keyring in database #66

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

Conversation

aweakley
Copy link
Contributor

This moves the keyring into the database to avoid duplication and also keep it up to date.
It would replace #63

def active(self):
return self.filter(expiry__gt=now())

def active_for_url_regexes(self, urls=None):
Copy link
Contributor

@vsalvino vsalvino Jul 16, 2024

Choose a reason for hiding this comment

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

I would prefer to enforce urls: List[str] in the signature, and make it required, rather than the fallback and casting logic below.

# Save the keyring.
_wagcache.set("keyring", keyring)
if urls:
active_keys = KeyringItem.objects.active_for_url_regexes(
Copy link
Contributor

Choose a reason for hiding this comment

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

If this returns a querystring... could you not instead call KeyringItem.objects.active_for_url_regexes(urls).delete() ?

@vsalvino
Copy link
Contributor

Very nice implementation. Thank you! I left a few comments. Also, another bug fix has been merged in recently (adds a try/catch in UpdateCacheMiddleware so you may need to rebase to resolve those conflicts).

self.clear_expired()
return item

def bulk_delete_cache_keys(self, keys: List[str]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer if keys is a QuerySet. Because during the delete, we are normally querying keys to delete, flattening them as values list, then passing it in here and re-querying. Double hits to the database. Would prefer to only make one database hit.

@aweakley aweakley marked this pull request as draft August 22, 2024 08:19
@aweakley
Copy link
Contributor Author

aweakley commented Aug 22, 2024

I've just reverted this to draft because we're finding that calling clear_expired() during set() is not thread safe.

@vsalvino
Copy link
Contributor

Interesting - are you using ASGI by chance? Or is this caused by concurrent calls to clear_expired()?

@aweakley
Copy link
Contributor Author

aweakley commented Oct 23, 2024

It's not ASGI, but there are lots of servers and workers. We were getting many concurrent requests creating calls to set and they were each creating a clear_expired query on the database. Because that was backing up the pages were not being put into the cache quickly, so that led to even more requests creating calls to both things.
We were finding a lot of pages were expiring at the same time, I guess on the anniversary of when the cache was last cleared. So I added a way to add some random variation to the timeout which smoothed things out a lot but it didn't fix it.
In the end the fix turned out to be to run ANALYZE; on the database and everything became fast again (for some reason auto vacuum didn't catch it) - so a completely separate issue than all the things I'd thought of up to then.
So now with the database back to normal I think clear_expired on set would probably be fine, but I've still kept the option to have it run separately.

I'll tidy it up and re-submit the PR.

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