From b137f8d7ad7fd09e7a0dc0cce8417cf76706ba55 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 12 Sep 2014 11:52:35 -0400 Subject: [PATCH] Explain query counts Fix trivial excesses (2 splits configured & don't fetch courses if there are none) LMS-11220, LMS-11391, LMS-11390 --- .../contentstore/tests/test_course_listing.py | 10 +- cms/envs/test.py | 11 -- .../xmodule/modulestore/split_mongo/split.py | 3 + .../tests/test_mixed_modulestore.py | 134 +++++++++++------- .../xmodule/modulestore/tests/test_publish.py | 20 ++- 5 files changed, 108 insertions(+), 70 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 7622e2d54b..d8bf286a60 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/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': { diff --git a/cms/envs/test.py b/cms/envs/test.py index 94e6889db5..9633751656 100644 --- a/cms/envs/test.py +++ b/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, diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index e26f59fc91..aff87bd255 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index f42b40e654..0487aca15a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index 24a85a9cad..2dc83dd006 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/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)