Uh oh!
There was an error while loading. Please reload this page.
Cache channel metadata - #2333
Conversation
Codecov Report
@@ Coverage Diff @@## develop #2333 +/- ##
===========================================
- Coverage 81.46% 79.72% -1.75%
===========================================
Files 293 295 +2 Lines 14106 14587 +481 ===========================================
+ Hits 11491 11629 +138 - Misses 2615 2958 +343
Continue to review full report at Codecov.
|
rtibbles
left a comment
There was a problem hiding this comment.
Some questions, some ignorable suggestions, and one suggested extension.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| ids = request.GET.get("id__in") | ||
| if not ids: | ||
| raise ValidationError("id__in GET parameter is required") | ||
| ids = ids.split(",") |
There was a problem hiding this comment.
Not necessary for the admin viewset, but if we generalize this we should probably do a quick filter here by the channels the user has view permissions for, so something like:
ids = self.get_queryset().filter(id__in=ids).values_list("id", flat=True)
Will then filter down the id list to the ones that the user has permissions for (in the admin use case, this is less important though).
rtibbles
commented
Sep 28, 2020
Just need to do a manual test, but this looks good to merge and build on! |
| def cache_stampede(expire, beta=1): | ||
| """Cache decorator with cache stampede protection. |
There was a problem hiding this comment.
Can we add a comment with a link to where we vendored it from?
There was a problem hiding this comment.
I added a link to the pdf of the paper and its explanation. I'll add it to the diskcache code too, I stole some ideas from there (mainly the decorator usage) but for the main logic I used the paper. This implementation is simpler than diskcache's.
There was a problem hiding this comment.
OK, if you didn't lean on diskcache's code that much, this is fine then!
| return result, delta | ||
| @functools.wraps(func) | ||
| def wrapper(*args, **kwargs): |
There was a problem hiding this comment.
If we want to reuse this decorator, guessing we want to be able to parameterize the cache key template - but I think everything else could stay the same (except channel_id would be a more generic id variable name). Thoughts?
There was a problem hiding this comment.
yes, because the key is fetched from the decorated function, the change should be calling it with the final key, applying the parametrization before, it should be an easy change. Do you want me to do it now, so it's prepare for it?
Description
Removes AdminChannelViewSet annotations and defers them to be executed in an async task that will cache the results.
As operations are executed asynchronously, the values are not immediately available if they have not been previously cached. In this case the values are marked with a DEFERRED_FLAG so the frontend knows that it can request them later.
The structure of the created object can be seen in the contentcuration/contentcuration/utils/channel.py and it's done according to the https://www.notion.so/learningequality/2020-12-24-Team-Sonic-counting-descendants-meeting-7ea0f46d4a6648109a22eefbba124a92 decissions
NOTE: some small optimization have also been done, mainly removing unneded order_by requirements in some of the queries, and the use of CTE to avoid scanning the whole File table after paginating the results.
Issue Addressed (if applicable)
Current annotations in AdminChannelViewSet returns queries needing days to be executed (File table has 76 millions of rows and ContentNode has 10 millions and the queries where scanning and joining both tables)
Implementation Notes (optional)
Example of use: http://localhost:8080/api/admin-channels/deferred_data?id__in=0598c68f00fc562486fc6616b63f267f,c1f2b7e6ac9f56a2bb44fa7a48b66dce
Two different tasks are implemented:
cache_channel_metadata_task(key, channel, tree_id)to cache one single channelcache_multiple_channels_metadata_task(channels)to cache multiple channelsCache invalidation
Cache keys will be stored forever
The timestamp stored in the key will be used to retrieve data, but will be marked as stale and recalculated after one hour (following a probabilistic cache invalidation algorithm to avoid a recalculation stampede)