Cutover to read course index data from MySQL

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -163,8 +163,8 @@ def test_courses_list_with_ccx_courses(self):
             self.assertEqual(len(list(courses_iter)), 0)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.split, 3),
-        (ModuleStoreEnum.Type.mongo, 2)
+        (ModuleStoreEnum.Type.split, 2),
+        (ModuleStoreEnum.Type.mongo, 1)
     )
     @ddt.unpack
     def test_staff_course_listing(self, default_store, mongo_calls):

cms/djangoapps/contentstore/views/tests/test_course_index.py

@@ -403,13 +403,13 @@ def check_index_page(self, separate_archived_courses, org):
 
     @ddt.data(
         # Staff user has course staff access
-        (True, 'staff', None, 1, 19),
-        (False, 'staff', None, 1, 19),
+        (True, 'staff', None, 0, 20),
+        (False, 'staff', None, 0, 20),
         # Base user has global staff access
-        (True, 'user', ORG, 3, 18),
-        (False, 'user', ORG, 3, 18),
-        (True, 'user', None, 3, 18),
-        (False, 'user', None, 3, 18),
+        (True, 'user', ORG, 1, 20),
+        (False, 'user', ORG, 1, 20),
+        (True, 'user', None, 1, 20),
+        (False, 'user', None, 1, 20),
     )
     @ddt.unpack
     def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries):

cms/djangoapps/contentstore/views/tests/test_item.py

@@ -2560,7 +2560,7 @@ def test_json_responses(self):
         self.validate_course_xblock_info(json_response, course_outline=True)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.split, 4, 4),
+        (ModuleStoreEnum.Type.split, 3, 3),
         (ModuleStoreEnum.Type.mongo, 5, 7),
     )
     @ddt.unpack

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

@@ -12,6 +12,7 @@
 from contextlib import contextmanager
 from time import time
 
+from ccx_keys.locator import CCXLocator
 from django.core.cache import caches, InvalidCacheBackendError
 from django.db.transaction import TransactionManagementError
 import pymongo
@@ -471,8 +472,7 @@ def update_course_index(self, course_index, from_index=None, course_context=None
             if not last_update_already_set:
                 course_index['last_update'] = datetime.datetime.now(pytz.utc)
             # Update the course index:
-            result = self.course_index.replace_one(query, course_index, upsert=False,)
-            return result.modified_count == 1
+            self.course_index.replace_one(query, course_index, upsert=False,)
 
     def delete_course_index(self, course_key):
         """
@@ -588,11 +588,7 @@ def get_course_index(self, key, ignore_case=False):
         """
         Get the course_index from the persistence mechanism whose id is the given key
         """
-        #######################
-        # TEMP: as we migrate, we are currently reading from MongoDB only, but writing to both MySQL + MongoDB
-        return super().get_course_index(key, ignore_case=ignore_case)
-        #######################
-        if key.version_guid and not key.org:  # pylint: disable=unreachable
+        if key.version_guid and not key.org:
             # I don't think it was intentional, but with the MongoPersistenceBackend, using a key with only a version
             # guid and no org/course/run value would not raise an error, but would always return None. So we need to be
             # compatible with that.
@@ -611,6 +607,17 @@ def get_course_index(self, key, ignore_case=False):
         try:
             return SplitModulestoreCourseIndex.objects.get(**query).as_v1_schema()
         except SplitModulestoreCourseIndex.DoesNotExist:
+            # The mongo implementation does not retrieve by string key; it retrieves by (org, course, run) tuple.
+            # As a result, it will handle read requests for a CCX key like
+            #   ccx-v1:org.0+course_0+Run_0+branch@published-branch+ccx@1
+            # identically to the corresponding course key. This seems to be an oversight though, not an intentional
+            # feature, as the CCXModulestoreWrapper is supposed to "hide" CCX keys from the underlying modulestore.
+            # Anyhow, for compatbility we need to do the same:
+            if isinstance(key, CCXLocator):
+                log.warning(
+                    f"A CCX key leaked through to the underlying modulestore, bypassing CCXModulestoreWrapper: {key}"
+                )
+                return self.get_course_index(key.to_course_locator(), ignore_case)
             return None
 
     def find_matching_course_indexes(  # pylint: disable=arguments-differ
@@ -632,10 +639,6 @@ def find_matching_course_indexes(  # pylint: disable=arguments-differ
             org_target: If specified, this is an ORG filter so that only course_indexs are
                 returned for the specified ORG
         """
-        #######################
-        # TEMP: as we migrate, we are currently reading from MongoDB only, but writing to both MySQL + MongoDB
-        force_mongo = True
-        #######################
         if force_mongo:
             # For data migration purposes, this argument will read from MongoDB instead of MySQL
             return super().find_matching_course_indexes(
@@ -688,31 +691,17 @@ def update_course_index(self, course_index, from_index=None, course_context=None
         RequestCache(namespace="course_index_cache").clear()
         course_index['last_update'] = datetime.datetime.now(pytz.utc)
         # Find the SplitModulestoreCourseIndex entry that we'll be updating:
-        try:
-            index_obj = SplitModulestoreCourseIndex.objects.get(objectid=course_index["_id"])
-        except SplitModulestoreCourseIndex.DoesNotExist:
-            #######################
-            # TEMP: Maybe the data migration hasn't (completely) run yet?
-            data = SplitModulestoreCourseIndex.fields_from_v1_schema(course_index)
-            if super().get_course_index(data["course_id"]) is None:
-                raise  # This course doesn't exist in MySQL or in MongoDB
-            # This course record exists in MongoDB but not yet in MySQL
-            index_obj = SplitModulestoreCourseIndex(**data)
-            if from_index:
-                index_obj.last_update = from_index["last_update"]  # Make sure this won't get marked as a collision
-            #######################
+        index_obj = SplitModulestoreCourseIndex.objects.get(objectid=course_index["_id"])
 
         # Check for collisions:
-        # Except this collision logic doesn't work when using both MySQL and MongoDB together, one for writes and one
-        # for reads, so we're temporarily defering to Mongo's colision logic.
-        # if from_index and index_obj.last_update != from_index["last_update"]:
-        #     # "last_update not only tells us when this course was last updated but also helps prevent collisions"
-        #     log.warning(
-        #         "Collision in Split Mongo when applying course index. This can happen in dev if django debug toolbar "
-        #         "is enabled, as it slows down parallel queries. \nNew index was: %s\nFrom index was: %s",
-        #         course_index, from_index,
-        #     )
-        #     return  # Collision; skip this update
+        if from_index and index_obj.last_update != from_index["last_update"]:
+            # "last_update not only tells us when this course was last updated but also helps prevent collisions"
+            log.warning(
+                "Collision in Split Mongo when applying course index. This can happen in dev if django debug toolbar "
+                "is enabled, as it slows down parallel queries. New index was: %s",
+                course_index,
+            )
+            return  # Collision; skip this update
 
         # Apply updates to the index entry. While doing so, track which branch versions were changed (if any).
         changed_branches = []
@@ -735,13 +724,10 @@ def update_course_index(self, course_index, from_index=None, course_context=None
             # which branch(es) were changed, not anything more useful than that.
             index_obj._change_reason = f'Updated {" and ".join(changed_branches)} branch'  # pylint: disable=protected-access
 
+        # Save the course index entry and create a historical record:
+        index_obj.save()
         # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well:
-        mongo_updated = super().update_course_index(
-            course_index, from_index, course_context, last_update_already_set=True
-        )
-        if mongo_updated:
-            # Save the course index entry and create a historical record:
-            index_obj.save()
+        super().update_course_index(course_index, from_index, course_context, last_update_already_set=True)
 
     def delete_course_index(self, course_key):
         """

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

@@ -37,7 +37,7 @@ def test_create_library(self):
         -> INSERT into SplitModulestoreCourseIndex to save the new library
         -> INSERT a historical record of the SplitModulestoreCourseIndex
         """
-        with check_mongo_calls(2, 3), self.assertNumQueries(3):
+        with check_mongo_calls(0, 3), self.assertNumQueries(5):
             LibraryFactory.create(modulestore=self.store)
 
     def test_duplicate_library(self):

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

@@ -368,7 +368,7 @@ def test_duplicate_course_error_with_different_case_ids(self, default_store):
     #    fake: one w/ wildcard version
     # split: has one lookup for the course and then one for the course items
     #    but the active_versions check is done in MySQL
-    @ddt.data((ModuleStoreEnum.Type.mongo, [1, 1], 0), (ModuleStoreEnum.Type.split, [2, 1], 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, [1, 1], 0), (ModuleStoreEnum.Type.split, [1, 1], 0))
     @ddt.unpack
     def test_has_item(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
@@ -391,17 +391,17 @@ def test_has_item(self, default_ms, max_find, max_send):
     # split:
     #   problem: active_versions, structure
     #   non-existent problem: ditto
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, [3, 2], 0), (ModuleStoreEnum.Type.split, 0, [2, 1], 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, [0, 0], [3, 2], 0), (ModuleStoreEnum.Type.split, [1, 0], [1, 1], 0))
     @ddt.unpack
     def test_get_item(self, default_ms, num_mysql, max_find, max_send):
         self.initdb(default_ms)
         self._create_block_hierarchy()
 
-        with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql):
+        with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql.pop(0)):
             assert self.store.get_item(self.problem_x1a_1) is not None  # lint-amnesty, pylint: disable=no-member
 
         # try negative cases
-        with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql):
+        with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql.pop(0)):
             with pytest.raises(ItemNotFoundError):
                 self.store.get_item(self.fake_location)
 
@@ -414,7 +414,7 @@ def test_get_item(self, default_ms, num_mysql, max_find, max_send):
     # Split:
     #    mysql: fetch course's active version from SplitModulestoreCourseIndex, spurious refetch x2
     #    find: get structure
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 14, 0), (ModuleStoreEnum.Type.split, 0, 3, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 14, 0), (ModuleStoreEnum.Type.split, 2, 1, 0))
     @ddt.unpack
     def test_get_items(self, default_ms, num_mysql, max_find, max_send):
         self.initdb(default_ms)
@@ -917,7 +917,7 @@ def test_has_changes_missing_child(self, default_ms, default_branch):
     #   mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record
     #   Find: active_versions, 2 structures (published & draft), definition (unnecessary)
     #   Sends: updated draft and published structures and active_versions
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 2), (ModuleStoreEnum.Type.split, 3, 3, 3))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 2), (ModuleStoreEnum.Type.split, 4, 2, 3))
     @ddt.unpack
     def test_delete_item(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -946,7 +946,7 @@ def test_delete_item(self, default_ms, num_mysql, max_find, max_send):
     #    mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record
     #    find: draft and published structures, definition (unnecessary)
     #    sends: update published (why?), draft, and active_versions
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 9, 2), (ModuleStoreEnum.Type.split, 3, 4, 3))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 9, 2), (ModuleStoreEnum.Type.split, 4, 3, 3))
     @ddt.unpack
     def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -1000,7 +1000,7 @@ def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send
     #   mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record
     #   find: structure (cached)
     #   send: update structure and active_versions
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 4, 1), (ModuleStoreEnum.Type.split, 3, 2, 2))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 4, 1), (ModuleStoreEnum.Type.split, 4, 1, 2))
     @ddt.unpack
     def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -1043,7 +1043,7 @@ def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send):
     #          executed twice, possibly unnecessarily)
     #   find: 2 reads of structure, definition (s/b lazy; so, unnecessary),
     #         plus 1 wildcard find in draft mongo which has none
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 0), (ModuleStoreEnum.Type.split, 0, 5, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 1, 2, 0), (ModuleStoreEnum.Type.split, 2, 3, 0))
     @ddt.unpack
     def test_get_courses(self, default_ms, num_mysql, max_find, max_send):
         self.initdb(default_ms)
@@ -1083,7 +1083,7 @@ def test_create_child_detached_tabs(self, default_ms):
 
     # draft is 2: find out which ms owns course, get item
     # split: active_versions (mysql), structure, definition (to load course wiki string)
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 0, 3, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 1, 2, 0))
     @ddt.unpack
     def test_get_course(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -1120,7 +1120,7 @@ def test_get_library(self, default_ms):
     # still only 2)
     # Draft: get_parent
     # Split: active_versions, structure
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 2, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0))
     @ddt.unpack
     def test_get_parent_locations(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -1638,7 +1638,7 @@ def test_get_parent_location_draft(self, default_ms):
     #    8-9. get vertical, compute inheritance
     #    10-11. get other vertical_x1b (why?) and compute inheritance
     # Split: loading structure from mongo (also loads active version from MySQL, not tracked here)
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, [12, 3], 0), (ModuleStoreEnum.Type.split, 0, [3, 1], 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, [0, 0], [12, 3], 0), (ModuleStoreEnum.Type.split, [1, 0], [2, 1], 0))
     @ddt.unpack
     def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends):
         """
@@ -1659,7 +1659,7 @@ def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends):
 
             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.assertNumQueries(num_mysql):
+                with check_mongo_calls(num_finds.pop(0), num_sends), self.assertNumQueries(num_mysql.pop(0)):
                     path = path_to_location(self.store, location)
                     assert path == expected
 
@@ -1878,7 +1878,7 @@ def _get_split_modulestore(self):
 
     # Draft: get all items which can be or should have parents
     # Split: active_versions (mysql), structure (mongo)
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 2, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0))
     @ddt.unpack
     def test_get_orphans(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -2015,7 +2015,7 @@ def test_create_item_populates_subtree_edited_info(self, default_ms):
 
     # Draft: wildcard search of draft (find) and split (mysql)
     # Split: wildcard search of draft (find) and split (mysql)
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 0, 2, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 1, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0))
     @ddt.unpack
     def test_get_courses_for_wiki(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -2046,7 +2046,7 @@ def test_get_courses_for_wiki(self, default_ms, num_mysql, max_find, max_send):
     #    Sends:
     #      1. insert structure
     #      2. write index entry
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 6), (ModuleStoreEnum.Type.split, 3, 3, 2))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 6), (ModuleStoreEnum.Type.split, 4, 2, 2))
     @ddt.unpack
     def test_unpublish(self, default_ms, num_mysql, max_find, max_send):
         """
@@ -2084,7 +2084,7 @@ def test_unpublish(self, default_ms, num_mysql, max_find, max_send):
 
     # Draft: specific query for revision None
     # Split: active_versions from MySQL, structure from mongo
-    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 2, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0))
     @ddt.unpack
     def test_has_published_version(self, default_ms, mysql_queries, max_find, max_send):
         """
@@ -3785,7 +3785,7 @@ def test_delete_item_with_asides(self, default_store):
         assert asides2[0].field11 == 'aside1_default_value1'
         assert asides2[0].field12 == 'aside1_default_value2'
 
-    @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 0))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 1, 0))
     @XBlockAside.register_temp_plugin(AsideFoo, 'test_aside1')
     @patch('xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types',
            lambda self, block: ['test_aside1'])