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

add CACHES to settings, defaulting to locmem #319

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Conversation

joshuadavidthomas
Copy link
Member

No description provided.

@joshuadavidthomas joshuadavidthomas requested a review from a team as a code owner September 13, 2024 13:54
Copy link

@jefftriplett jefftriplett left a comment

Choose a reason for hiding this comment

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

I'm waffling on what the default should be. I think it's fine/safe to start with this, but if something comes up, we might want to make default to Django's default https://docs.djangoproject.com/en/5.1/topics/cache/#local-memory-caching

Also, we probably need to make sure our test settings have dummycache or keep that in mind in case we see anything weird pop up.

@joshuadavidthomas
Copy link
Member Author

@jefftriplett Re: the dummycache, that's already in place

CACHES = {
"default": {
"BACKEND": "django.core.cache.backends.dummy.DummyCache",
}
}

What about checking for the presence of the CACHE_URL env var, and if it's not present falling back to the in memory?

@joshuadavidthomas
Copy link
Member Author

Ah, you can set the localmem via the cache url https:/epicserve/django-cache-url#supported-caches

locmem (default): 'locmem://[NAME]'

I'm not opposed to that, but like I mentioned in one of our most recent meetings my only worry would be memory usage and how our applications are currently deployed. I've never done any benchmarking around this, so I do not know how much memory the caching would take and whether it would make a measurable difference where we would have to bump the specs up on the app machines. If you have some experience in this area, I'm all ears.

@joshuadavidthomas
Copy link
Member Author

On the other hand, I probably run those machines a little too close to the edge in terms of allocated memory, so maybe they could all stand to be bumped a bit. 🤷

@jefftriplett
Copy link

I'm not opposed to that, but like I mentioned in one of our most recent meetings my only worry would be memory usage and how our applications are currently deployed.

Yes, but this is the default, and we aren't seeing any apps run out of memory. I think the change is fine, but there might be a non-zero impact if we were to set it to one of fly's persistent volumes. (which we aren't using)

@joshuadavidthomas
Copy link
Member Author

Ok, you've convinced me. I'm probably being a bit overly worried about it anyway.

@joshuadavidthomas joshuadavidthomas changed the title add CACHES to settings, defaulting to local file system add CACHES to settings, defaulting to locmem Sep 13, 2024
@joshuadavidthomas joshuadavidthomas merged commit 801860c into main Sep 13, 2024
6 checks passed
@joshuadavidthomas joshuadavidthomas deleted the cache-url branch September 13, 2024 15:12
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.

2 participants