From 4e22affb24fd8b424717614c466498211a638388 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 30 Jun 2016 00:15:19 -0400 Subject: [PATCH] [PERF-344] Add versioning of cached course assets to allow graceful cache invalidation When releasing the versioned assets work, we stumbled on a problem with old pickled versions of the StaticContent objects residing in cache, which triggered a bug in the code. Not wanting to blow away all cached items, we ended up having to revert and add in some backwards-compatible helper code to ease the transition. With this, we'll now utilize the version argument that Django's caching interface allows, in conjunction with a constant value that can be modified when breaking changes are being made, to let us gracefully ignore older cached course assets. --- .../contentstore/tests/test_core_caching.py | 5 +- cms/djangoapps/contentstore/views/assets.py | 2 +- .../views/tests/test_transcripts.py | 2 +- common/djangoapps/cache_toolbox/core.py | 27 ---------- common/djangoapps/contentserver/__init__.py | 5 ++ common/djangoapps/contentserver/caching.py | 49 +++++++++++++++++++ common/djangoapps/contentserver/middleware.py | 2 +- .../courseware/tests/test_video_handlers.py | 2 +- 8 files changed, 62 insertions(+), 32 deletions(-) create mode 100644 common/djangoapps/contentserver/caching.py diff --git a/cms/djangoapps/contentstore/tests/test_core_caching.py b/cms/djangoapps/contentstore/tests/test_core_caching.py index 191226dbf7..ae90df002e 100644 --- a/cms/djangoapps/contentstore/tests/test_core_caching.py +++ b/cms/djangoapps/contentstore/tests/test_core_caching.py @@ -1,4 +1,7 @@ -from cache_toolbox.core import get_cached_content, set_cached_content, del_cached_content +""" +Tests core caching facilities. +""" +from contentserver.caching import get_cached_content, set_cached_content, del_cached_content from opaque_keys.edx.locations import Location from django.test import TestCase diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 7bea65feec..8ddc9a32af 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -10,7 +10,7 @@ from django.views.decorators.http import require_http_methods, require_POST from django.conf import settings from edxmako.shortcuts import render_to_response -from cache_toolbox.core import del_cached_content +from contentserver.caching import del_cached_content from contentstore.utils import reverse_course_url from xmodule.contentstore.django import contentstore diff --git a/cms/djangoapps/contentstore/views/tests/test_transcripts.py b/cms/djangoapps/contentstore/views/tests/test_transcripts.py index 863f8f6b23..7bd526721f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_transcripts.py +++ b/cms/djangoapps/contentstore/views/tests/test_transcripts.py @@ -13,7 +13,7 @@ from django.test.utils import override_settings from django.conf import settings from contentstore.tests.utils import CourseTestCase, mock_requests_get -from cache_toolbox.core import del_cached_content +from contentserver.caching import del_cached_content from xmodule.modulestore.django import modulestore from xmodule.contentstore.django import contentstore from xmodule.contentstore.content import StaticContent diff --git a/common/djangoapps/cache_toolbox/core.py b/common/djangoapps/cache_toolbox/core.py index 31c853f7eb..0122220d10 100644 --- a/common/djangoapps/cache_toolbox/core.py +++ b/common/djangoapps/cache_toolbox/core.py @@ -107,30 +107,3 @@ def instance_key(model, instance_or_pk): model._meta.model_name, getattr(instance_or_pk, 'pk', instance_or_pk), ) - - -def set_cached_content(content): - cache.set(unicode(content.location).encode("utf-8"), content) - - -def get_cached_content(location): - return cache.get(unicode(location).encode("utf-8")) - - -def del_cached_content(location): - """ - delete content for the given location, as well as for content with run=None. - it's possible that the content could have been cached without knowing the - course_key - and so without having the run. - """ - def location_str(loc): - return unicode(loc).encode("utf-8") - - locations = [location_str(location)] - try: - locations.append(location_str(location.replace(run=None))) - except InvalidKeyError: - # although deprecated keys allowed run=None, new keys don't if there is no version. - pass - - cache.delete_many(locations) diff --git a/common/djangoapps/contentserver/__init__.py b/common/djangoapps/contentserver/__init__.py index e69de29bb2..a1038dee36 100644 --- a/common/djangoapps/contentserver/__init__.py +++ b/common/djangoapps/contentserver/__init__.py @@ -0,0 +1,5 @@ +""" +Serves course assets to end users. +""" + +CONTENTSERVER_VERSION = 1 diff --git a/common/djangoapps/contentserver/caching.py b/common/djangoapps/contentserver/caching.py new file mode 100644 index 0000000000..7a33032b01 --- /dev/null +++ b/common/djangoapps/contentserver/caching.py @@ -0,0 +1,49 @@ +""" +Helper functions for caching course assets. +""" +from django.core.cache import caches +from django.core.cache.backends.base import InvalidCacheBackendError +from opaque_keys import InvalidKeyError +from . import CONTENTSERVER_VERSION + +# See if there's a "course_assets" cache configured, and if not, fallback to the default cache. +CONTENT_CACHE = caches['default'] +try: + CONTENT_CACHE = caches['course_assets'] +except InvalidCacheBackendError: + pass + + +def set_cached_content(content): + """ + Stores the given piece of content in the cache, using its location as the key. + """ + CONTENT_CACHE.set(unicode(content.location).encode("utf-8"), content, version=CONTENTSERVER_VERSION) + + +def get_cached_content(location): + """ + Retrieves the given piece of content by its location if cached. + """ + return CONTENT_CACHE.get(unicode(location).encode("utf-8"), version=CONTENTSERVER_VERSION) + + +def del_cached_content(location): + """ + Delete content for the given location, as well versions of the content without a run. + + It's possible that the content could have been cached without knowing the course_key, + and so without having the run. + """ + def location_str(loc): + """Force the location to a Unicode string.""" + return unicode(loc).encode("utf-8") + + locations = [location_str(location)] + try: + locations.append(location_str(location.replace(run=None))) + except InvalidKeyError: + # although deprecated keys allowed run=None, new keys don't if there is no version. + pass + + CONTENT_CACHE.delete_many(locations, version=CONTENTSERVER_VERSION) diff --git a/common/djangoapps/contentserver/middleware.py b/common/djangoapps/contentserver/middleware.py index 29eed2e048..28416582a7 100644 --- a/common/djangoapps/contentserver/middleware.py +++ b/common/djangoapps/contentserver/middleware.py @@ -17,7 +17,7 @@ from xmodule.contentstore.content import StaticContent, XASSET_LOCATION_TAG from xmodule.modulestore import InvalidLocationError from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import AssetLocator -from cache_toolbox.core import get_cached_content, set_cached_content +from .caching import get_cached_content, set_cached_content from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.exceptions import NotFoundError diff --git a/lms/djangoapps/courseware/tests/test_video_handlers.py b/lms/djangoapps/courseware/tests/test_video_handlers.py index 72356c1cce..7b0130dc5b 100644 --- a/lms/djangoapps/courseware/tests/test_video_handlers.py +++ b/lms/djangoapps/courseware/tests/test_video_handlers.py @@ -20,7 +20,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.x_module import STUDENT_VIEW from . import BaseTestXmodule from .test_video_xml import SOURCE_XML -from cache_toolbox.core import del_cached_content +from contentserver.caching import del_cached_content from xmodule.exceptions import NotFoundError from xmodule.video_module.transcripts_utils import (