Merge pull request #1758 from MITx/fix/cdodge/use-request-scroped-cache-for-metadata

Fix/cdodge/use request scoped cache for metadata inheritance

cms/envs/common.py

@@ -113,6 +113,7 @@ TEMPLATE_LOADERS = (
 
 MIDDLEWARE_CLASSES = (
     'contentserver.middleware.StaticContentServer',
+    'request_cache.middleware.RequestCache',
     'django.middleware.cache.UpdateCacheMiddleware',
     'django.middleware.common.CommonMiddleware',
     'django.contrib.sessions.middleware.SessionMiddleware',

cms/one_time_startup.py

 from dogapi import dog_http_api, dog_stats_api
 from django.conf import settings
 from xmodule.modulestore.django import modulestore
+from request_cache.middleware import RequestCache
 
 from django.core.cache import get_cache, InvalidCacheBackendError
 
 cache = get_cache('mongo_metadata_inheritance')
 for store_name in settings.MODULESTORE:
     store = modulestore(store_name)
-    store.metadata_inheritance_cache = cache
+    store.metadata_inheritance_cache_subsystem = cache
+    store.request_cache = RequestCache.get_request_cache()
 
 if hasattr(settings, 'DATADOG_API'):
     dog_http_api.api_key = settings.DATADOG_API

common/djangoapps/request_cache/middleware.py

+import threading
+
+_request_cache_threadlocal = threading.local()
+_request_cache_threadlocal.data = {}
+
+class RequestCache(object):
+    @classmethod
+    def get_request_cache(cls):
+        return _request_cache_threadlocal
+            
+    def clear_request_cache(self):
+        _request_cache_threadlocal.data = {}
+
+    def process_request(self, request):
+        self.clear_request_cache()
+        return None
+
+    def process_response(self, request, response):
+        self.clear_request_cache()
+        return response
\ No newline at end of file

common/lib/xmodule/xmodule/modulestore/mongo.py

@@ -109,7 +109,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     references to metadata_inheritance_tree
     """
     def __init__(self, modulestore, module_data, default_class, resources_fs,
-                 error_tracker, render_template, metadata_cache=None):
+                 error_tracker, render_template, cached_metadata=None):
         """
         modulestore: the module store that can be used to retrieve additional modules
 
@@ -134,7 +134,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         # cdodge: other Systems have a course_id attribute defined. To keep things consistent, let's
         # define an attribute here as well, even though it's None
         self.course_id = None
-        self.metadata_cache = metadata_cache
+        self.cached_metadata = cached_metadata
+
 
     def load_item(self, location):
         """
@@ -170,8 +171,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
 
                 model_data = DbModel(kvs, class_, None, MongoUsage(self.course_id, location))
                 module = class_(self, location, model_data)
-                if self.metadata_cache is not None:
-                    metadata_to_inherit = self.metadata_cache.get(metadata_cache_key(location), {}).get('parent_metadata', {}).get(location.url(), {})
+                if self.cached_metadata is not None:
+                    metadata_to_inherit = self.cached_metadata.get(location.url(), {})
                     inherit_metadata(module, metadata_to_inherit)
                 return module
             except:
@@ -223,7 +224,8 @@ class MongoModuleStore(ModuleStoreBase):
     def __init__(self, host, db, collection, fs_root, render_template,
                  port=27017, default_class=None,
                  error_tracker=null_error_tracker,
-                 user=None, password=None, **kwargs):
+                 user=None, password=None, request_cache=None, 
+                 metadata_inheritance_cache_subsystem=None, **kwargs):
 
         ModuleStoreBase.__init__(self)
 
@@ -254,8 +256,10 @@ class MongoModuleStore(ModuleStoreBase):
         self.error_tracker = error_tracker
         self.render_template = render_template
         self.ignore_write_events_on_courses = []
+        self.request_cache = request_cache
+        self.metadata_inheritance_cache_subsystem = metadata_inheritance_cache_subsystem
 
-    def get_metadata_inheritance_tree(self, location):
+    def compute_metadata_inheritance_tree(self, location):
         '''
         TODO (cdodge) This method can be deleted when the 'split module store' work has been completed
         '''
@@ -323,32 +327,45 @@ class MongoModuleStore(ModuleStoreBase):
         if root is not None:
             _compute_inherited_metadata(root)
 
-        return {'parent_metadata': metadata_to_inherit,
-            'timestamp': datetime.now()}
+        return metadata_to_inherit
 
-    def get_cached_metadata_inheritance_trees(self, locations, force_refresh=False):
+    def get_cached_metadata_inheritance_tree(self, location, force_refresh=False):
         '''
         TODO (cdodge) This method can be deleted when the 'split module store' work has been completed
         '''
-
-        trees = {}
-        if locations and self.metadata_inheritance_cache is not None and not force_refresh:
-            trees = self.metadata_inheritance_cache.get_many(list(set([metadata_cache_key(loc) for loc in locations])))
-        else:
-            # This is to help guard against an accident prod runtime without a cache
-            logging.warning('Running MongoModuleStore without metadata_inheritance_cache. '
-                            'This should not happen in production!')
-
-        to_cache = {}
-        for loc in locations:
-            cache_key = metadata_cache_key(loc)
-            if cache_key not in trees:
-                to_cache[cache_key] = trees[cache_key] = self.get_metadata_inheritance_tree(loc)
-
-        if to_cache and self.metadata_inheritance_cache is not None:
-            self.metadata_inheritance_cache.set_many(to_cache)
-
-        return trees
+        key = metadata_cache_key(location)
+        tree = {}
+        
+        if not force_refresh:
+            # see if we are first in the request cache (if present)
+            if self.request_cache is not None and key in self.request_cache.data.get('metadata_inheritance', {}):
+                return self.request_cache.data['metadata_inheritance'][key]
+
+            # then look in any caching subsystem (e.g. memcached)
+            if self.metadata_inheritance_cache_subsystem is not None:
+                tree = self.metadata_inheritance_cache_subsystem.get(key, {})
+            else:
+                logging.warning('Running MongoModuleStore without a metadata_inheritance_cache_subsystem. This is OK in localdev and testing environment. Not OK in production.')
+
+        if not tree:
+            # if not in subsystem, or we are on force refresh, then we have to compute
+            tree = self.compute_metadata_inheritance_tree(location)
+            
+            # now write out computed tree to caching subsystem (e.g. memcached), if available
+            if self.metadata_inheritance_cache_subsystem is not None:
+                self.metadata_inheritance_cache_subsystem.set(key, tree)
+
+        # now populate a request_cache, if available. NOTE, we are outside of the
+        # scope of the above if: statement so that after a memcache hit, it'll get
+        # put into the request_cache
+        if self.request_cache is not None:
+            # we can't assume the 'metadatat_inheritance' part of the request cache dict has been
+            # defined
+            if 'metadata_inheritance' not in self.request_cache.data:
+                self.request_cache.data['metadata_inheritance'] = {}
+            self.request_cache.data['metadata_inheritance'][key] = tree
+
+        return tree
 
     def refresh_cached_metadata_inheritance_tree(self, location):
         """
@@ -357,15 +374,7 @@ class MongoModuleStore(ModuleStoreBase):
         """
         pseudo_course_id = '/'.join([location.org, location.course])
         if pseudo_course_id not in self.ignore_write_events_on_courses:
-            self.get_cached_metadata_inheritance_trees([location], force_refresh=True)
-
-    def clear_cached_metadata_inheritance_tree(self, location):
-        """
-        Delete the cached metadata inheritance tree for the org/course combination
-        for location
-        """
-        if self.metadata_inheritance_cache is not None:
-            self.metadata_inheritance_cache.delete(metadata_cache_key(location))
+            self.get_cached_metadata_inheritance_tree(location, force_refresh=True)
 
     def _clean_item_data(self, item):
         """
@@ -411,18 +420,7 @@ class MongoModuleStore(ModuleStoreBase):
 
         return data
 
-    def _cache_metadata_inheritance(self, items, depth, force_refresh=False):
-        """
-        Retrieves all course metadata inheritance trees needed to load items
-        """
-
-        locations = [
-            Location(item['location']) for item in items
-            if not (item['location']['category'] == 'course' and depth == 0)
-        ]
-        return self.get_cached_metadata_inheritance_trees(locations, force_refresh=force_refresh)
-
-    def _load_item(self, item, data_cache, metadata_cache):
+    def _load_item(self, item, data_cache, apply_cached_metadata=True):
         """
         Load an XModuleDescriptor from item, using the children stored in data_cache
         """
@@ -434,6 +432,10 @@ class MongoModuleStore(ModuleStoreBase):
 
         resource_fs = OSFS(root)
 
+        cached_metadata = {}
+        if apply_cached_metadata:
+            cached_metadata = self.get_cached_metadata_inheritance_tree(Location(item['location']))
+
         # TODO (cdodge): When the 'split module store' work has been completed, we should remove
         # the 'metadata_inheritance_tree' parameter
         system = CachingDescriptorSystem(
@@ -443,7 +445,7 @@ class MongoModuleStore(ModuleStoreBase):
             resource_fs,
             self.error_tracker,
             self.render_template,
-            metadata_cache,
+            cached_metadata,
         )
         return system.load_item(item['location'])
 
@@ -453,11 +455,11 @@ class MongoModuleStore(ModuleStoreBase):
         to specified depth
         """
         data_cache = self._cache_children(items, depth)
-        inheritance_cache = self._cache_metadata_inheritance(items, depth)
 
         # if we are loading a course object, if we're not prefetching children (depth != 0) then don't
-        # bother with the metadata inheritence
-        return [self._load_item(item, data_cache, inheritance_cache) for item in items]
+        # bother with the metadata inheritance
+        return [self._load_item(item, data_cache,
+            apply_cached_metadata=(item['location']['category']!='course' or depth !=0)) for item in items]
 
     def get_courses(self):
         '''

common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py

@@ -103,58 +103,3 @@ class TestMongoModuleStore(object):
     def test_path_to_location(self):
         '''Make sure that path_to_location works'''
         check_path_to_location(self.store)
-
-    def test_metadata_inheritance_query_count(self):
-        '''
-        When retrieving items from mongo, we should only query the cache a number of times
-        equal to the number of courses being retrieved from.
-
-        We should also not query
-        '''
-        self.store.metadata_inheritance_cache = Mock()
-        get_many = self.store.metadata_inheritance_cache.get_many
-        set_many = self.store.metadata_inheritance_cache.set_many
-        get_many.return_value = {('edX', 'toy'): {}}
-
-        self.store.get_item(Location("i4x://edX/toy/course/2012_Fall"), depth=0)
-        assert_false(get_many.called)
-        assert_false(set_many.called)
-        get_many.reset_mock()
-
-        self.store.get_item(Location("i4x://edX/toy/course/2012_Fall"), depth=3)
-        get_many.assert_called_with([('edX', 'toy')])
-        assert_equals(0, set_many.call_count)
-        get_many.reset_mock()
-
-        self.store.get_items(Location('i4x', 'edX', None, 'course', None), depth=0)
-        assert_false(get_many.called)
-        assert_false(set_many.called)
-        get_many.reset_mock()
-
-        self.store.get_items(Location('i4x', 'edX', None, 'course', None), depth=3)
-        assert_equals(1, get_many.call_count)
-        assert_equals([('edX', 'simple'), ('edX', 'toy')], sorted(get_many.call_args[0][0]))
-        assert_equals(1, set_many.call_count)
-        assert_equals([('edX', 'simple')], sorted(set_many.call_args[0][0].keys()))
-        get_many.reset_mock()
-
-        self.store.get_items(Location('i4x', 'edX', None, None, None), depth=0)
-        assert_equals(1, get_many.call_count)
-        assert_equals([('edX', 'simple'), ('edX', 'toy')], sorted(get_many.call_args[0][0]))
-        assert_equals(1, set_many.call_count)
-        assert_equals([('edX', 'simple')], sorted(set_many.call_args[0][0].keys()))
-        get_many.reset_mock()
-
-    def test_metadata_inheritance_query_count_forced_refresh(self):
-        self.store.metadata_inheritance_cache = Mock()
-        get_many = self.store.metadata_inheritance_cache.get_many
-        set_many = self.store.metadata_inheritance_cache.set_many
-        get_many.return_value = {('edX', 'toy'): {}}
-
-        self.store.get_cached_metadata_inheritance_trees(
-            [Location("i4x://edX/toy/course/2012_Fall"), Location("i4x://edX/simple/course/2012_Fall")],
-            True
-        )
-        assert_false(get_many.called)
-        assert_equals(1, set_many.call_count)
-        assert_equals([('edX', 'simple'), ('edX', 'toy')], sorted(set_many.call_args[0][0].keys()))

lms/envs/common.py

@@ -364,6 +364,7 @@ TEMPLATE_LOADERS = (
 
 MIDDLEWARE_CLASSES = (
     'contentserver.middleware.StaticContentServer',
+    'request_cache.middleware.RequestCache',
     'django_comment_client.middleware.AjaxExceptionMiddleware',
     'django.middleware.common.CommonMiddleware',
     'django.contrib.sessions.middleware.SessionMiddleware',

lms/one_time_startup.py

@@ -2,13 +2,15 @@ import logging
 from dogapi import dog_http_api, dog_stats_api
 from django.conf import settings
 from xmodule.modulestore.django import modulestore
+from request_cache.middleware import RequestCache
 
 from django.core.cache import get_cache, InvalidCacheBackendError
 
 cache = get_cache('mongo_metadata_inheritance')
 for store_name in settings.MODULESTORE:
     store = modulestore(store_name)
-    store.metadata_inheritance_cache = cache
+    store.metadata_inheritance_cache_subsystem = cache
+    store.request_cache = RequestCache.get_request_cache()
 
 if hasattr(settings, 'DATADOG_API'):
     dog_http_api.api_key = settings.DATADOG_API