Merge pull request #6779 from edx/cpennington/split-use-request-cache

Cpennington/split use request cache

common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py

@@ -21,3 +21,78 @@ class BlockKey(namedtuple('BlockKey', 'type id')):
 
 
 CourseEnvelope = namedtuple('CourseEnvelope', 'course_key structure')
+
+
+class BlockData(object):
+    """
+    Wrap the block data in an object instead of using a straight Python dictionary.
+    Allows the storing of meta-information about a structure that doesn't persist along with
+    the structure itself.
+    """
+    @contract(block_dict=dict)
+    def __init__(self, block_dict={}):  # pylint: disable=dangerous-default-value
+        # Has the definition been loaded?
+        self.definition_loaded = False
+        self.from_storable(block_dict)
+
+    def to_storable(self):
+        """
+        Serialize to a Mongo-storable format.
+        """
+        return {
+            'fields': self.fields,
+            'block_type': self.block_type,
+            'definition': self.definition,
+            'defaults': self.defaults,
+            'edit_info': self.edit_info
+        }
+
+    @contract(stored=dict)
+    def from_storable(self, stored):
+        """
+        De-serialize from Mongo-storable format to an object.
+        """
+        self.fields = stored.get('fields', {})
+        self.block_type = stored.get('block_type', None)
+        self.definition = stored.get('definition', None)
+        self.defaults = stored.get('defaults', {})
+        self.edit_info = stored.get('edit_info', {})
+
+    def get(self, key, *args, **kwargs):
+        """
+        Dict-like 'get' method. Raises AttributeError if requesting non-existent attribute and no default.
+        """
+        if len(args) > 0:
+            return getattr(self, key, args[0])
+        elif 'default' in kwargs:
+            return getattr(self, key, kwargs['default'])
+        else:
+            return getattr(self, key)
+
+    def __getitem__(self, key):
+        """
+        Dict-like '__getitem__'.
+        """
+        if not hasattr(self, key):
+            raise KeyError
+        else:
+            return getattr(self, key)
+
+    def __setitem__(self, key, value):
+        setattr(self, key, value)
+
+    def __delitem__(self, key):
+        delattr(self, key)
+
+    def __iter__(self):
+        return self.__dict__.iterkeys()
+
+    def setdefault(self, key, default=None):
+        """
+        Dict-like 'setdefault'.
+        """
+        try:
+            return getattr(self, key)
+        except AttributeError:
+            setattr(self, key, default)
+            return default

common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py

@@ -53,7 +53,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         if course_entry.course_key.course:
             root = modulestore.fs_root / course_entry.course_key.org / course_entry.course_key.course / course_entry.course_key.run
         else:
-            root = modulestore.fs_root / course_entry.structure['_id']
+            root = modulestore.fs_root / str(course_entry.structure['_id'])
         root.makedirs_p()  # create directory if it doesn't exist
 
         id_manager = SplitMongoIdManager(self)
@@ -117,10 +117,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         if cached_module:
             return cached_module
 
-        json_data = self.get_module_data(block_key, course_key)
+        block_data = self.get_module_data(block_key, course_key)
 
-        class_ = self.load_block_type(json_data.get('block_type'))
-        block = self.xblock_from_json(class_, course_key, block_key, json_data, course_entry_override, **kwargs)
+        class_ = self.load_block_type(block_data.get('block_type'))
+        block = self.xblock_from_json(class_, course_key, block_key, block_data, course_entry_override, **kwargs)
         self.modulestore.cache_block(course_key, version_guid, block_key, block)
         return block
 
@@ -154,26 +154,27 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
     # pointing to the same structure, the access is likely to be chunky enough that the last known container
     # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id.
     @contract(block_key="BlockKey | None")
-    def xblock_from_json(
-        self, class_, course_key, block_key, json_data, course_entry_override=None, **kwargs
-    ):
+    def xblock_from_json(self, class_, course_key, block_key, block_data, course_entry_override=None, **kwargs):
+        """
+        Load and return block info.
+        """
         if course_entry_override is None:
             course_entry_override = self.course_entry
         else:
             # most recent retrieval is most likely the right one for next caller (see comment above fn)
             self.course_entry = CourseEnvelope(course_entry_override.course_key, self.course_entry.structure)
 
-        definition_id = json_data.get('definition')
+        definition_id = block_data.get('definition')
 
         # If no usage id is provided, generate an in-memory id
         if block_key is None:
-            block_key = BlockKey(json_data['block_type'], LocalId())
+            block_key = BlockKey(block_data['block_type'], LocalId())
 
         convert_fields = lambda field: self.modulestore.convert_references_to_keys(
             course_key, class_, field, self.course_entry.structure['blocks'],
         )
 
-        if definition_id is not None and not json_data.get('definition_loaded', False):
+        if definition_id is not None and not block_data['definition_loaded']:
             definition_loader = DefinitionLazyLoader(
                 self.modulestore,
                 course_key,
@@ -194,8 +195,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
             block_id=block_key.id,
         )
 
-        converted_fields = convert_fields(json_data.get('fields', {}))
-        converted_defaults = convert_fields(json_data.get('defaults', {}))
+        converted_fields = convert_fields(block_data.get('fields', {}))
+        converted_defaults = convert_fields(block_data.get('defaults', {}))
         if block_key in self._parent_map:
             parent_key = self._parent_map[block_key]
             parent = course_key.make_usage_key(parent_key.type, parent_key.id)
@@ -223,7 +224,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         except Exception:
             log.warning("Failed to load descriptor", exc_info=True)
             return ErrorDescriptor.from_json(
-                json_data,
+                block_data,
                 self,
                 course_entry_override.course_key.make_usage_key(
                     block_type='error',
@@ -232,9 +233,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
                 error_msg=exc_info_to_str(sys.exc_info())
             )
 
-        edit_info = json_data.get('edit_info', {})
-        module._edited_by = edit_info.get('edited_by')
-        module._edited_on = edit_info.get('edited_on')
+        edit_info = block_data.get('edit_info', {})
+        module._edited_by = edit_info.get('edited_by')  # pylint: disable=protected-access
+        module._edited_on = edit_info.get('edited_on')  # pylint: disable=protected-access
         module.previous_version = edit_info.get('previous_version')
         module.update_version = edit_info.get('update_version')
         module.source_version = edit_info.get('source_version', None)

common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py

@@ -8,13 +8,16 @@ import pymongo
 # Import this just to export it
 from pymongo.errors import DuplicateKeyError  # pylint: disable=unused-import
 
-from contracts import check
+from contracts import check, new_contract
 from xmodule.exceptions import HeartbeatFailure
-from xmodule.modulestore.split_mongo import BlockKey
+from xmodule.modulestore.split_mongo import BlockKey, BlockData
 import datetime
 import pytz
 
 
+new_contract('BlockData', BlockData)
+
+
 def structure_from_mongo(structure):
     """
     Converts the 'blocks' key from a list [block_data] to a map
@@ -34,7 +37,7 @@ def structure_from_mongo(structure):
     for block in structure['blocks']:
         if 'children' in block['fields']:
             block['fields']['children'] = [BlockKey(*child) for child in block['fields']['children']]
-        new_blocks[BlockKey(block['block_type'], block.pop('block_id'))] = block
+        new_blocks[BlockKey(block['block_type'], block.pop('block_id'))] = BlockData(block)
     structure['blocks'] = new_blocks
 
     return structure
@@ -49,7 +52,7 @@ def structure_to_mongo(structure):
         directly into mongo.
     """
     check('BlockKey', structure['root'])
-    check('dict(BlockKey: dict)', structure['blocks'])
+    check('dict(BlockKey: BlockData)', structure['blocks'])
     for block in structure['blocks'].itervalues():
         if 'children' in block['fields']:
             check('list(BlockKey)', block['fields']['children'])
@@ -58,7 +61,7 @@ def structure_to_mongo(structure):
     new_structure['blocks'] = []
 
     for block_key, block in structure['blocks'].iteritems():
-        new_block = dict(block)
+        new_block = dict(block.to_storable())
         new_block.setdefault('block_type', block_key.type)
         new_block['block_id'] = block_key.id
         new_structure['blocks'].append(new_block)

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -53,7 +53,6 @@ Representation:
         definition. Acts as a pseudo-object identifier.
 """
 import copy
-import threading
 import datetime
 import hashlib
 import logging
@@ -79,7 +78,7 @@ from xmodule.modulestore import (
 from ..exceptions import ItemNotFoundError
 from .caching_descriptor_system import CachingDescriptorSystem
 from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection, DuplicateKeyError
-from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope
+from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope, BlockData
 from xmodule.error_module import ErrorDescriptor
 from collections import defaultdict
 from types import NoneType
@@ -618,10 +617,6 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         self.db_connection = MongoConnection(**doc_store_config)
         self.db = self.db_connection.database
 
-        # Code review question: How should I expire entries?
-        # _add_cache could use a lru mechanism to control the cache size?
-        self.thread_cache = threading.local()
-
         if default_class is not None:
             module_path, __, class_name = default_class.rpartition('.')
             class_ = getattr(import_module(module_path), class_name)
@@ -681,8 +676,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             new_module_data = {}
             for block_id in base_block_ids:
                 new_module_data = self.descendants(
-                    # copy or our changes like setting 'definition_loaded' will affect the active bulk operation data
-                    copy.deepcopy(system.course_entry.structure['blocks']),
+                    system.course_entry.structure['blocks'],
                     block_id,
                     depth,
                     new_module_data
@@ -732,10 +726,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         Find the descriptor cache for this course if it exists
         :param course_version_guid:
         """
-        if not hasattr(self.thread_cache, 'course_cache'):
-            self.thread_cache.course_cache = {}
-        system = self.thread_cache.course_cache
-        return system.get(course_version_guid)
+        if self.request_cache is None:
+            return None
+
+        return self.request_cache.data.setdefault('course_cache', {}).get(course_version_guid)
 
     def _add_cache(self, course_version_guid, system):
         """
@@ -743,9 +737,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         :param course_version_guid:
         :param system:
         """
-        if not hasattr(self.thread_cache, 'course_cache'):
-            self.thread_cache.course_cache = {}
-        self.thread_cache.course_cache[course_version_guid] = system
+        if self.request_cache is not None:
+            self.request_cache.data.setdefault('course_cache', {})[course_version_guid] = system
         return system
 
     def _clear_cache(self, course_version_guid=None):
@@ -753,15 +746,16 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         Should only be used by testing or something which implements transactional boundary semantics.
         :param course_version_guid: if provided, clear only this entry
         """
+        if self.request_cache is None:
+            return
+
         if course_version_guid:
-            if not hasattr(self.thread_cache, 'course_cache'):
-                self.thread_cache.course_cache = {}
             try:
-                del self.thread_cache.course_cache[course_version_guid]
+                del self.request_cache.data.setdefault('course_cache', {})[course_version_guid]
             except KeyError:
                 pass
         else:
-            self.thread_cache.course_cache = {}
+            self.request_cache.data['course_cache'] = {}
 
     def _lookup_course(self, course_key):
         '''
@@ -2337,7 +2331,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
             return result
 
-    @contract(block_key=BlockKey, blocks='dict(BlockKey: dict)')
+    @contract(block_key=BlockKey, blocks='dict(BlockKey: BlockData)')
     def _remove_subtree(self, block_key, blocks):
         """
         Remove the subtree rooted at block_key
@@ -2917,6 +2911,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 self._delete_if_true_orphan(BlockKey(*child), structure)
             del structure['blocks'][orphan]
 
+    @contract(returns=BlockData)
     def _new_block(self, user_id, category, block_fields, definition_id, new_id, raw=False, block_defaults=None):
         """
         Create the core document structure for a block.
@@ -2941,20 +2936,20 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         }
         if block_defaults:
             document['defaults'] = block_defaults
-        return document
+        return BlockData(document)
 
-    @contract(block_key=BlockKey)
+    @contract(block_key=BlockKey, returns='BlockData | None')
     def _get_block_from_structure(self, structure, block_key):
         """
-        Encodes the block id before retrieving it from the structure to ensure it can
+        Encodes the block key before retrieving it from the structure to ensure it can
         be a json dict key.
         """
         return structure['blocks'].get(block_key)
 
-    @contract(block_key=BlockKey)
+    @contract(block_key=BlockKey, content=BlockData)
     def _update_block_in_structure(self, structure, block_key, content):
         """
-        Encodes the block id before accessing it in the structure to ensure it can
+        Encodes the block key before accessing it in the structure to ensure it can
         be a json dict key.
         """
         structure['blocks'][block_key] = content