Updated caching strategy #576
Conversation
|
|
||
| def test_one_course(self): | ||
| self.set_runtime_option() | ||
| self.precompute_reviews() |
There was a problem hiding this comment.
Doesn't matter for functionality, but you can also assert error/success responses with the self.out defined
backend/tests/review/test_api.py
Outdated
| "course-reviews", | ||
| "CIS-1200", | ||
| {"hello": "world"}, # No cache miss | ||
| query_params={"semester": OLD_SEMESTER}, |
There was a problem hiding this comment.
@shiva-menta we should make sure this is intended behavior
| @@ -277,6 +278,7 @@ def handle(self, *args, **kwargs): | |||
|
|
|||
| print("Recomputing Section.has_reviews...") | |||
| recompute_has_reviews() | |||
| precompute_pcr_views(True, True) | |||
There was a problem hiding this comment.
nit: maybe make these keyword args
| if verbose: | ||
| print("Now precomputing PCR reviews.") | ||
|
|
||
| CachedReviewResponse.objects.all().update(expired=True) |
There was a problem hiding this comment.
This is not in an atomic block, so what's the potential time in which the cached responses are down totally. Is that acceptable?
There was a problem hiding this comment.
NVM i see what you're doing
There was a problem hiding this comment.
expired CachedReviewResponses can still be returned from the view
There was a problem hiding this comment.
Yeah, but I think there's no reason to not have this in an atomic block either – will change.
| objs_to_update.append(response_obj) | ||
| except Http404: | ||
| logging.info( | ||
| f"Topic returned 404 (topic_id {topic_id}, " |
There was a problem hiding this comment.
nit: is a 404 an expected behavior? if so, logging.info makes sense; if not logging.error might be better so it looks correct on our sentry
There was a problem hiding this comment.
Iirc there are some topics that have a course with no reviews and that leads to a 404 on our current implementation (can't remember if this is the exact error, but it's something similar). So yeah would say it can be expected behavior on certain courses.
|
|
||
| if topic_id in topic_set: | ||
| try: | ||
| has_count += 1 |
There was a problem hiding this comment.
nit: usually try to minimize the stuff inside the try block to what is absolutely going to throw an error (for debug/code readability reasons)
| print("Now precomputing PCR reviews.") | ||
|
|
||
| CachedReviewResponse.objects.all().update(expired=True) | ||
| responses = CachedReviewResponse.objects.all() |
There was a problem hiding this comment.
nit: I got a little confused bc this is called responses. maybe cached_responses is a better name?
|
|
||
| CachedReviewResponse.objects.all().update(expired=True) | ||
| responses = CachedReviewResponse.objects.all() | ||
| topic_set = {response.topic_id: response for response in responses} |
There was a problem hiding this comment.
nit: since this is a dict, and also is a mapping only for cached responses (not for all topics) it might be good to rename it
| to a JSON object storing summarized course review data (all the data frontend uses to display | ||
| reviews). | ||
| """ | ||
|
|
There was a problem hiding this comment.
I think topic_id should be unique=True here (theoretically our loading script should enforce this, but I think its good to specify here too?)
| topic = recent_course.topic | ||
| course_id_list = list(topic.courses.values_list("id")) | ||
| topic_id = ".".join([str(id[0]) for id in sorted(course_id_list)]) | ||
| cache.set(course_code, topic_id, MONTH_IN_SECONDS) |
There was a problem hiding this comment.
why do we set the cache manually?
There was a problem hiding this comment.
instead of using the decorator
There was a problem hiding this comment.
also we shpuld make sure this cache is cleared every time we reimport and/or that the TTL is short enough.
There was a problem hiding this comment.
Cache is set manually because we want some control over setting the course to topic mappings and the topic to response mappings. Using the decorator, since the routes are based on course, would only allow for course to response mapping, which can be a bit space inefficient.
There was a problem hiding this comment.
Yup the relevant cache entries are cleared in the precompute_pcr_reviews script.
No description provided.