Skip to content

Conversation

@shanbady
Copy link
Contributor

@shanbady shanbady commented Sep 12, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5450

Description (What does it do?)

This PR adds caching to the remaining server side views. We cache for all users on views that don't have any user specific info using "cache_page_for_all_users" decorator otherwise we use "cache_page_for_anonymous_users" to only cache for anonymous users

How can this be tested?

  1. checkout this branch. have data loaded
  2. as you browse around the first page view will cache api calls (most of them for anonymous users only but some for all users)
  3. make note of performance differences between main and the cached version
  4. to clear all the caches we can manually run
from main.utils import clear_search_cache
clear_search_cache()

there are some further instructions (specific to search cache but can now be applied to remaining api views) here

Additional Context

Some key things in this PR for reviewer to double check:

  1. The cache mechanism used for each view is appropriate (to cache for all users vs just anonymous)
  2. Are the resources in the cached view refreshed appropriately (every 24 hours on loading new resources). Are there any views we should remove this caching from or tweak when it is cleared.

The cache is by default now set to the "DummyCache" - we can explicitly enable and disable on a per test basis if the point of the test is to test something with caching enabled as done here

@shanbady shanbady marked this pull request as ready for review September 13, 2024 18:10
@shanbady shanbady added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Sep 13, 2024
@mbertrand mbertrand self-assigned this Sep 13, 2024
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Looks great, just one comment about caching of the featured resources view (check with Ferdi about that one).

Also, the news_events/views/FeedItemViewSet and FeedSourceViewSet could use caching.

.distinct()
)

@method_decorator(
Copy link
Member

Choose a reason for hiding this comment

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

There was some deliberate results randomization added to this featured list view so as not to give any apparent preference to any particular unit. Might be good to double-check with Ferdi to see if it's okay if that randomization is cached so all anonymous users see the same result for 24 hours before it's randomized again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

double checked with ferdi. He said we can leave this as-is and have a separate ticket to handle the randomization on the frontend.

@mbertrand
Copy link
Member

PS and clear the cache after news_events tasks or mgmt command are run

@mbertrand mbertrand added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Sep 16, 2024
@shanbady shanbady requested a review from mbertrand September 17, 2024 14:36
@shanbady shanbady added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Sep 17, 2024
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@mbertrand mbertrand added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Sep 17, 2024
@shanbady shanbady merged commit 26441d2 into main Sep 17, 2024
@shanbady shanbady deleted the shanbady/cache-remaining-views branch September 17, 2024 15:28
@odlbot odlbot mentioned this pull request Sep 18, 2024
13 tasks
This was referenced Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants