Andal.LND/edx-platform
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: Read course indexes from MySQL, not MongoDB (#29184)

Browse Source 
Description
This is a follow up to #29058 and #29413. This is the next step in moving part of the modulestore data (the course indexes / "active versions" table) from MongoDB to MySQL.

There are four steps planned in moving course index data to MySQL:

Step 1: create the tables in MySQL, start writing to MySQL + MongoDB ✅ done
Step 2: migrate all remaining courses to MySQL ✅ done
Step 3: switch reads from MongoDB to MySQL (this PR)
Step 4 (much later, once we know this is working well): stop writing to MongoDB altogether.
Supporting information
OpenCraft Jira ticket: MNG-2557

Status
✅ Tested with a large Open edX instance is in progress.

Testing instructions
Try making changes in Studio and verify that they work fine.

Deadline
None
This commit is contained in:
Braden MacDonald
2022-03-09 07:21:09 -08:00
committed by GitHub
parent 835285d494
commit dcb7ef8821
34 changed files with 145 additions and 169 deletions
Download Patch File Download Diff File Expand all files Collapse all files

66
common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py
View File

@@ -12,6 +12,7 @@ import zlib
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 @@ class MongoPersistenceBackend:
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 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend):
"""
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 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend):
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 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend):
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 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend):
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 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend):
# 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):
"""

2
common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py
View File

@@ -37,7 +37,7 @@ class TestLibraries(MixedSplitTestCase):
-> 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):

36
common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
View File

@@ -368,7 +368,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
# 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 @@ class TestAsidesWithMixedModuleStore(CommonMixedModuleStoreSetup):
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'])

18
common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py
View File

@@ -154,14 +154,14 @@ class CountMongoCallsCourseTraversal(TestCase):
(MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, True, False, 359),
# The line below shows the way this traversal *should* be done
# (if you'll eventually access all the fields and load all the definitions anyway).
(MIXED_SPLIT_MODULESTORE_BUILDER, None, False, True, 3),
(MIXED_SPLIT_MODULESTORE_BUILDER, None, True, True, 38),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 38),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, True, 38),
(MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 3),
(MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 3),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 3),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 3),
(MIXED_SPLIT_MODULESTORE_BUILDER, None, False, True, 2),
(MIXED_SPLIT_MODULESTORE_BUILDER, None, True, True, 37),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 37),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, True, 37),
(MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 2),
(MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 2),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 2),
(MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 2),
)
@ddt.unpack
def test_number_mongo_calls(self, store_builder, depth, lazy, access_all_block_fields, num_mongo_calls):
@@ -178,7 +178,7 @@ class CountMongoCallsCourseTraversal(TestCase):
@ddt.data(
(MIXED_OLD_MONGO_MODULESTORE_BUILDER, 176),
(MIXED_SPLIT_MODULESTORE_BUILDER, 4),
(MIXED_SPLIT_MODULESTORE_BUILDER, 3),
)
@ddt.unpack
def test_lazy_when_course_previously_cached(self, store_builder, num_mongo_calls):