Merge pull request #5197 from edx/reruns/count_queries

Explain query counts

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -200,12 +200,14 @@ class TestCourseListing(ModuleStoreTestCase):
         self.assertGreaterEqual(iteration_over_courses_time_2.elapsed, iteration_over_groups_time_2.elapsed)
 
         # Now count the db queries
-        store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo)
         with check_mongo_calls(USER_COURSES_COUNT):
             _accessible_courses_list_from_groups(self.request)
 
-        # TODO: LMS-11220: Document why this takes 6 calls
-        with check_mongo_calls(6):
+        # Calls:
+        #    1) query old mongo
+        #    2) get_more on old mongo
+        #    3) query split (but no courses so no fetching of data)
+        with check_mongo_calls(3):
             _accessible_courses_list(self.request)
 
     def test_get_course_list_with_same_course_id(self):
@@ -276,7 +278,7 @@ class TestCourseListing(ModuleStoreTestCase):
         course_location = SlashSeparatedCourseKey('testOrg', 'erroredCourse', 'RunBabyRun')
         course = self._create_course_with_access_groups(course_location, self.user)
         course_db_record = store._find_one(course.location)
-        course_db_record.setdefault('metadata', {}).get('tabs', []).append({"type": "wiko", "name": "Wiki" })
+        course_db_record.setdefault('metadata', {}).get('tabs', []).append({"type": "wiko", "name": "Wiki"})
         store.collection.update(
             {'_id': course.location.to_deprecated_son()},
             {'$set': {

cms/envs/test.py

@@ -73,17 +73,6 @@ STATICFILES_STORAGE='pipeline.storage.NonPackagingPipelineStorage'
 STATIC_URL = "/static/"
 PIPELINE_ENABLED=False
 
-# Add split as another store for testing
-MODULESTORE['default']['OPTIONS']['stores'].append(
-    {
-        'NAME': 'split',
-        'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore',
-        'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
-        'OPTIONS': {
-            'render_template': 'edxmako.shortcuts.render_to_string',
-        }
-    },
-)
 # Update module store settings per defaults for tests
 update_module_store_settings(
     MODULESTORE,

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

@@ -681,6 +681,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             version_guids.append(version_guid)
             id_version_map[version_guid] = course_index
 
+        if not version_guids:
+            return []
+
         matching_structures = self.find_structures_by_id(version_guids)
 
         # get the blocks for each course index (s/b the root)

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

@@ -290,11 +290,9 @@ class TestMixedModuleStore(unittest.TestCase):
                 self.store.create_course('org_x', 'course_y', 'run_z', self.user_id)
 
     # Draft:
-    #    - One lookup to locate an item that exists
-    #    - Two lookups to determine an item doesn't exist (one to check mongo, one to check split)
+    #    problem: One lookup to locate an item that exists
+    #    fake: one w/ wildcard version
     # split has one lookup for the course and then one for the course items
-    # TODO: LMS-11220: Document why draft find count is [1, 1]
-    # TODO: LMS-11220: Document why split find count is [2, 2]
     @ddt.data(('draft', [1, 1], 0), ('split', [2, 2], 0))
     @ddt.unpack
     def test_has_item(self, default_ms, max_find, max_send):
@@ -317,10 +315,12 @@ class TestMixedModuleStore(unittest.TestCase):
         with self.assertRaises(UnsupportedRevisionError):
             self.store.has_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred)
 
-    # draft is 2 to compute inheritance
-    # split is 2 (would be 3 on course b/c it looks up the wiki_slug in definitions)
-    # TODO: LMS-11220: Document why draft find count is [2, 2]
-    # TODO: LMS-11220: Document why split find count is [3, 3]
+    # draft queries:
+    #   problem: find draft item, find all items pertinent to inheritance computation
+    #   non-existent problem: find draft, find published
+    # split:
+    #   problem: active_versions, structure, then active_versions again?
+    #   non-existent problem: ditto
     @ddt.data(('draft', [2, 2], 0), ('split', [3, 3], 0))
     @ddt.unpack
     def test_get_item(self, default_ms, max_find, max_send):
@@ -345,8 +345,10 @@ class TestMixedModuleStore(unittest.TestCase):
         with self.assertRaises(UnsupportedRevisionError):
             self.store.get_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred)
 
-    # compared to get_item for the course, draft asks for both draft and published
-    # TODO: LMS-11220: Document why split find count is 3
+    # Draft:
+    #    wildcard query, 6! load pertinent items for inheritance calls, course root fetch (why)
+    # Split:
+    #    active_versions (with regex), structure, and spurious active_versions refetch
     @ddt.data(('draft', 8, 0), ('split', 3, 0))
     @ddt.unpack
     def test_get_items(self, default_ms, max_find, max_send):
@@ -372,10 +374,11 @@ class TestMixedModuleStore(unittest.TestCase):
                 revision=ModuleStoreEnum.RevisionOption.draft_preferred
             )
 
-    # draft: 2 to look in draft and then published and then 5 for updating ancestors.
-    # split: 3 to get the course structure & the course definition (show_calculator is scope content)
-    #  before the change. 1 during change to refetch the definition. 3 afterward (b/c it calls get_item to return the "new" object).
-    #  2 sends to update index & structure (calculator is a setting field)
+    # draft: get draft, count parents, get parents, count & get grandparents, count & get greatgrand,
+    #        count & get next ancestor (chapter's parent), count non-existent next ancestor, get inheritance
+    #    sends: update problem and then each ancestor up to course (edit info)
+    # split: active_versions, definitions (calculator field), structures
+    #  2 sends to update index & structure (note, it would also be definition if a content field changed)
     @ddt.data(('draft', 11, 5), ('split', 3, 2))
     @ddt.unpack
     def test_update_item(self, default_ms, max_find, max_send):
@@ -638,8 +641,14 @@ class TestMixedModuleStore(unittest.TestCase):
         # Check the parent for changes should return True and not throw an exception
         self.assertTrue(self.store.has_changes(parent))
 
-    # TODO: LMS-11220: Document why split find count is 4
-    # TODO: LMS-11220: Document why split send count is 3
+    # Draft
+    #   Find: find parents (definition.children query), get parent, get course (fill in run?),
+    #         find parents of the parent (course), get inheritance items,
+    #         get errors, get item (to delete subtree), get inheritance again.
+    #   Sends: delete item, update parent
+    # Split
+    #   Find: active_versions, 2 structures (published & draft), definition (unnecessary)
+    #   Sends: updated draft and published structures and active_versions
     @ddt.data(('draft', 8, 2), ('split', 4, 3))
     @ddt.unpack
     def test_delete_item(self, default_ms, max_find, max_send):
@@ -656,11 +665,17 @@ class TestMixedModuleStore(unittest.TestCase):
             self.store.delete_item(self.writable_chapter_location, self.user_id)
 
         # verify it's gone
+        # FIXME check both published and draft branches
         with self.assertRaises(ItemNotFoundError):
             self.store.get_item(self.writable_chapter_location)
 
-    # TODO: LMS-11220: Document why split find count is 4
-    # TODO: LMS-11220: Document why split send count is 3
+    # Draft:
+    #    queries: find parent (definition.children), count versions of item, get parent, count grandparents,
+    #             inheritance items, draft item, draft child, get errors, inheritance
+    #    sends: delete draft vertical and update parent
+    # Split:
+    #    queries: active_versions, draft and published structures, definition (unnecessary)
+    #    sends: update published (why?), draft, and active_versions
     @ddt.data(('draft', 9, 2), ('split', 4, 3))
     @ddt.unpack
     def test_delete_private_vertical(self, default_ms, max_find, max_send):
@@ -706,7 +721,12 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertFalse(self.store.has_item(leaf_loc))
         self.assertNotIn(vert_loc, course.children)
 
-    # TODO: LMS-11220: Document why split find count is 2
+    # Draft:
+    #   find: find parent (definition.children) 2x, find draft item, check error state, get inheritance items
+    #   send: one delete query for specific item
+    # Split:
+    #   find: active_version & structure
+    #   send: update structure and active_versions
     @ddt.data(('draft', 5, 1), ('split', 2, 2))
     @ddt.unpack
     def test_delete_draft_vertical(self, default_ms, max_find, max_send):
@@ -740,9 +760,15 @@ class TestMixedModuleStore(unittest.TestCase):
         with check_mongo_calls(max_find, max_send):
             self.store.delete_item(private_leaf.location, self.user_id)
 
-    # TODO: LMS-11220: Document why split find count is 5
-    # TODO: LMS-11220: Document why draft find count is 4
-    @ddt.data(('draft', 4, 0), ('split', 5, 0))
+    # Draft:
+    #   1) find all courses (wildcard),
+    #   2) get each course 1 at a time (1 course),
+    #   3) wildcard split if it has any (1) but it doesn't
+    # Split:
+    #   1) wildcard split search,
+    #   2-4) active_versions, structure, definition (s/b lazy; so, unnecessary)
+    #   5) wildcard draft mongo which has none
+    @ddt.data(('draft', 3, 0), ('split', 5, 0))
     @ddt.unpack
     def test_get_courses(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
@@ -785,9 +811,8 @@ class TestMixedModuleStore(unittest.TestCase):
         with self.assertRaises(AttributeError):
             xml_store.create_course("org", "course", "run", self.user_id)
 
-    # draft is 2 to compute inheritance
-    # split is 3 (one for the index, one for the definition to check if the wiki is set, and one for the course structure
-    # TODO: LMS-11220: Document why split find count is 4
+    # draft is 2: find out which ms owns course, get item
+    # split: find out which ms owns course, active_versions, structure, definition (definition s/b unnecessary unless lazy is false)
     @ddt.data(('draft', 2, 0), ('split', 4, 0))
     @ddt.unpack
     def test_get_course(self, default_ms, max_find, max_send):
@@ -805,7 +830,8 @@ class TestMixedModuleStore(unittest.TestCase):
 
     # notice this doesn't test getting a public item via draft_preferred which draft would have 2 hits (split
     # still only 2)
-    # TODO: LMS-11220: Document why draft find count is 2
+    # Draft: count via definition.children query, then fetch via that query
+    # Split: active_versions, structure
     @ddt.data(('draft', 2, 0), ('split', 2, 0))
     @ddt.unpack
     def test_get_parent_locations(self, default_ms, max_find, max_send):
@@ -902,20 +928,22 @@ class TestMixedModuleStore(unittest.TestCase):
             (child_to_delete_location, None, ModuleStoreEnum.RevisionOption.published_only),
         ])
 
-    # Mongo reads:
-    #    First location:
-    #        - count problem (1)
-    #        - For each level of ancestors: (5)
-    #          - Count ancestor
-    #          - retrieve ancestor
-    #        - compute inheritable data
-    #    Second location:
-    #        - load vertical
-    #        - load inheritance data
-
-    # TODO: LMS-11220: Document why draft send count is 5
-    # TODO: LMS-11220: Document why draft find count is [19, 6]
-    # TODO: LMS-11220: Document why split find count is [2, 2]
+    # Draft:
+    #   Problem path:
+    #    1. Get problem
+    #    2-3. count matches definition.children called 2x?
+    #    4. get parent via definition.children query
+    #    5-7. 2 counts and 1 get grandparent via definition.children
+    #    8-10. ditto for great-grandparent
+    #    11-13. ditto for next ancestor
+    #    14. fail count query looking for parent of course (unnecessary)
+    #    15. get course record direct query (not via definition.children) (already fetched in 13)
+    #    16. get items for inheritance computation
+    #    17. get vertical (parent of problem)
+    #    18. get items for inheritance computation (why? caching should handle)
+    #    19-20. get vertical_x1b (? why? this is the only ref in trace) & items for inheritance computation
+    #   Chapter path: get chapter, count parents 2x, get parents, count non-existent grandparents
+    # Split: active_versions & structure
     @ddt.data(('draft', [20, 5], 0), ('split', [2, 2], 0))
     @ddt.unpack
     def test_path_to_location(self, default_ms, num_finds, num_sends):
@@ -936,6 +964,7 @@ class TestMixedModuleStore(unittest.TestCase):
             )
 
             for location, expected in should_work:
+                # each iteration has different find count, pop this iter's find count
                 with check_mongo_calls(num_finds.pop(0), num_sends):
                     self.assertEqual(path_to_location(self.store, location), expected)
 
@@ -1074,6 +1103,8 @@ class TestMixedModuleStore(unittest.TestCase):
         # It does not discard the child vertical, even though that child is a draft (with no published version)
         self.assertEqual(num_children, len(reverted_parent.children))
 
+    # Draft: get all items which can be or should have parents
+    # Split: active_versions, structure
     @ddt.data(('draft', 1, 0), ('split', 2, 0))
     @ddt.unpack
     def test_get_orphans(self, default_ms, max_find, max_send):
@@ -1212,8 +1243,8 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertEqual(self.user_id, block.subtree_edited_by)
         self.assertGreater(datetime.datetime.now(UTC), block.subtree_edited_on)
 
-    # TODO: LMS-11220: Document why split find count is 2
-    # TODO: LMS-11220: Document why draft find count is 2
+    # Draft: wildcard search of draft and split
+    # Split: wildcard search of draft and split
     @ddt.data(('draft', 2, 0), ('split', 2, 0))
     @ddt.unpack
     def test_get_courses_for_wiki(self, default_ms, max_find, max_send):
@@ -1242,15 +1273,16 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertEqual(len(self.store.get_courses_for_wiki('edX.simple.2012_Fall')), 0)
         self.assertEqual(len(self.store.get_courses_for_wiki('no_such_wiki')), 0)
 
-    # Mongo reads:
-    #    - load vertical
-    #    - load vertical children
-    #    - get last error
-    # Split takes 1 query to read the course structure, deletes all of the entries in memory, and loads the module from an in-memory cache
+    # Draft:
+    #    Find: find vertical, find children, get last error
+    #    Sends:
+    #      1. delete all of the published nodes in subtree
+    #      2. insert vertical as published (deleted in step 1) w/ the deleted problems as children
+    #      3-6. insert the 3 problems and 1 html as published
+    # Split: active_versions, 2 structures (pre & post published?)
     # Sends:
     #    - insert structure
     #    - write index entry
-    # TODO: LMS-11220: Document why split find count is 3
     @ddt.data(('draft', 3, 6), ('split', 3, 2))
     @ddt.unpack
     def test_unpublish(self, default_ms, max_find, max_send):
@@ -1285,6 +1317,8 @@ class TestMixedModuleStore(unittest.TestCase):
         )
         self.assertIsNotNone(draft_xblock)
 
+    # Draft: specific query for revision None
+    # Split: active_versions, structure
     @ddt.data(('draft', 1, 0), ('split', 2, 0))
     @ddt.unpack
     def test_has_published_version(self, default_ms, max_find, max_send):
@@ -1727,9 +1761,9 @@ class TestMixedModuleStore(unittest.TestCase):
             with self.store.default_store(fake_store):
                 pass  # pragma: no cover
 
-#=============================================================================================================
+# ============================================================================================================
 # General utils for not using django settings
-#=============================================================================================================
+# ============================================================================================================
 
 
 def load_function(path):

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

@@ -89,11 +89,21 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         """
         vert_location = self.old_course_key.make_usage_key('vertical', block_id='Vert1')
         item = self.draft_mongo.get_item(vert_location, 2)
-        # Vert1 has 3 children; so, publishes 4 nodes which may mean 4 inserts & 1 bulk remove
-        # TODO: LMS-11220: Document why find count is 25
-        # 25-June-2014 find calls are 19. Probably due to inheritance recomputation?
-        # 02-July-2014 send calls are 7. 5 from above, plus 2 for updating subtree edit info for Chapter1 and course
-        #              find calls are 22. 19 from above, plus 3 for finding the parent of Vert1, Chapter1, and course
+        # Finds:
+        #   1 get draft vert,
+        #   2-10 for each child: (3 children x 3 queries each)
+        #      get draft and then published child
+        #      compute inheritance
+        #   11 get published vert
+        #   12-15 get each ancestor (count then get): (2 x 2),
+        #   16 then fail count of course parent (1)
+        #   17 compute inheritance
+        #   18 get last error
+        #   19-20 get draft and published vert
+        # Sends:
+        #   delete the subtree of drafts (1 call),
+        #   update the published version of each node in subtree (4 calls),
+        #   update the ancestors up to course (2 calls)
         with check_mongo_calls(20, 7):
             self.draft_mongo.publish(item.location, self.user_id)