Pre-load Definitions in Split Modulestore when accessing all blocks

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

@@ -432,9 +432,11 @@ def get_definitions(self, course_key, ids):
 
         if len(ids):
             # Query the db for the definitions.
-            defs_from_db = self.db_connection.get_definitions(list(ids), course_key)
+            defs_from_db = list(self.db_connection.get_definitions(list(ids), course_key))
+            defs_dict = {d.get('_id'): d for d in defs_from_db}
             # Add the retrieved definitions to the cache.
-            bulk_write_record.definitions.update({d.get('_id'): d for d in defs_from_db})
+            bulk_write_record.definitions_in_db.update(defs_dict.iterkeys())
+            bulk_write_record.definitions.update(defs_dict)
             definitions.extend(defs_from_db)
         return definitions
 
@@ -772,11 +774,16 @@ def _load_items(self, course_entry, block_keys, depth=0, **kwargs):
         Load the definitions into each block if lazy is in kwargs and is False;
         otherwise, do not load the definitions - they'll be loaded later when needed.
         """
+        lazy = kwargs.pop('lazy', True)
+        should_cache_items = not lazy
+
         runtime = self._get_cache(course_entry.structure['_id'])
         if runtime is None:
-            lazy = kwargs.pop('lazy', True)
             runtime = self.create_runtime(course_entry, lazy)
             self._add_cache(course_entry.structure['_id'], runtime)
+            should_cache_items = True
+
+        if should_cache_items:
             self.cache_items(runtime, block_keys, course_entry.course_key, depth, lazy)
 
         return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]

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

@@ -13,7 +13,8 @@
 from xmodule.modulestore.tests.factories import check_mongo_calls
 from xmodule.modulestore.tests.utils import (
     MixedModulestoreBuilder, VersioningModulestoreBuilder,
-    MongoModulestoreBuilder, TEST_DATA_DIR
+    MongoModulestoreBuilder, TEST_DATA_DIR,
+    MemoryCache,
 )
 
 MIXED_OLD_MONGO_MODULESTORE_BUILDER = MixedModulestoreBuilder([('draft', MongoModulestoreBuilder())])
@@ -87,6 +88,46 @@ class CountMongoCallsCourseTraversal(TestCase):
     to the leaf nodes.
     """
 
+    def _traverse_blocks_in_course(self, course, access_all_block_fields):
+        """
+        Traverses all the blocks in the given course.
+        If access_all_block_fields is True, also reads all the
+        xblock fields in each block in the course.
+        """
+        all_blocks = []
+        stack = [course]
+        while stack:
+            curr_block = stack.pop()
+            all_blocks.append(curr_block)
+            if curr_block.has_children:
+                for block in reversed(curr_block.get_children()):
+                    stack.append(block)
+
+        if access_all_block_fields:
+            # Read the fields on each block in order to ensure each block and its definition is loaded.
+            for xblock in all_blocks:
+                for __, field in xblock.fields.iteritems():
+                    if field.is_set_on(xblock):
+                        __ = field.read_from(xblock)
+
+    def _import_course(self, content_store, modulestore):
+        """
+        Imports a course for testing.
+        Returns the course key.
+        """
+        course_key = modulestore.make_course_key('a', 'course', 'course')
+        import_course_from_xml(
+            modulestore,
+            'test_user',
+            TEST_DATA_DIR,
+            source_dirs=['manual-testing-complete'],
+            static_content_store=content_store,
+            target_id=course_key,
+            create_if_not_present=True,
+            raise_on_failure=True,
+        )
+        return course_key
+
     # Suppose you want to traverse a course - maybe accessing the fields of each XBlock in the course,
     # maybe not. What parameters should one use for get_course() in order to minimize the number of
     # mongo calls? The tests below both ensure that code changes don't increase the number of mongo calls
@@ -109,52 +150,43 @@ class CountMongoCallsCourseTraversal(TestCase):
         # (if you'll eventually access all the fields and load all the definitions anyway).
         (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, True, 4),
         (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, True, 38),
-        (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 131),
+        (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 38),
         (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, True, 38),
         (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 4),
         (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 4),
-        # TODO: The call count below seems like a bug - should be 4?
-        # Seems to be related to using self.lazy in CachingDescriptorSystem.get_module_data().
-        (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 131),
+        (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 4),
         (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 4)
     )
     @ddt.unpack
-    def test_number_mongo_calls(self, store, depth, lazy, access_all_block_fields, num_mongo_calls):
-        with store.build() as (source_content, source_store):
-
-            source_course_key = source_store.make_course_key('a', 'course', 'course')
-
-            # First, import a course.
-            import_course_from_xml(
-                source_store,
-                'test_user',
-                TEST_DATA_DIR,
-                source_dirs=['manual-testing-complete'],
-                static_content_store=source_content,
-                target_id=source_course_key,
-                create_if_not_present=True,
-                raise_on_failure=True,
-            )
+    def test_number_mongo_calls(self, store_builder, depth, lazy, access_all_block_fields, num_mongo_calls):
+        request_cache = MemoryCache()
+        with store_builder.build(request_cache=request_cache) as (content_store, modulestore):
+            course_key = self._import_course(content_store, modulestore)
 
             # Course traversal modeled after the traversal done here:
             # lms/djangoapps/mobile_api/video_outlines/serializers.py:BlockOutline
             # Starting at the root course block, do a breadth-first traversal using
             # get_children() to retrieve each block's children.
             with check_mongo_calls(num_mongo_calls):
-                with source_store.bulk_operations(source_course_key):
-                    start_block = source_store.get_course(source_course_key, depth=depth, lazy=lazy)
-                    all_blocks = []
-                    stack = [start_block]
-                    while stack:
-                        curr_block = stack.pop()
-                        all_blocks.append(curr_block)
-                        if curr_block.has_children:
-                            for block in reversed(curr_block.get_children()):
-                                stack.append(block)
-
-                    if access_all_block_fields:
-                        # Read the fields on each block in order to ensure each block and its definition is loaded.
-                        for xblock in all_blocks:
-                            for __, field in xblock.fields.iteritems():
-                                if field.is_set_on(xblock):
-                                    __ = field.read_from(xblock)
+                with modulestore.bulk_operations(course_key):
+                    start_block = modulestore.get_course(course_key, depth=depth, lazy=lazy)
+                    self._traverse_blocks_in_course(start_block, access_all_block_fields)
+
+    @ddt.data(
+        (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 176),
+        (MIXED_SPLIT_MODULESTORE_BUILDER, 5),
+    )
+    @ddt.unpack
+    def test_lazy_when_course_previously_cached(self, store_builder, num_mongo_calls):
+        request_cache = MemoryCache()
+        with store_builder.build(request_cache=request_cache) as (content_store, modulestore):
+            course_key = self._import_course(content_store, modulestore)
+
+            with check_mongo_calls(num_mongo_calls):
+                with modulestore.bulk_operations(course_key):
+                    # assume the course was retrieved earlier
+                    course = modulestore.get_course(course_key, depth=0, lazy=True)
+
+                    # and then subsequently retrieved with the lazy and depth=None values
+                    course = modulestore.get_item(course.location, depth=None, lazy=False)
+                    self._traverse_blocks_in_course(course, access_all_block_fields=True)

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

@@ -1,3 +1,7 @@
+"""
+Tests for bulk operations in Split Modulestore.
+"""
+# pylint: disable=protected-access
 import copy
 import ddt
 import unittest
@@ -444,6 +448,17 @@ def test_get_definitions(self, search_ids, active_ids, db_ids):
             else:
                 self.assertNotIn(db_definition(_id), results)
 
+    def test_get_definitions_doesnt_update_db(self):
+        test_ids = [1, 2]
+        db_definition = lambda _id: {'db': 'definition', '_id': _id}
+
+        db_definitions = [db_definition(_id) for _id in test_ids]
+        self.conn.get_definitions.return_value = db_definitions
+        self.bulk._begin_bulk_operation(self.course_key)
+        self.bulk.get_definitions(self.course_key, test_ids)
+        self.bulk._end_bulk_operation(self.course_key)
+        self.assertFalse(self.conn.insert_definition.called)
+
     def test_no_bulk_find_structures_derived_from(self):
         ids = [Mock(name='id')]
         self.conn.find_structures_derived_from.return_value = [MagicMock(name='result')]

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

@@ -194,7 +194,7 @@ class MemoryCache(object):
     the modulestore, and stores the data in a dictionary in memory.
     """
     def __init__(self):
-        self._data = {}
+        self.data = {}
 
     def get(self, key, default=None):
         """
@@ -204,7 +204,7 @@ def get(self, key, default=None):
             key: The key to update.
             default: The value to return if the key hasn't been set previously.
         """
-        return self._data.get(key, default)
+        return self.data.get(key, default)
 
     def set(self, key, value):
         """
@@ -214,7 +214,7 @@ def set(self, key, value):
             key: The key to update.
             value: The value change the key to.
         """
-        self._data[key] = value
+        self.data[key] = value
 
 
 class MongoContentstoreBuilder(object):
@@ -255,19 +255,19 @@ def build(self, **kwargs):
         """
         contentstore = kwargs.pop('contentstore', None)
         if not contentstore:
-            with self.build_without_contentstore() as (contentstore, modulestore):
+            with self.build_without_contentstore(**kwargs) as (contentstore, modulestore):
                 yield contentstore, modulestore
         else:
-            with self.build_with_contentstore(contentstore) as modulestore:
+            with self.build_with_contentstore(contentstore, **kwargs) as modulestore:
                 yield modulestore
 
     @contextmanager
-    def build_without_contentstore(self):
+    def build_without_contentstore(self, **kwargs):
         """
         Build both the contentstore and the modulestore.
         """
         with MongoContentstoreBuilder().build() as contentstore:
-            with self.build_with_contentstore(contentstore) as modulestore:
+            with self.build_with_contentstore(contentstore, **kwargs) as modulestore:
                 yield contentstore, modulestore
 
 
@@ -276,7 +276,7 @@ class MongoModulestoreBuilder(StoreBuilderBase):
     A builder class for a DraftModuleStore.
     """
     @contextmanager
-    def build_with_contentstore(self, contentstore):
+    def build_with_contentstore(self, contentstore, **kwargs):
         """
         A contextmanager that returns an isolated mongo modulestore, and then deletes
         all of its data at the end of the context.
@@ -324,7 +324,7 @@ class VersioningModulestoreBuilder(StoreBuilderBase):
     A builder class for a VersioningModuleStore.
     """
     @contextmanager
-    def build_with_contentstore(self, contentstore):
+    def build_with_contentstore(self, contentstore, **kwargs):
         """
         A contextmanager that returns an isolated versioning modulestore, and then deletes
         all of its data at the end of the context.
@@ -347,6 +347,7 @@ def build_with_contentstore(self, contentstore):
             fs_root,
             render_template=repr,
             xblock_mixins=XBLOCK_MIXINS,
+            **kwargs
         )
         modulestore.ensure_indexes()
 
@@ -369,7 +370,7 @@ class XmlModulestoreBuilder(StoreBuilderBase):
     """
     # pylint: disable=unused-argument
     @contextmanager
-    def build_with_contentstore(self, contentstore=None, course_ids=None):
+    def build_with_contentstore(self, contentstore=None, course_ids=None, **kwargs):
         """
         A contextmanager that returns an isolated xml modulestore
 
@@ -403,7 +404,7 @@ def __init__(self, store_builders, mappings=None):
         self.mixed_modulestore = None
 
     @contextmanager
-    def build_with_contentstore(self, contentstore):
+    def build_with_contentstore(self, contentstore, **kwargs):
         """
         A contextmanager that returns a mixed modulestore built on top of modulestores
         generated by other builder classes.
@@ -414,7 +415,7 @@ def build_with_contentstore(self, contentstore):
         """
         names, generators = zip(*self.store_builders)
 
-        with nested(*(gen.build_with_contentstore(contentstore) for gen in generators)) as modulestores:
+        with nested(*(gen.build_with_contentstore(contentstore, **kwargs) for gen in generators)) as modulestores:
             # Make the modulestore creation function just return the already-created modulestores
             store_iterator = iter(modulestores)
             next_modulestore = lambda *args, **kwargs: store_iterator.next()

lms/djangoapps/course_api/blocks/tests/test_api.py

@@ -2,12 +2,14 @@
 Tests for Blocks api.py
 """
 
+import ddt
 from django.test.client import RequestFactory
 
+from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache
 from student.tests.factories import UserFactory
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
-from xmodule.modulestore.tests.factories import SampleCourseFactory
+from xmodule.modulestore.tests.factories import SampleCourseFactory, check_mongo_calls
 
 from ..api import get_blocks
 
@@ -19,7 +21,8 @@ class TestGetBlocks(SharedModuleStoreTestCase):
     @classmethod
     def setUpClass(cls):
         super(TestGetBlocks, cls).setUpClass()
-        cls.course = SampleCourseFactory.create()
+        with cls.store.default_store(ModuleStoreEnum.Type.split):
+            cls.course = SampleCourseFactory.create()
 
         # hide the html block
         cls.html_block = cls.store.get_item(cls.course.id.make_usage_key('html', 'html_x1a_1'))
@@ -95,3 +98,47 @@ def test_filtering_by_block_types(self):
         self.assertEquals(len(blocks['blocks']), 3)
         for block in blocks['blocks'].itervalues():
             self.assertEqual(block['type'], 'problem')
+
+
+@ddt.ddt
+class TestGetBlocksQueryCounts(SharedModuleStoreTestCase):
+    """
+    Tests query counts for the get_blocks function.
+    """
+    def setUp(self):
+        super(TestGetBlocksQueryCounts, self).setUp()
+
+        self.user = UserFactory.create()
+        self.request = RequestFactory().get("/dummy")
+        self.request.user = self.user
+
+    def _create_course(self, store_type):
+        """
+        Creates the sample course in the given store type.
+        """
+        with self.store.default_store(store_type):
+            return SampleCourseFactory.create()
+
+    def _get_blocks(self, course, expected_mongo_queries):
+        """
+        Verifies the number of expected queries when calling
+        get_blocks on the given course.
+        """
+        with check_mongo_calls(expected_mongo_queries):
+            with self.assertNumQueries(2):
+                get_blocks(self.request, course.location, self.user)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_query_counts_cached(self, store_type):
+        course = self._create_course(store_type)
+        self._get_blocks(course, expected_mongo_queries=0)
+
+    @ddt.data(
+        (ModuleStoreEnum.Type.mongo, 5),
+        (ModuleStoreEnum.Type.split, 3),
+    )
+    @ddt.unpack
+    def test_query_counts_uncached(self, store_type, expected_mongo_queries):
+        course = self._create_course(store_type)
+        clear_course_from_cache(course.id)
+        self._get_blocks(course, expected_mongo_queries)