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

Remove queryset caching support #6639

Closed
jeremystretch opened this issue Jun 21, 2021 · 7 comments
Closed

Remove queryset caching support #6639

jeremystretch opened this issue Jun 21, 2021 · 7 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: deprecation Removal of existing functionality or behavior
Milestone

Comments

@jeremystretch
Copy link
Member

jeremystretch commented Jun 21, 2021

Proposed Changes

Remove the support for query caching that was introduced in v2.6 under #2647. This would remove django-cacheops as a dependency and eliminate the REDS['caching'] and CACHE_TIMEOUT configuration parameters.

Justification

Since its implementation, we have been constantly chasing numerous bugs attributable to limitations in the caching engine. I have addressed some of these issues within django-cacheops itself, however we continue to run into problems, particularly where complex workflows are involved.

In my opinion, it does not make sense to continue burning time chasing these bugs to support a feature with relatively little benefit. I would prefer to optimize NetBox itself where necessary to meet performance expectations rather than trying to keep the caching layer intact. I've opened this issue for discussion to see what the community thinks.

@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation type: deprecation Removal of existing functionality or behavior labels Jun 21, 2021
@jhujhiti
Copy link

Are there any quantitative numbers on how much the removal of caching would hurt specific API endpoints, or a way to disable the cache in an existing 2.11 environment to evaluate what the impact of this change would be? As a user, I'm certainly in favor of reducing the complexity of the stack, but my deployment has a number of identical queries run by external monitoring repeatedly and losing the cache there could put a lot more easily-cached load on the database. (Although I do suspect Redis isn't making much difference, I don't have a way to prove it.)

@jeremystretch
Copy link
Member Author

@jhujhiti You can test on your own deployment by toggling the caching feature on and off. Setting CACHE_TIMEOUT = 0 in configuration.py and restarting the NetBox service will disable caching.

@jhujhiti
Copy link

Great! No discernible impact on the 95th or 50th percentile latency on the API endpoints I've been monitoring. Consider this a vote in favor.

@dreng
Copy link
Contributor

dreng commented Jun 24, 2021

Very low impact in our environment. Not noticeable in every day use. I'd say: kick it!

@jeremystretch jeremystretch changed the title Remove caching support in NetBox v3.0 Remove caching support Jun 30, 2021
@maximumG
Copy link
Contributor

maximumG commented Jul 1, 2021

As mentioned in issue #6651. The check_release process will be impacted by cache removal.

However check_release can easily rely on RQ result caching (result_ttl) and RQ registries to achieve the same result as the current implementation.

When browsing the home page it is possible to check in the RQ registries for the latest check_release job and its corresponding result. If the result has expired (> result_ttl time), we can queue a new check_release job.

I think that setting the check_release job with a result_ttl to 86400s is acceptable to check for an update every day.

@candlerb
Copy link
Contributor

candlerb commented Jul 2, 2021

Just to add a +1 here. Databases like postgres have very good internal cache mechanisms, and I never saw any benefit in adding a layer on top.

Caching of HTML rendering might have some justification, but only for pages which are viewed by multiple people and fetched repeatedly - which would probably only be the front page, and even then it varies by the user's permissions.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Jul 7, 2021
@jeremystretch jeremystretch added this to the v3.0 milestone Jul 7, 2021
@jeremystretch jeremystretch changed the title Remove caching support Remove queryset caching support Jul 7, 2021
@jeremystretch
Copy link
Member Author

I should clarify that this is just removing queryset caching. We will still maintain a Redis-based cache for generic caching functions (e.g. checking for the latest NetBox release). We will likely replace django-cacheops with django-redis, which includes support for Redis Sentinel.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: deprecation Removal of existing functionality or behavior
Projects
None yet
Development

No branches or pull requests

5 participants