From 5909102abc6b7037b28ba4beffe32d533c7e60af Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 29 Sep 2021 15:55:52 -0700 Subject: [PATCH 01/13] feat: store split modulestore's course indexes in Django/MySQL Split modulestore persists data in three MongoDB "collections": course_index (list of courses and the current version of each), structure (outline of the courses, and some XBlock fields), and definition (other XBlock fields). While "structure" and "definition" data can get very large, which is one of the reasons MongoDB was chosen for modulestore, the course index data is very small. By moving course index data to MySQL / a django model, we get these advantages: * Full history of changes to the course index data is now preserved * Includes a django admin view to inspect the list of courses and libraries * It's much easier to "reset" a corrupted course to a known working state, by using the simple-history revert tools from the django admin. * The remaining MongoDB collections (structure and definition) are essentially just used as key-value stores of large JSON data structures. This paves the way for future changes that allow migrating courses one at a time from MongoDB to S3, and thus eliminating any use of MongoDB by split modulestore, simplifying the stack. --- .../contentstore/tests/test_clone_course.py | 2 +- .../contentstore/tests/test_course_listing.py | 10 +- .../contentstore/tests/test_orphan.py | 2 +- .../views/tests/test_course_index.py | 12 +- .../contentstore/views/tests/test_item.py | 2 +- cms/envs/common.py | 1 + .../split_modulestore_django/__init__.py | 0 .../split_modulestore_django/admin.py | 18 ++ .../split_modulestore_django/apps.py | 12 ++ .../migrations/0001_initial.py | 65 +++++++ .../migrations/0002_data_migration.py | 41 ++++ .../migrations/__init__.py | 0 .../split_modulestore_django/models.py | 164 ++++++++++++++++ .../split_mongo/mongo_connection.py | 183 +++++++++++++++++- .../xmodule/modulestore/split_mongo/split.py | 4 +- .../xmodule/modulestore/tests/django_utils.py | 26 ++- .../modulestore/tests/test_libraries.py | 22 ++- .../tests/test_mixed_modulestore.py | 153 ++++++++------- .../tests/test_mongo_call_count.py | 18 +- .../xmodule/modulestore/tests/test_publish.py | 1 + .../tests/test_split_modulestore.py | 1 + .../test_split_modulestore_bulk_operations.py | 4 +- .../test_split_mongo_mongo_connection.py | 6 +- .../tests/test_split_w_old_mongo.py | 1 + .../tests/test_course_metadata_utils.py | 1 + .../backends/tests/test_badgr_backend.py | 3 +- lms/djangoapps/badges/tests/test_models.py | 4 +- .../bulk_enroll/tests/test_views.py | 3 +- .../tests/test_field_override_performance.py | 32 +-- lms/djangoapps/ccx/tests/test_models.py | 6 +- .../course_api/blocks/tests/test_api.py | 16 +- .../courseware/tests/test_module_render.py | 12 +- lms/djangoapps/courseware/tests/test_views.py | 2 +- lms/djangoapps/courseware/testutils.py | 6 +- .../django_comment_client/base/tests.py | 4 +- .../discussion/rest_api/tests/test_views.py | 6 +- lms/djangoapps/discussion/tests/test_views.py | 8 +- lms/djangoapps/grades/tests/test_tasks.py | 10 +- .../grades/tests/test_transformer.py | 2 +- lms/djangoapps/instructor/tests/test_tools.py | 4 +- .../tests/views/test_instructor_dashboard.py | 2 +- .../tests/test_tasks_helper.py | 7 +- lms/envs/common.py | 1 + .../djangoapps/bookmarks/tests/test_models.py | 20 +- .../djangoapps/bookmarks/tests/test_tasks.py | 8 +- .../tests/test_course_overviews.py | 2 +- .../course_groups/tests/test_cohorts.py | 2 +- .../courseware_api/tests/test_views.py | 10 +- .../djangoapps/user_api/tests/test_models.py | 3 +- .../tests/views/test_course_home.py | 4 +- .../tests/views/test_course_updates.py | 4 +- .../completion_integration/test_models.py | 8 +- 52 files changed, 731 insertions(+), 207 deletions(-) create mode 100644 common/djangoapps/split_modulestore_django/__init__.py create mode 100644 common/djangoapps/split_modulestore_django/admin.py create mode 100644 common/djangoapps/split_modulestore_django/apps.py create mode 100644 common/djangoapps/split_modulestore_django/migrations/0001_initial.py create mode 100644 common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py create mode 100644 common/djangoapps/split_modulestore_django/migrations/__init__.py create mode 100644 common/djangoapps/split_modulestore_django/models.py diff --git a/cms/djangoapps/contentstore/tests/test_clone_course.py b/cms/djangoapps/contentstore/tests/test_clone_course.py index 34b3ae705358..f6ef12fddba0 100644 --- a/cms/djangoapps/contentstore/tests/test_clone_course.py +++ b/cms/djangoapps/contentstore/tests/test_clone_course.py @@ -125,7 +125,7 @@ def test_rerun_course(self): ) # try to hit the generic exception catch - with patch('xmodule.modulestore.split_mongo.mongo_connection.MongoConnection.insert_course_index', Mock(side_effect=Exception)): # lint-amnesty, pylint: disable=line-too-long + with patch('xmodule.modulestore.split_mongo.mongo_connection.MongoPersistenceBackend.insert_course_index', Mock(side_effect=Exception)): # lint-amnesty, pylint: disable=line-too-long split_course4_id = CourseLocator(org="edx3", course="split3", run="rerun_fail") fields = {'display_name': 'total failure'} CourseRerunState.objects.initiated(split_course3_id, split_course4_id, self.user, fields['display_name']) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index acd2fbfd82fd..dcffbd795497 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -157,8 +157,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): @@ -239,8 +239,8 @@ def test_get_course_list_with_invalid_course_location(self, store): ) @ddt.data( - (ModuleStoreEnum.Type.split, 3, 3), - (ModuleStoreEnum.Type.mongo, 2, 2) + (ModuleStoreEnum.Type.split, 2, 2), + (ModuleStoreEnum.Type.mongo, 1, 1) ) @ddt.unpack def test_course_listing_performance(self, store, courses_list_from_group_calls, courses_list_calls): @@ -289,7 +289,7 @@ def test_course_listing_performance(self, store, courses_list_from_group_calls, # Calls: # 1) query old mongo # 2) get_more on old mongo - # 3) query split (but no courses so no fetching of data) + # 3) query split (handled with MySQL only) @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo) def test_course_listing_errored_deleted_courses(self, store): diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index 48745753f496..1e19bc21fcde 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -103,7 +103,7 @@ def test_get_orphans(self, default_store): self.assertIn(str(location), orphans) @ddt.data( - (ModuleStoreEnum.Type.split, 9, 5), + (ModuleStoreEnum.Type.split, 5, 3), (ModuleStoreEnum.Type.mongo, 34, 12), ) @ddt.unpack diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index b2182b9ec913..dff64735d53a 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -391,13 +391,13 @@ def check_index_page(self, separate_archived_courses, org): @ddt.data( # Staff user has course staff access - (True, 'staff', None, 3, 18), - (False, 'staff', None, 3, 18), + (True, 'staff', None, 1, 20), + (False, 'staff', None, 1, 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): diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 0d93d8b4545d..a5367abc245f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -2528,7 +2528,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 diff --git a/cms/envs/common.py b/cms/envs/common.py index 974e99370a27..576da73dc71f 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1478,6 +1478,7 @@ # For CMS 'cms.djangoapps.contentstore.apps.ContentstoreConfig', + 'common.djangoapps.split_modulestore_django.apps.SplitModulestoreDjangoBackendAppConfig', 'openedx.core.djangoapps.contentserver', 'cms.djangoapps.course_creators', diff --git a/common/djangoapps/split_modulestore_django/__init__.py b/common/djangoapps/split_modulestore_django/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/common/djangoapps/split_modulestore_django/admin.py b/common/djangoapps/split_modulestore_django/admin.py new file mode 100644 index 000000000000..4dbddce9fa08 --- /dev/null +++ b/common/djangoapps/split_modulestore_django/admin.py @@ -0,0 +1,18 @@ +""" +Admin registration for Split Modulestore Django Backend +""" +from django.contrib import admin +from simple_history.admin import SimpleHistoryAdmin + +from .models import SplitModulestoreCourseIndex + + +@admin.register(SplitModulestoreCourseIndex) +class SplitModulestoreCourseIndexAdmin(SimpleHistoryAdmin): + """ + Admin config for course indexes + """ + list_display = ('course_id', 'draft_version', 'published_version', 'library_version', 'wiki_slug', 'last_update') + search_fields = ('course_id', 'wiki_slug') + ordering = ('course_id', ) + readonly_fields = ('id', 'objectid', 'course_id', 'org', ) diff --git a/common/djangoapps/split_modulestore_django/apps.py b/common/djangoapps/split_modulestore_django/apps.py new file mode 100644 index 000000000000..022533938d01 --- /dev/null +++ b/common/djangoapps/split_modulestore_django/apps.py @@ -0,0 +1,12 @@ +""" +Define this module as a Django app +""" +from django.apps import AppConfig + + +class SplitModulestoreDjangoBackendAppConfig(AppConfig): + """ + Django app that provides a backend for Split Modulestore instead of MongoDB. + """ + name = 'common.djangoapps.split_modulestore_django' + verbose_name = "Split Modulestore Django Backend" diff --git a/common/djangoapps/split_modulestore_django/migrations/0001_initial.py b/common/djangoapps/split_modulestore_django/migrations/0001_initial.py new file mode 100644 index 000000000000..598090f8b014 --- /dev/null +++ b/common/djangoapps/split_modulestore_django/migrations/0001_initial.py @@ -0,0 +1,65 @@ +# Generated by Django 2.2.20 on 2021-05-07 18:29 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import opaque_keys.edx.django.models +import simple_history.models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='SplitModulestoreCourseIndex', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('objectid', models.CharField(max_length=24, unique=True)), + ('course_id', opaque_keys.edx.django.models.LearningContextKeyField(db_index=True, max_length=255, unique=True)), + ('org', models.CharField(db_index=True, max_length=255)), + ('draft_version', models.CharField(blank=True, max_length=24)), + ('published_version', models.CharField(blank=True, max_length=24)), + ('library_version', models.CharField(blank=True, max_length=24)), + ('wiki_slug', models.CharField(db_index=True, blank=True, max_length=255)), + ('base_store', models.CharField(choices=[('mongodb', 'MongoDB'), ('django', 'Django - not implemented yet')], max_length=20)), + ('edited_on', models.DateTimeField()), + ('last_update', models.DateTimeField()), + ('edited_by_id', models.IntegerField(null=True)), + ], + options={'ordering': ['course_id'], 'verbose_name_plural': 'Split modulestore course indexes'}, + ), + migrations.CreateModel( + name='HistoricalSplitModulestoreCourseIndex', + fields=[ + ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('objectid', models.CharField(db_index=True, max_length=24)), + ('course_id', opaque_keys.edx.django.models.LearningContextKeyField(db_index=True, max_length=255)), + ('org', models.CharField(db_index=True, max_length=255)), + ('draft_version', models.CharField(blank=True, max_length=24)), + ('published_version', models.CharField(blank=True, max_length=24)), + ('library_version', models.CharField(blank=True, max_length=24)), + ('wiki_slug', models.CharField(db_index=True, blank=True, max_length=255)), + ('base_store', models.CharField(choices=[('mongodb', 'MongoDB'), ('django', 'Django - not implemented yet')], max_length=20)), + ('edited_on', models.DateTimeField()), + ('last_update', models.DateTimeField()), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField()), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('edited_by_id', models.IntegerField(null=True)), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'historical split modulestore course index', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + ] diff --git a/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py b/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py new file mode 100644 index 000000000000..148e1b6b751a --- /dev/null +++ b/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py @@ -0,0 +1,41 @@ +from django.db import migrations, models +from django.db.utils import IntegrityError + + +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore + +from ..models import SplitModulestoreCourseIndex as SplitModulestoreCourseIndex_Real + + +def forwards_func(apps, schema_editor): + """ + Copy all course index data from MongoDB to MySQL. + """ + db_alias = schema_editor.connection.alias + SplitModulestoreCourseIndex = apps.get_model("split_modulestore_django", "SplitModulestoreCourseIndex") + split_modulestore = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.split) + + for course_index in split_modulestore.db_connection.find_matching_course_indexes(force_mongo=True): + data = SplitModulestoreCourseIndex_Real.fields_from_v1_schema(course_index) + + SplitModulestoreCourseIndex(**data).save(using=db_alias) + +def reverse_func(apps, schema_editor): + """ + Reverse the data migration, deleting all entries in this table. + """ + db_alias = schema_editor.connection.alias + SplitModulestoreCourseIndex = apps.get_model("split_modulestore_django", "SplitModulestoreCourseIndex") + SplitModulestoreCourseIndex.objects.using(db_alias).all().delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('split_modulestore_django', '0001_initial'), + ] + + operations = [ + migrations.RunPython(forwards_func, reverse_func), + ] diff --git a/common/djangoapps/split_modulestore_django/migrations/__init__.py b/common/djangoapps/split_modulestore_django/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/common/djangoapps/split_modulestore_django/models.py b/common/djangoapps/split_modulestore_django/models.py new file mode 100644 index 000000000000..b3f8b6d1457e --- /dev/null +++ b/common/djangoapps/split_modulestore_django/models.py @@ -0,0 +1,164 @@ +""" +Django model to store the "course index" data +""" +from bson.objectid import ObjectId +from django.contrib.auth import get_user_model +from django.db import models +from opaque_keys.edx.locator import CourseLocator, LibraryLocator +from opaque_keys.edx.django.models import LearningContextKeyField +from simple_history.models import HistoricalRecords + +from xmodule.modulestore import ModuleStoreEnum + +User = get_user_model() + + +class SplitModulestoreCourseIndex(models.Model): + """ + A "course index" for a course in "split modulestore." + + This model/table mostly stores the current version of each course. + (Well, twice for each course - "draft" and "published" branch versions are + tracked separately.) + + This MySQL table / django model is designed to replace the "active_versions" + MongoDB collection. They contain the same information. + + It also stores the "wiki_slug" to facilitate looking up a course + by it's wiki slug, which is required due to the nuances of the + django-wiki integration. + + .. no_pii: + """ + # For compatibility with MongoDB, each course index must have an ObjectId. We still have an integer primary key too. + objectid = models.CharField(max_length=24, null=False, blank=False, unique=True) + + # The ID of this course (or library). Must start with "course-v1:" or "library-v1:" + course_id = LearningContextKeyField(max_length=255, db_index=True, unique=True, null=False) + # Extract the "org" value from the course_id key so that we can search by org. + # This gets set automatically by clean() + org = models.CharField(max_length=255, db_index=True) + + # Version fields: The ObjectId of the current entry in the "structures" collection, for this course. + # The version is stored separately for each "branch". + # Note that there are only three branch names allowed. Draft/published are used for courses, while "library" is used + # for content libraries. + + # ModuleStoreEnum.BranchName.draft = 'draft-branch' + draft_version = models.CharField(max_length=24, null=False, blank=True) + # ModuleStoreEnum.BranchName.published = 'published-branch' + published_version = models.CharField(max_length=24, null=False, blank=True) + # ModuleStoreEnum.BranchName.library = 'library' + library_version = models.CharField(max_length=24, null=False, blank=True) + + # Wiki slug for this course + wiki_slug = models.CharField(max_length=255, db_index=True, blank=True) + + # Base store - whether the "structures" and "definitions" data are in MongoDB or object storage (S3) + BASE_STORE_MONGO = "mongodb" + BASE_STORE_DJANGO = "django" + BASE_STORE_CHOICES = [ + (BASE_STORE_MONGO, "MongoDB"), # For now, MongoDB is the only implemented option + (BASE_STORE_DJANGO, "Django - not implemented yet"), + ] + base_store = models.CharField(max_length=20, blank=False, choices=BASE_STORE_CHOICES) + + # Edit history: + # ID of the user that made the latest edit. This is not a ForeignKey because some values (like + # ModuleStoreEnum.UserID.*) are not real user IDs. + edited_by_id = models.IntegerField(null=True) + edited_on = models.DateTimeField() + # last_update is different from edited_on, and is used only to prevent collisions? + last_update = models.DateTimeField() + + # Keep track of the history of this table: + history = HistoricalRecords() + + def __str__(self): + return f"Course Index ({self.course_id})" + + class Meta: + ordering = ["course_id"] + verbose_name_plural = "Split modulestore course indexes" + + def as_v1_schema(self): + """ Return in the same format as was stored in MongoDB """ + versions = {} + for branch in ("draft", "published", "library"): + # The current version of this branch, a hex-encoded ObjectID - or an empty string: + version_str = getattr(self, f"{branch}_version") + if version_str: + versions[getattr(ModuleStoreEnum.BranchName, branch)] = ObjectId(version_str) + return { + "_id": ObjectId(self.objectid), + "org": self.course_id.org, + "course": self.course_id.course, + "run": self.course_id.run, # pylint: disable=no-member + "edited_by": self.edited_by_id, + "edited_on": self.edited_on, + "last_update": self.last_update, + "versions": versions, + "schema_version": 1, # This matches schema version 1, see SplitMongoModuleStore.SCHEMA_VERSION + "search_targets": {"wiki_slug": self.wiki_slug}, + } + + @staticmethod + def fields_from_v1_schema(values): + """ Convert the MongoDB-style dict shape to a dict of fields that match this model """ + if values["run"] == LibraryLocator.RUN and ModuleStoreEnum.BranchName.library in values["versions"]: + # This is a content library: + locator = LibraryLocator(org=values["org"], library=values["course"]) + else: + # This is a course: + locator = CourseLocator(org=values["org"], course=values["course"], run=values["run"]) + result = { + "course_id": locator, + "org": values["org"], + "edited_by_id": values["edited_by"], + "edited_on": values["edited_on"], + "base_store": SplitModulestoreCourseIndex.BASE_STORE_MONGO, + } + if "_id" in values: + result["objectid"] = str(values["_id"]) # Convert ObjectId to its hex representation + if "last_update" in values: + result["last_update"] = values["last_update"] + if "search_targets" in values and "wiki_slug" in values["search_targets"]: + result["wiki_slug"] = values["search_targets"]["wiki_slug"] + for branch in ("draft", "published", "library"): + version = values["versions"].get(getattr(ModuleStoreEnum.BranchName, branch)) + if version: + result[f"{branch}_version"] = str(version) # Convert version from ObjectId to hex string + return result + + @staticmethod + def field_name_for_branch(branch_name): + """ Given a full branch name, get the name of the field in this table that stores that branch's version """ + if branch_name == ModuleStoreEnum.BranchName.draft: + return "draft_version" + if branch_name == ModuleStoreEnum.BranchName.published: + return "published_version" + if branch_name == ModuleStoreEnum.BranchName.library: + return "library_version" + raise ValueError(f"Unknown branch name: {branch_name}") + + def clean(self): + """ + Validation for this model + """ + super().clean() + # Check that course_id is a supported type: + course_id_str = str(self.course_id) + if not course_id_str.startswith("course-v1:") and not course_id_str.startswith("library-v1:"): + raise ValueError( + f"Split modulestore cannot store course[like] object with key {course_id_str}" + " - only course-v1/library-v1 prefixed keys are supported." + ) + # Set the "org" field automatically - ensure it always matches the "org" in the course_id + self.org = self.course_id.org + + def save(self, *args, **kwargs): + """ Save this model """ + # Override to ensure that full_clean()/clean() is always called, so that the checks in clean() above are run. + # But don't validate_unique(), it just runs extra queries and the database enforces it anyways. + self.full_clean(validate_unique=False) + return super().save(*args, **kwargs) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index c868ee89f255..e51dc9e0dfb0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -13,11 +13,14 @@ from time import time from django.core.cache import caches, InvalidCacheBackendError +from django.db.transaction import TransactionManagementError import pymongo import pytz from mongodb_proxy import autoretry_read # Import this just to export it from pymongo.errors import DuplicateKeyError # pylint: disable=unused-import + +from common.djangoapps.split_modulestore_django.models import SplitModulestoreCourseIndex from xmodule.exceptions import HeartbeatFailure from xmodule.modulestore import BlockData from xmodule.modulestore.split_mongo import BlockKey @@ -243,7 +246,7 @@ def set(self, key, structure, course_context=None): self.cache.set(key, compressed_pickled_data, None) -class MongoConnection: +class MongoPersistenceBackend: """ Segregation of pymongo functions from the data modeling mechanisms for split modulestore. """ @@ -429,15 +432,18 @@ def _generate_query_from_course_keys(self, branch, course_keys): return courses_queries - def insert_course_index(self, course_index, course_context=None): + def insert_course_index(self, course_index, course_context=None, last_update_already_set=False): """ Create the course_index in the db """ with TIMER.timer("insert_course_index", course_context): - course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Set last_update which is used to avoid collisions, unless a subclass already set it before calling super() + if not last_update_already_set: + course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Insert the new index: self.course_index.insert_one(course_index) - def update_course_index(self, course_index, from_index=None, course_context=None): + def update_course_index(self, course_index, from_index=None, course_context=None, last_update_already_set=False): """ Update the db record for course_index. @@ -457,7 +463,10 @@ def update_course_index(self, course_index, from_index=None, course_context=None 'course': course_index['course'], 'run': course_index['run'], } - course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Set last_update which is used to avoid collisions, unless a subclass already set it before calling super() + if not last_update_already_set: + course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Update the course index: self.course_index.replace_one(query, course_index, upsert=False,) def delete_course_index(self, course_key): @@ -551,3 +560,167 @@ def _drop_database(self, database=True, collections=True, connections=True): if connections: connection.close() + + +class DjangoFlexPersistenceBackend(MongoPersistenceBackend): + """ + Backend for split mongo that can read/write from MySQL and/or S3 instead of Mongo, + either partially replacing MongoDB or fully replacing it. + """ + + # Structures and definitions are only supported in MongoDB for now. + # Course indexes are read from MySQL and written to both MongoDB and MySQL + + def get_course_index(self, key, ignore_case=False): + """ + Get the course_index from the persistence mechanism whose id is the given key + """ + 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. + # e.g. test_split_modulestore.py:SplitModuleCourseTests.test_get_course -> get_course(key with only version) + # > _load_items > cache_items > begin bulk operations > get_course_index > results in this situation. + log.warning("DjangoFlexPersistenceBackend: get_course_index without org/course/run will always return None") + return None + # We never include the branch or the version in the course key in the SplitModulestoreCourseIndex table: + key = key.for_branch(None).version_agnostic() + if not ignore_case: + query = {"course_id": key} + else: + # Case insensitive search is important when creating courses to reject course IDs that differ only by + # capitalization. + query = {"course_id__iexact": key} + try: + return SplitModulestoreCourseIndex.objects.get(**query).as_v1_schema() + except SplitModulestoreCourseIndex.DoesNotExist: + return None + + def find_matching_course_indexes( # pylint: disable=arguments-differ + self, + branch=None, + search_targets=None, + org_target=None, + course_context=None, + course_keys=None, + force_mongo=False, + ): + """ + Find the course_index matching particular conditions. + + Arguments: + branch: If specified, this branch must exist in the returned courses + search_targets: If specified, this must be a dictionary specifying field values + that must exist in the search_targets of the returned courses + org_target: If specified, this is an ORG filter so that only course_indexs are + returned for the specified ORG + """ + if force_mongo: + # For data migration purposes, this argument will read from MongoDB instead of MySQL + return super().find_matching_course_indexes( + branch=branch, search_targets=search_targets, org_target=org_target, + course_context=course_context, course_keys=course_keys, + ) + queryset = SplitModulestoreCourseIndex.objects.all() + if course_keys: + queryset = queryset.filter(course_id__in=course_keys) + if search_targets: + if "wiki_slug" in search_targets: + queryset = queryset.filter(wiki_slug=search_targets.pop("wiki_slug")) + if search_targets: # If there are any search targets besides wiki_slug (which we've handled by this point): + raise ValueError(f"Unsupported search_targets: {', '.join(search_targets.keys())}") + if org_target: + queryset = queryset.filter(org=org_target) + if branch is not None: + branch_field = SplitModulestoreCourseIndex.field_name_for_branch(branch) + queryset = queryset.exclude(**{branch_field: ""}) + + return (course_index.as_v1_schema() for course_index in queryset) + + def insert_course_index(self, course_index, course_context=None): # pylint: disable=arguments-differ + """ + Create the course_index in the db + """ + course_index['last_update'] = datetime.datetime.now(pytz.utc) + new_index = SplitModulestoreCourseIndex(**SplitModulestoreCourseIndex.fields_from_v1_schema(course_index)) + new_index.save() + # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: + super().insert_course_index(course_index, course_context, last_update_already_set=True) + + def update_course_index(self, course_index, from_index=None, course_context=None): # pylint: disable=arguments-differ + """ + Update the db record for course_index. + + Arguments: + from_index: If set, only update an index if it matches the one specified in `from_index`. + + Exceptions: + SplitModulestoreCourseIndex.DoesNotExist: If the given object_id is not valid + """ + # "last_update not only tells us when this course was last updated but also helps prevent collisions" + # This code is just copying the behavior of the existing MongoPersistenceBackend + # See https://github.com/edx/edx-platform/pull/5200 for context + course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Find the SplitModulestoreCourseIndex entry that we'll be updating: + index_obj = SplitModulestoreCourseIndex.objects.get(objectid=course_index["_id"]) + + # Check for collisions: + 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 = [] + for attr, value in SplitModulestoreCourseIndex.fields_from_v1_schema(course_index).items(): + if attr in ("objectid", "course_id"): + # Enforce these attributes as immutable. + if getattr(index_obj, attr) != value: + raise ValueError( + f"Attempted to change the {attr} key of a course index entry ({index_obj.course_id})" + ) + else: + if attr.endswith("_version"): + # Model fields ending in _version are branches. If the branch version has changed, convert the field + # name to a branch name and report it in the history below. + if getattr(index_obj, attr) != value: + changed_branches.append(attr[:-8]) + setattr(index_obj, attr, value) + if changed_branches: + # For the django simple history, indicate what was changed. Unfortunately at this point we only really know + # 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: + super().update_course_index(course_index, from_index, course_context, last_update_already_set=True) + + def delete_course_index(self, course_key): + """ + Delete the course_index from the persistence mechanism whose id is the given course_index + """ + SplitModulestoreCourseIndex.objects.filter(course_id=course_key).delete() + # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: + super().delete_course_index(course_key) + + def _drop_database(self, database=True, collections=True, connections=True): + """ + Reset data for testing. + """ + try: + SplitModulestoreCourseIndex.objects.all().delete() + except TransactionManagementError as err: + # If the test doesn't use 'with self.allow_transaction_exception():', then this error can occur and it may + # be non-obvious why, so give a very clear explanation of how to fix it. See the docstring of + # allow_transaction_exception() for more details. + raise RuntimeError( + "post-test cleanup failed with TransactionManagementError. " + "Use 'with self.allow_transaction_exception():' from ModuleStoreTestCase/...IsolationMixin to fix it." + ) from err + # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: + super()._drop_database(database, collections, connections) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index ac50ace8a734..7878b85d076f 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -101,7 +101,7 @@ VersionConflictError ) from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope -from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, MongoConnection +from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, DjangoFlexPersistenceBackend from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES from xmodule.partitions.partitions_service import PartitionService @@ -651,7 +651,7 @@ def __init__(self, contentstore, doc_store_config, fs_root, render_template, super().__init__(contentstore, **kwargs) - self.db_connection = MongoConnection(**doc_store_config) + self.db_connection = DjangoFlexPersistenceBackend(**doc_store_config) if default_class is not None: module_path, __, class_name = default_class.rpartition('.') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 908ad97baa61..bd94c642d851 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -12,7 +12,7 @@ from django.conf import settings from django.contrib.auth.models import AnonymousUser -from django.db import connections +from django.db import connections, transaction from django.test import TestCase from django.test.utils import override_settings @@ -331,6 +331,30 @@ def end_modulestore_isolation(cls): cls.end_cache_isolation() cls.enable_all_signals() + @staticmethod + def allow_transaction_exception(): + """ + Context manager to wrap modulestore-using test code that may throw an exception. + + (Use this if a modulestore test is failing with TransactionManagementError during cleanup.) + + Details: + Some test cases that purposely throw an exception may normally cause the end_modulestore_isolation() cleanup + step to fail with + TransactionManagementError: + An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block. + This happens because the test is wrapped in an implicit transaction and when the exception occurs, django won't + allow any subsequent database queries in the same transaction - in particular, the queries needed to clean up + split modulestore's SplitModulestoreCourseIndex table after the test. + + By wrapping the inner part of the test in this atomic() call, we create a savepoint so that if an exception is + thrown, Django merely rolls back to the savepoint and the overall transaction continues, including the eventual + cleanup step. + + This method mostly exists to provide this docstring/explanation; the code itself is trivial. + """ + return transaction.atomic() + class ModuleStoreTestUsersMixin(): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py index 6cb17268a740..d607c0e7e903 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py @@ -23,17 +23,21 @@ class TestLibraries(MixedSplitTestCase): def test_create_library(self): """ - Test that we can create a library, and see how many mongo calls it uses to do so. + Test that we can create a library, and see how many database calls it uses to do so. Expected mongo calls, in order: - find_one({'org': '...', 'run': 'library', 'course': '...'}) - insert(definition: {'block_type': 'library', 'fields': {}}) - - insert_structure(bulk) - insert_course_index(bulk) - get_course_index(bulk) - """ - with check_mongo_calls(2, 3): + -> insert(definition: {'block_type': 'library', 'fields': {}}) + -> insert_structure(bulk) + -> insert_course_index(bulk) + + Expected MySQL calls in order: + -> SELECT from SplitModulestoreCourseIndex case insensitive search for existing libraries + -> SELECT from SplitModulestoreCourseIndex lookup library with that exact ID + -> SELECT from XBlockConfiguration (?) + -> INSERT into SplitModulestoreCourseIndex to save the new library + -> INSERT a historical record of the SplitModulestoreCourseIndex + """ + with check_mongo_calls(0, 3), self.assertNumQueries(5): LibraryFactory.create(modulestore=self.store) def test_duplicate_library(self): 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 27b014a3a163..f847630403c7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -366,8 +366,9 @@ def test_duplicate_course_error_with_different_case_ids(self, default_store): # Draft: # 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 - @ddt.data((ModuleStoreEnum.Type.mongo, [1, 1], 0), (ModuleStoreEnum.Type.split, [2, 2], 0)) + # 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, [1, 1], 0)) @ddt.unpack def test_has_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -390,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, [3, 2], 0), (ModuleStoreEnum.Type.split, [2, 2], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, [3, 2], 0), (ModuleStoreEnum.Type.split, 1, [1, 1], 0)) @ddt.unpack - def test_get_item(self, default_ms, max_find, max_send): + 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): + with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql): 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): + with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql): with pytest.raises(ItemNotFoundError): self.store.get_item(self.fake_location) @@ -411,15 +412,16 @@ def test_get_item(self, default_ms, max_find, max_send): # Draft: # wildcard query, 6! load pertinent items for inheritance calls, load parents, course root fetch (why) # Split: - # active_versions (with regex), structure, and spurious active_versions refetch - @ddt.data((ModuleStoreEnum.Type.mongo, 14, 0), (ModuleStoreEnum.Type.split, 4, 0)) + # 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, 3, 1, 0)) @ddt.unpack - def test_get_items(self, default_ms, max_find, max_send): + def test_get_items(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) self._create_block_hierarchy() course_locn = self.course_locations[self.MONGO_COURSEID] - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'}) assert len(modules) == 6 @@ -513,13 +515,16 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp assert (orphan in [item.location for item in items_in_tree]) == orphan_in_items assert len(items_in_tree) == expected_items_in_tree - # draft: get draft, get ancestors up to course (2-6), compute inheritance + # draft: + # find: get draft, get ancestors up to course (2-6), compute 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((ModuleStoreEnum.Type.mongo, 7, 5), (ModuleStoreEnum.Type.split, 3, 2)) + # split: + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # find: definitions (calculator field), structures + # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 5), (ModuleStoreEnum.Type.split, 4, 2, 2)) @ddt.unpack - def test_update_item(self, default_ms, max_find, max_send): + def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ Update should succeed for r/w dbs """ @@ -529,7 +534,7 @@ def test_update_item(self, default_ms, max_find, max_send): # if following raised, then the test is really a noop, change it assert problem.max_attempts != 2, 'Default changed making test meaningless' problem.max_attempts = 2 - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): problem = self.store.update_item(problem, self.user_id) assert problem.max_attempts == 2, "Update didn't persist" @@ -909,11 +914,12 @@ def test_has_changes_missing_child(self, default_ms, default_branch): # get item (to delete subtree), get inheritance again. # Sends: delete item, update parent # Split + # 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, 7, 2), (ModuleStoreEnum.Type.split, 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, max_find, max_send): + def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ Delete should reject on r/o db and work on r/w one """ @@ -922,7 +928,7 @@ def test_delete_item(self, default_ms, max_find, max_send): max_find += 1 with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.writable_chapter_location.course_key): # lint-amnesty, pylint: disable=line-too-long - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): self.store.delete_item(self.writable_chapter_location, self.user_id) # verify it's gone @@ -933,15 +939,16 @@ def test_delete_item(self, default_ms, max_find, max_send): self.store.get_item(self.writable_chapter_location, revision=ModuleStoreEnum.RevisionOption.published_only) # Draft: - # queries: find parent (definition.children), count versions of item, get parent, count grandparents, - # inheritance items, draft item, draft child, inheritance + # find: find parent (definition.children), count versions of item, get parent, count grandparents, + # inheritance items, draft item, draft child, inheritance # sends: delete draft vertical and update parent # Split: - # queries: active_versions, draft and published structures, definition (unnecessary) + # 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, 9, 2), (ModuleStoreEnum.Type.split, 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, max_find, max_send): + def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ Because old mongo treated verticals as the first layer which could be draft, it has some interesting behavioral properties which this deletion test gets at. @@ -972,7 +979,7 @@ def test_delete_private_vertical(self, default_ms, max_find, max_send): assert vert_loc in course.children # delete the vertical and ensure the course no longer points to it - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): self.store.delete_item(vert_loc, self.user_id) course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0) if hasattr(private_vert.location, 'version_guid'): @@ -990,11 +997,12 @@ def test_delete_private_vertical(self, default_ms, max_find, max_send): # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # find: active_version & structure (cached) + # 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, 4, 1), (ModuleStoreEnum.Type.split, 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, max_find, max_send): + def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ Test deleting a draft vertical which has a published version. """ @@ -1024,23 +1032,23 @@ def test_delete_draft_vertical(self, default_ms, max_find, max_send): # test succeeds if delete succeeds w/o error if default_ms == ModuleStoreEnum.Type.mongo and mongo_uses_error_check(self.store): max_find += 1 - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): self.store.delete_item(private_leaf.location, self.user_id) # 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 + # mysql: 1 select on SplitModulestoreCourseIndex since this searches both modulestores + # find: 1 find all courses (wildcard), 1 find to get each course 1 at a time (1 course) # 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((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 6, 0)) + # mysql: 3 selects on SplitModulestoreCourseIndex - 1 to get all courses, 2 to get specific course (this query is + # 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, 1, 2, 0), (ModuleStoreEnum.Type.split, 3, 3, 0)) @ddt.unpack - def test_get_courses(self, default_ms, max_find, max_send): + def test_get_courses(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) # we should have one course across all stores - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): courses = self.store.get_courses() course_ids = [course.location for course in courses] assert len(courses) == 1, f'Not one course: {course_ids}' @@ -1074,16 +1082,16 @@ def test_create_child_detached_tabs(self, default_ms): assert len(mongo_course.children) == 1 # draft is 2: find out which ms owns course, get item - # split: active_versions, structure, definition (to load course wiki string) - @ddt.data((ModuleStoreEnum.Type.mongo, 2, 0), (ModuleStoreEnum.Type.split, 3, 0)) + # split: active_versions (mysql), structure, definition (to load course wiki string) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 1, 2, 0)) @ddt.unpack - def test_get_course(self, default_ms, max_find, max_send): + def test_get_course(self, default_ms, num_mysql, max_find, max_send): """ This test is here for the performance comparison not functionality. It tests the performance of getting an item whose scope.content fields are looked at. """ self.initdb(default_ms) - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): course = self.store.get_item(self.course_locations[self.MONGO_COURSEID]) assert course.id == self.course_locations[self.MONGO_COURSEID].course_key @@ -1112,16 +1120,16 @@ def test_get_library(self, default_ms): # still only 2) # Draft: get_parent # Split: active_versions, structure - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 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, max_find, max_send): + def test_get_parent_locations(self, default_ms, num_mysql, max_find, max_send): """ Test a simple get parent for a direct only category (i.e, always published) """ self.initdb(default_ms) self._create_block_hierarchy() - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): parent = self.store.get_parent_location(self.problem_x1a_1) # lint-amnesty, pylint: disable=no-member assert parent == self.vertical_x1a # lint-amnesty, pylint: disable=no-member @@ -1629,10 +1637,10 @@ def test_get_parent_location_draft(self, default_ms): # 7-8. get sequential, compute inheritance # 8-9. get vertical, compute inheritance # 10-11. get other vertical_x1b (why?) and compute inheritance - # Split: active_versions & structure - @ddt.data((ModuleStoreEnum.Type.mongo, [12, 3], 0), (ModuleStoreEnum.Type.split, [3, 2], 0)) + # 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, 1, [2, 1], 0)) @ddt.unpack - def test_path_to_location(self, default_ms, num_finds, num_sends): + def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends): """ Make sure that path_to_location works """ @@ -1651,7 +1659,7 @@ def test_path_to_location(self, default_ms, 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): + with check_mongo_calls(num_finds.pop(0), num_sends), self.assertNumQueries(num_mysql): path = path_to_location(self.store, location) assert path == expected @@ -1868,10 +1876,10 @@ def _get_split_modulestore(self): assert False, "SplitMongoModuleStore was not found in MixedModuleStore" # Draft: get all items which can be or should have parents - # Split: active_versions, structure - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 0)) + # Split: active_versions (mysql), structure (mongo) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack - def test_get_orphans(self, default_ms, max_find, max_send): + def test_get_orphans(self, default_ms, num_mysql, max_find, max_send): """ Test finding orphans. """ @@ -1903,7 +1911,7 @@ def test_get_orphans(self, default_ms, max_find, max_send): block_id=location.block_id ) - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key) self.assertCountEqual(found_orphans, orphan_locations) @@ -2004,17 +2012,17 @@ def test_create_item_populates_subtree_edited_info(self, default_ms): assert self.user_id == block.subtree_edited_by assert datetime.datetime.now(UTC) > block.subtree_edited_on - # Draft: wildcard search of draft and split - # Split: wildcard search of draft and split - @ddt.data((ModuleStoreEnum.Type.mongo, 2, 0), (ModuleStoreEnum.Type.split, 2, 0)) + # Draft: wildcard search of draft (find) and split (mysql) + # Split: wildcard search of draft (find) and split (mysql) + @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, max_find, max_send): + def test_get_courses_for_wiki(self, default_ms, num_mysql, max_find, max_send): """ Test the get_courses_for_wiki method """ self.initdb(default_ms) # Test Mongo wiki - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): wiki_courses = self.store.get_courses_for_wiki('999') assert len(wiki_courses) == 1 assert self.course_locations[self.MONGO_COURSEID].course_key.replace(branch=None) in wiki_courses @@ -2028,13 +2036,18 @@ def test_get_courses_for_wiki(self, default_ms, max_find, max_send): # 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 - @ddt.data((ModuleStoreEnum.Type.mongo, 2, 6), (ModuleStoreEnum.Type.split, 3, 2)) + # Split: + # MySQL SplitModulestoreCourseIndex: + # 1. Select by course ID + # 2. Select by objectid + # 3-4. Update index version, update historical record + # Find: 2 structures (pre & post published?) + # Sends: + # 1. insert structure + # 2. write index entry + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 6), (ModuleStoreEnum.Type.split, 4, 2, 2)) @ddt.unpack - def test_unpublish(self, default_ms, max_find, max_send): + def test_unpublish(self, default_ms, num_mysql, max_find, max_send): """ Test calling unpublish """ @@ -2052,7 +2065,7 @@ def test_unpublish(self, default_ms, max_find, max_send): assert published_xblock is not None # unpublish - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): self.store.unpublish(self.vertical_x1a, self.user_id) # lint-amnesty, pylint: disable=no-member with pytest.raises(ItemNotFoundError): @@ -2069,10 +2082,10 @@ def test_unpublish(self, default_ms, max_find, max_send): assert draft_xblock is not None # Draft: specific query for revision None - # Split: active_versions, structure - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 0)) + # Split: active_versions from MySQL, structure from mongo + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack - def test_has_published_version(self, default_ms, max_find, max_send): + def test_has_published_version(self, default_ms, mysql_queries, max_find, max_send): """ Test the has_published_version method """ @@ -2082,7 +2095,7 @@ def test_has_published_version(self, default_ms, max_find, max_send): # start off as Private item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state') # lint-amnesty, pylint: disable=line-too-long item_location = item.location - with check_mongo_calls(max_find, max_send): + with self.assertNumQueries(mysql_queries), check_mongo_calls(max_find, max_send): assert not self.store.has_published_version(item) # Private -> Public @@ -3771,7 +3784,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']) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py index 8402d2be882a..ba618da8f329 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py @@ -154,14 +154,14 @@ def _import_course(self, content_store, modulestore): (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 @@ def test_number_mongo_calls(self, store_builder, depth, lazy, access_all_block_f @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): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index efcf5ef68ca6..c2a4b168487e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py @@ -163,6 +163,7 @@ def test_publish_draft_delete(self): assert self.draft_mongo.has_item(other_child_loc), 'Oops, lost moved item' +@pytest.mark.django_db # required if using split modulestore class DraftPublishedOpTestCourseSetup(unittest.TestCase): """ This class exists to test XML import and export between different modulestore diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 1739f48761c0..3550ef69ea3c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -53,6 +53,7 @@ @attr('mongo') +@pytest.mark.django_db class SplitModuleTest(unittest.TestCase): ''' The base set of tests manually populates a db w/ courses which have diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py index da0a6d0d7a87..6690770a86ee 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py @@ -12,7 +12,7 @@ from bson.objectid import ObjectId from opaque_keys.edx.locator import CourseLocator -from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection +from xmodule.modulestore.split_mongo.mongo_connection import MongoPersistenceBackend from xmodule.modulestore.split_mongo.split import SplitBulkWriteMixin VERSION_GUID_DICT = { @@ -30,7 +30,7 @@ def setUp(self): self.bulk = SplitBulkWriteMixin() self.bulk.SCHEMA_VERSION = 1 self.clear_cache = self.bulk._clear_cache = Mock(name='_clear_cache') - self.conn = self.bulk.db_connection = MagicMock(name='db_connection', spec=MongoConnection) + self.conn = self.bulk.db_connection = MagicMock(name='db_connection', spec=MongoPersistenceBackend) self.conn.get_course_index.return_value = {'initial': 'index'} self.course_key = CourseLocator('org', 'course', 'run-a', branch='test') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py index 3460470186b3..008c1cc2a654 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py @@ -1,4 +1,4 @@ -""" Test the behavior of split_mongo/MongoConnection """ +""" Test the behavior of split_mongo/MongoPersistenceBackend """ import unittest @@ -8,7 +8,7 @@ from pymongo.errors import ConnectionFailure from xmodule.exceptions import HeartbeatFailure -from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection +from xmodule.modulestore.split_mongo.mongo_connection import MongoPersistenceBackend class TestHeartbeatFailureException(unittest.TestCase): @@ -20,7 +20,7 @@ def test_heartbeat_raises_exception_when_connection_alive_is_false(self, *calls) # pylint: disable=W0613 with patch('mongodb_proxy.MongoProxy') as mock_proxy: mock_proxy.return_value.admin.command.side_effect = ConnectionFailure('Test') - useless_conn = MongoConnection('useless', 'useless', 'useless') + useless_conn = MongoPersistenceBackend('useless', 'useless', 'useless') with pytest.raises(HeartbeatFailure): useless_conn.heartbeat() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py index 2798bcc9fe90..fa55146808f4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py @@ -19,6 +19,7 @@ @pytest.mark.mongo +@pytest.mark.django_db class SplitWMongoCourseBootstrapper(unittest.TestCase): """ Helper for tests which need to construct split mongo & old mongo based courses to get interesting internal structure. # lint-amnesty, pylint: disable=line-too-long diff --git a/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py b/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py index dc3ad0813f20..f1bd4b6f8df8 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py +++ b/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py @@ -34,6 +34,7 @@ _NEXT_WEEK = _TODAY + timedelta(days=7) +@pytest.mark.django_db class CourseMetadataUtilsTestCase(TestCase): """ Tests for course_metadata_utils. diff --git a/lms/djangoapps/badges/backends/tests/test_badgr_backend.py b/lms/djangoapps/badges/backends/tests/test_badgr_backend.py index 3a500c117bd2..1817fa92a8fe 100644 --- a/lms/djangoapps/badges/backends/tests/test_badgr_backend.py +++ b/lms/djangoapps/badges/backends/tests/test_badgr_backend.py @@ -114,7 +114,8 @@ def test_create_badge(self, post): Verify badge spec creation works. """ self.handler._get_access_token = Mock(return_value='12345') - self.handler._create_badge(self.badge_class) + with self.allow_transaction_exception(): + self.handler._create_badge(self.badge_class) args, kwargs = post.call_args assert args[0] == 'https://example.com/v2/issuers/test-issuer/badgeclasses' assert kwargs['files']['image'][0] == self.badge_class.image.name diff --git a/lms/djangoapps/badges/tests/test_models.py b/lms/djangoapps/badges/tests/test_models.py index 154dd747045f..41a49c6629ac 100644 --- a/lms/djangoapps/badges/tests/test_models.py +++ b/lms/djangoapps/badges/tests/test_models.py @@ -196,8 +196,8 @@ def test_get_badge_class_data_validate(self): Verify handing incomplete data for required fields when making a badge class raises an Integrity error. """ image = get_image('good') - pytest.raises(IntegrityError, BadgeClass.get_badge_class, slug='new_slug', issuing_component='new_component', - image_file_handle=image) + with pytest.raises(IntegrityError), self.allow_transaction_exception(): + BadgeClass.get_badge_class(slug='new_slug', issuing_component='new_component', image_file_handle=image) def test_get_for_user(self): """ diff --git a/lms/djangoapps/bulk_enroll/tests/test_views.py b/lms/djangoapps/bulk_enroll/tests/test_views.py index 1ce5b1775ba1..70e37b15c73e 100644 --- a/lms/djangoapps/bulk_enroll/tests/test_views.py +++ b/lms/djangoapps/bulk_enroll/tests/test_views.py @@ -107,7 +107,8 @@ def test_non_staff(self): """ Test that non global staff users are forbidden from API use. """ self.staff.is_staff = False self.staff.save() - response = self.request_bulk_enroll() + with self.allow_transaction_exception(): + response = self.request_bulk_enroll() assert response.status_code == 403 def test_missing_params(self): diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 8cfca804d33a..0970ab8c990f 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -271,22 +271,22 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 33 + QUERY_COUNT = 34 TEST_DATA = { - ('no_overrides', 1, True, False): (QUERY_COUNT, 3), - ('no_overrides', 2, True, False): (QUERY_COUNT, 3), - ('no_overrides', 3, True, False): (QUERY_COUNT, 3), - ('ccx', 1, True, False): (QUERY_COUNT, 3), - ('ccx', 2, True, False): (QUERY_COUNT, 3), - ('ccx', 3, True, False): (QUERY_COUNT, 3), - ('ccx', 1, True, True): (QUERY_COUNT + 2, 3), - ('ccx', 2, True, True): (QUERY_COUNT + 2, 3), - ('ccx', 3, True, True): (QUERY_COUNT + 2, 3), - ('no_overrides', 1, False, False): (QUERY_COUNT, 3), - ('no_overrides', 2, False, False): (QUERY_COUNT, 3), - ('no_overrides', 3, False, False): (QUERY_COUNT, 3), - ('ccx', 1, False, False): (QUERY_COUNT, 3), - ('ccx', 2, False, False): (QUERY_COUNT, 3), - ('ccx', 3, False, False): (QUERY_COUNT, 3), + ('no_overrides', 1, True, False): (QUERY_COUNT, 2), + ('no_overrides', 2, True, False): (QUERY_COUNT, 2), + ('no_overrides', 3, True, False): (QUERY_COUNT, 2), + ('ccx', 1, True, False): (QUERY_COUNT, 2), + ('ccx', 2, True, False): (QUERY_COUNT, 2), + ('ccx', 3, True, False): (QUERY_COUNT, 2), + ('ccx', 1, True, True): (QUERY_COUNT + 3, 2), + ('ccx', 2, True, True): (QUERY_COUNT + 3, 2), + ('ccx', 3, True, True): (QUERY_COUNT + 3, 2), + ('no_overrides', 1, False, False): (QUERY_COUNT, 2), + ('no_overrides', 2, False, False): (QUERY_COUNT, 2), + ('no_overrides', 3, False, False): (QUERY_COUNT, 2), + ('ccx', 1, False, False): (QUERY_COUNT, 2), + ('ccx', 2, False, False): (QUERY_COUNT, 2), + ('ccx', 3, False, False): (QUERY_COUNT, 2), } diff --git a/lms/djangoapps/ccx/tests/test_models.py b/lms/djangoapps/ccx/tests/test_models.py index 006ba60de568..97c8761b5946 100644 --- a/lms/djangoapps/ccx/tests/test_models.py +++ b/lms/djangoapps/ccx/tests/test_models.py @@ -46,7 +46,7 @@ def test_ccx_course_is_correct_course(self): def test_ccx_course_caching(self): """verify that caching the propery works to limit queries""" - with check_mongo_calls(3): + with check_mongo_calls(2): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. @@ -72,7 +72,7 @@ def test_ccx_start_caching(self): """verify that caching the start property works to limit queries""" now = datetime.now(utc) self.set_ccx_override('start', now) - with check_mongo_calls(3): + with check_mongo_calls(2): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. @@ -97,7 +97,7 @@ def test_ccx_due_caching(self): """verify that caching the due property works to limit queries""" expected = datetime.now(utc) self.set_ccx_override('due', expected) - with check_mongo_calls(3): + with check_mongo_calls(2): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index e7b3fbdd9a98..5297f7f9f2aa 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -224,23 +224,17 @@ def test_query_counts_cached(self, store_type, with_storage_backing): ) @ddt.data( - *product( - ((ModuleStoreEnum.Type.mongo, 5), (ModuleStoreEnum.Type.split, 3)), - (True, False), - ) + (ModuleStoreEnum.Type.mongo, 5, True, 23), + (ModuleStoreEnum.Type.mongo, 5, False, 13), + (ModuleStoreEnum.Type.split, 2, True, 24), + (ModuleStoreEnum.Type.split, 2, False, 14), ) @ddt.unpack - def test_query_counts_uncached(self, store_type_tuple, with_storage_backing): - store_type, expected_mongo_queries = store_type_tuple + def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries): with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): course = self._create_course(store_type) clear_course_from_cache(course.id) - if with_storage_backing: - num_sql_queries = 23 - else: - num_sql_queries = 13 - self._get_blocks( course, expected_mongo_queries, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 4cc29a5e117f..e6acde7d225d 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1014,9 +1014,9 @@ def setup_request_and_course(self, num_finds, num_sends): # Split makes 2 queries to load the course to depth 2: # - 1 for the structure # - 1 for 5 definitions - # Split makes 1 query to render the toc: - # - 1 for the active version at the start of the bulk operation - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) + # Split makes 1 MySQL query to render the toc: + # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) @ddt.unpack def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): @@ -1054,9 +1054,9 @@ def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_fi # Split makes 2 queries to load the course to depth 2: # - 1 for the structure # - 1 for 5 definitions - # Split makes 1 query to render the toc: - # - 1 for the active version at the start of the bulk operation - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) + # Split makes 1 MySQL query to render the toc: + # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) @ddt.unpack def test_toc_toy_from_section(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index ddafdde6563e..350bf95eda7c 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -389,7 +389,7 @@ class IndexQueryTestCase(ModuleStoreTestCase): @ddt.data( (ModuleStoreEnum.Type.mongo, 10, 164), - (ModuleStoreEnum.Type.split, 4, 160), + (ModuleStoreEnum.Type.split, 3, 161), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index 6c4caa6be16e..e3fb0132ed82 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -162,9 +162,9 @@ def verify_response(self, expected_response_code=200, url_params=None): @ddt.data( ('vertical_block', ModuleStoreEnum.Type.mongo, 13), - ('vertical_block', ModuleStoreEnum.Type.split, 6), + ('vertical_block', ModuleStoreEnum.Type.split, 4), ('html_block', ModuleStoreEnum.Type.mongo, 14), - ('html_block', ModuleStoreEnum.Type.split, 6), + ('html_block', ModuleStoreEnum.Type.split, 4), ) @ddt.unpack def test_courseware_html(self, block_name, default_store, mongo_calls): @@ -192,7 +192,7 @@ def test_courseware_html(self, block_name, default_store, mongo_calls): @ddt.data( (ModuleStoreEnum.Type.mongo, 5), - (ModuleStoreEnum.Type.split, 5), + (ModuleStoreEnum.Type.split, 4), ) @ddt.unpack def test_success_enrolled_staff(self, default_store, mongo_calls): diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index efb018980908..efc96a325dc9 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -402,7 +402,7 @@ def inner(self, default_store, module_count, mongo_calls, sql_queries, *args, ** @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 4, 39), - (ModuleStoreEnum.Type.split, 3, 13, 39), + (ModuleStoreEnum.Type.split, 3, 8, 44), ) @ddt.unpack @count_queries @@ -411,7 +411,7 @@ def test_create_thread(self, mock_request): @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 3, 35), - (ModuleStoreEnum.Type.split, 3, 10, 35), + (ModuleStoreEnum.Type.split, 3, 6, 39), ) @ddt.unpack @count_queries diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 61103af01c01..ffcffbef6cc5 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -413,10 +413,10 @@ def test_basic(self): (2, ModuleStoreEnum.Type.mongo, 2, {"Test Topic 1": {"id": "test_topic_1"}}), (2, ModuleStoreEnum.Type.mongo, 2, {"Test Topic 1": {"id": "test_topic_1"}, "Test Topic 2": {"id": "test_topic_2"}}), - (2, ModuleStoreEnum.Type.split, 3, {"Test Topic 1": {"id": "test_topic_1"}}), - (2, ModuleStoreEnum.Type.split, 3, + (2, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}}), + (2, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}, "Test Topic 2": {"id": "test_topic_2"}}), - (10, ModuleStoreEnum.Type.split, 3, {"Test Topic 1": {"id": "test_topic_1"}}), + (10, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}}), ) @ddt.unpack def test_bulk_response(self, modules_count, module_store, mongo_calls, topics): diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 0cf438e2319a..fb1da7cb6822 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -483,15 +483,15 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 21, 8), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, False, 1, 2, 2, 22, 8), + (ModuleStoreEnum.Type.split, False, 50, 2, 2, 22, 8), # Enabling Enterprise integration should have no effect on the number of mongo queries made. (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 21, 8), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, True, 1, 2, 2, 22, 8), + (ModuleStoreEnum.Type.split, True, 50, 2, 2, 22, 8), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index c68b17f37bc5..cfee479e46ae 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -164,8 +164,8 @@ def test_block_structure_created_only_once(self): @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39, True), (ModuleStoreEnum.Type.mongo, 1, 39, False), - (ModuleStoreEnum.Type.split, 3, 39, True), - (ModuleStoreEnum.Type.split, 3, 39, False), + (ModuleStoreEnum.Type.split, 2, 40, True), + (ModuleStoreEnum.Type.split, 2, 40, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -178,7 +178,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39), - (ModuleStoreEnum.Type.split, 3, 39), + (ModuleStoreEnum.Type.split, 2, 40), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -224,7 +224,7 @@ def test_other_inaccessible_subsection(self, mock_subsection_signal): @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 22), - (ModuleStoreEnum.Type.split, 3, 22), + (ModuleStoreEnum.Type.split, 2, 23), ) @ddt.unpack def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): @@ -239,7 +239,7 @@ def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_ @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 40), - (ModuleStoreEnum.Type.split, 3, 40), + (ModuleStoreEnum.Type.split, 2, 41), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/grades/tests/test_transformer.py b/lms/djangoapps/grades/tests/test_transformer.py index f2bf8741b8ac..98685ea2d534 100644 --- a/lms/djangoapps/grades/tests/test_transformer.py +++ b/lms/djangoapps/grades/tests/test_transformer.py @@ -430,7 +430,7 @@ def setUp(self): self.client.login(username=self.student.username, password=password) @ddt.data( - (ModuleStoreEnum.Type.split, 3), + (ModuleStoreEnum.Type.split, 2), (ModuleStoreEnum.Type.mongo, 2), ) @ddt.unpack diff --git a/lms/djangoapps/instructor/tests/test_tools.py b/lms/djangoapps/instructor/tests/test_tools.py index 0713bd1f5616..0840f7426570 100644 --- a/lms/djangoapps/instructor/tests/test_tools.py +++ b/lms/djangoapps/instructor/tests/test_tools.py @@ -252,12 +252,12 @@ def test_set_due_date_extension(self): def test_set_due_date_extension_invalid_date(self): extended = datetime.datetime(2009, 1, 1, 0, 0, tzinfo=UTC) - with pytest.raises(tools.DashboardError): + with pytest.raises(tools.DashboardError), self.allow_transaction_exception(): tools.set_due_date_extension(self.course, self.week1, self.user, extended) def test_set_due_date_extension_no_date(self): extended = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=UTC) - with pytest.raises(tools.DashboardError): + with pytest.raises(tools.DashboardError), self.allow_transaction_exception(): tools.set_due_date_extension(self.course, self.week3, self.user, extended) def test_reset_due_date_extension(self): diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 90a35f1d6b0b..6be624e9fd9c 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -673,6 +673,6 @@ def test_spoc_gradebook_mongo_calls(self): # check MongoDB calls count url = reverse('spoc_gradebook', kwargs={'course_id': self.course.id}) - with check_mongo_calls(9): + with check_mongo_calls(6): response = self.client.get(url) assert response.status_code == 200 diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 7420dfa9d5fa..c3dd3eb9afb2 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -371,11 +371,11 @@ def test_certificate_eligibility(self): self._verify_cell_data_for_user(verified_user.username, course.id, 'Certificate Eligible', 'Y', num_rows=2) @ddt.data( - (ModuleStoreEnum.Type.mongo, 4), - (ModuleStoreEnum.Type.split, 3), + (ModuleStoreEnum.Type.mongo, 4, 46), + (ModuleStoreEnum.Type.split, 2, 47), ) @ddt.unpack - def test_query_counts(self, store_type, mongo_count): + def test_query_counts(self, store_type, mongo_count, expected_query_count): with self.store.default_store(store_type): experiment_group_a = Group(2, 'Expériment Group A') experiment_group_b = Group(3, 'Expériment Group B') @@ -401,7 +401,6 @@ def test_query_counts(self, store_type, mongo_count): RequestCache.clear_all_namespaces() - expected_query_count = 46 with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): with check_mongo_calls(mongo_count): with self.assertNumQueries(expected_query_count): diff --git a/lms/envs/common.py b/lms/envs/common.py index 22580a0d592c..35a4fce2a31c 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2938,6 +2938,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'lms.djangoapps.courseware', 'lms.djangoapps.coursewarehistoryextended', 'common.djangoapps.student.apps.StudentConfig', + 'common.djangoapps.split_modulestore_django.apps.SplitModulestoreDjangoBackendAppConfig', 'lms.djangoapps.static_template_view', 'lms.djangoapps.staticbook', diff --git a/openedx/core/djangoapps/bookmarks/tests/test_models.py b/openedx/core/djangoapps/bookmarks/tests/test_models.py index 46e5b8ed7095..df5d89c5f5fc 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_models.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_models.py @@ -276,11 +276,11 @@ def get_bookmark_data(self, block, user=None): (ModuleStoreEnum.Type.mongo, 'sequential_1', ['chapter_1'], 4), (ModuleStoreEnum.Type.mongo, 'vertical_1', ['chapter_1', 'sequential_1'], 6), (ModuleStoreEnum.Type.mongo, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 7), - (ModuleStoreEnum.Type.split, 'course', [], 3), - (ModuleStoreEnum.Type.split, 'chapter_1', [], 2), - (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 2), - (ModuleStoreEnum.Type.split, 'vertical_1', ['chapter_1', 'sequential_1'], 2), - (ModuleStoreEnum.Type.split, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 2), + (ModuleStoreEnum.Type.split, 'course', [], 2), + (ModuleStoreEnum.Type.split, 'chapter_1', [], 1), + (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 1), + (ModuleStoreEnum.Type.split, 'vertical_1', ['chapter_1', 'sequential_1'], 1), + (ModuleStoreEnum.Type.split, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 1), ) @ddt.unpack def test_path_and_queries_on_create(self, store_type, block_to_bookmark, ancestors_attrs, expected_mongo_calls): @@ -376,11 +376,11 @@ def test_path(self, seconds_delta, paths, get_path_call_count, mock_get_path): # (ModuleStoreEnum.Type.mongo, 6, 3, 3), Too slow. (ModuleStoreEnum.Type.mongo, 2, 4, 4), # (ModuleStoreEnum.Type.mongo, 4, 4, 4), - (ModuleStoreEnum.Type.split, 2, 2, 2), - (ModuleStoreEnum.Type.split, 4, 2, 2), - (ModuleStoreEnum.Type.split, 2, 3, 2), - # (ModuleStoreEnum.Type.split, 4, 3, 2), - (ModuleStoreEnum.Type.split, 2, 4, 2), + (ModuleStoreEnum.Type.split, 2, 2, 1), + (ModuleStoreEnum.Type.split, 4, 2, 1), + (ModuleStoreEnum.Type.split, 2, 3, 1), + # (ModuleStoreEnum.Type.split, 4, 3, 1), + (ModuleStoreEnum.Type.split, 2, 4, 1), ) @ddt.unpack def test_get_path_queries(self, store_type, children_per_block, depth, expected_mongo_calls): diff --git a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py index f033e6bc1e49..615d1b5d8a4e 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py @@ -108,10 +108,10 @@ def setUp(self): (ModuleStoreEnum.Type.mongo, 4, 3, 5), (ModuleStoreEnum.Type.mongo, 2, 4, 6), # (ModuleStoreEnum.Type.mongo, 4, 4, 6), Too slow. - (ModuleStoreEnum.Type.split, 2, 2, 3), - (ModuleStoreEnum.Type.split, 4, 2, 3), - (ModuleStoreEnum.Type.split, 2, 3, 3), - (ModuleStoreEnum.Type.split, 2, 4, 3), + (ModuleStoreEnum.Type.split, 2, 2, 2), + (ModuleStoreEnum.Type.split, 4, 2, 2), + (ModuleStoreEnum.Type.split, 2, 3, 2), + (ModuleStoreEnum.Type.split, 2, 4, 2), ) @ddt.unpack def test_calculate_course_xblocks_data_queries(self, store_type, children_per_block, depth, expected_mongo_calls): diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 425f2c89a154..39b30bd4174f 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -378,7 +378,7 @@ def test_malformed_grading_policy(self): course_overview = CourseOverview._create_or_update(course) # pylint: disable=protected-access assert course_overview.lowest_passing_grade is None - @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 2, 2)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index f3760cb9879b..8b082bef0b81 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -743,7 +743,7 @@ def test_multiple_partition_groups(self): self.partition_id, self.group1_id, ) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError), self.allow_transaction_exception(): self._link_cohort_partition_group( self.first_cohort, self.partition_id, diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index 2c66336bce5a..4e00ac0b23db 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -396,7 +396,15 @@ def test_sequence_metadata(self): def test_hidden_after_due(self, is_past_due, masquerade_config, expected_hidden, expected_banner): """Validate the metadata when hide-after-due is set for a sequence""" due = datetime.now() + timedelta(days=-1 if is_past_due else 1) - sequence = ItemFactory(parent=self.chapter, category='sequential', hide_after_due=True, due=due) + sequence = ItemFactory( + parent_location=self.chapter.location, + # ^ It is very important that we use parent_location=self.chapter.location (and not parent=self.chapter), as + # chapter is a class attribute and passing it by value will update its .children=[] which will then leak + # into other tests and cause errors if the children no longer exist. + category='sequential', + hide_after_due=True, + due=due, + ) CourseEnrollment.enroll(self.user, self.course.id) diff --git a/openedx/core/djangoapps/user_api/tests/test_models.py b/openedx/core/djangoapps/user_api/tests/test_models.py index 6a5d64bd2456..5634aa00b9ec 100644 --- a/openedx/core/djangoapps/user_api/tests/test_models.py +++ b/openedx/core/djangoapps/user_api/tests/test_models.py @@ -23,7 +23,8 @@ class UserPreferenceModelTest(ModuleStoreTestCase): def test_duplicate_user_key(self): user = UserFactory.create() UserPreferenceFactory.create(user=user, key="testkey", value="first") - pytest.raises(IntegrityError, UserPreferenceFactory.create) + with self.allow_transaction_exception(): + pytest.raises(IntegrityError, UserPreferenceFactory.create) def test_arbitrary_values(self): user = UserFactory.create() diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 36dd785966e2..e8499de9fb76 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -205,8 +205,8 @@ def test_queries(self): # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(72, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): - with check_mongo_calls(4): + with self.assertNumQueries(73, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with check_mongo_calls(3): url = course_home_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index a3abea4b3db8..a246486e2db8 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -49,7 +49,7 @@ def test_queries(self): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(50, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): - with check_mongo_calls(4): + with self.assertNumQueries(51, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with check_mongo_calls(3): url = course_updates_url(self.course) self.client.get(url) diff --git a/openedx/tests/completion_integration/test_models.py b/openedx/tests/completion_integration/test_models.py index 8871272b3cf0..74c61de25461 100644 --- a/openedx/tests/completion_integration/test_models.py +++ b/openedx/tests/completion_integration/test_models.py @@ -62,8 +62,8 @@ def setUp(self): self.set_up_completion() def test_changed_value(self): - with self.assertNumQueries(SELECT + UPDATE + 2 * SAVEPOINT + 2 * OTHER): - # OTHER = user exists, completion exists + with self.assertNumQueries(SELECT + UPDATE + 2 * SAVEPOINT + 4 * OTHER): + # OTHER = user exists, completion exists, 2x look up course in splitmodulestorecourseindex completion, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, block_key=self.block_key, @@ -88,7 +88,7 @@ def test_unchanged_value(self): def test_new_user(self): newuser = UserFactory() - with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT): + with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 2 * OTHER): _, isnew = models.BlockCompletion.objects.submit_completion( user=newuser, block_key=self.block_key, @@ -99,7 +99,7 @@ def test_new_user(self): def test_new_block(self): newblock = UsageKey.from_string('block-v1:edx+test+run+type@video+block@puppers') - with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT): + with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 2 * OTHER): _, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, block_key=newblock, From ab74f1824bbf09e1f3a2538b7b42d177f94fc1a4 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 18 Oct 2021 16:54:40 -0700 Subject: [PATCH 02/13] fix: Make course_id case-sensitive --- .../migrations/0001_initial.py | 12 ++++- .../tests/__init__.py | 0 .../tests/test_models.py | 45 +++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 common/djangoapps/split_modulestore_django/tests/__init__.py create mode 100644 common/djangoapps/split_modulestore_django/tests/test_models.py diff --git a/common/djangoapps/split_modulestore_django/migrations/0001_initial.py b/common/djangoapps/split_modulestore_django/migrations/0001_initial.py index 598090f8b014..25b752da68d8 100644 --- a/common/djangoapps/split_modulestore_django/migrations/0001_initial.py +++ b/common/djangoapps/split_modulestore_django/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 2.2.20 on 2021-05-07 18:29 +# Generated by Django 2.2.20 on 2021-05-07 18:29, manually modified to make "course_id" column case sensitive from django.conf import settings from django.db import migrations, models @@ -62,4 +62,14 @@ class Migration(migrations.Migration): }, bases=(simple_history.models.HistoricalChanges, models.Model), ), + # Custom code: Convert columns to utf8_bin because we want to allow + # case-sensitive comparisons for CourseKeys, which were case-sensitive in MongoDB + migrations.RunSQL( + 'ALTER TABLE split_modulestore_django_splitmodulestorecourseindex MODIFY course_id varchar(255) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL UNIQUE;', + reverse_sql=migrations.RunSQL.noop, + ), + migrations.RunSQL( + 'ALTER TABLE split_modulestore_django_historicalsplitmodulestorecourseindex MODIFY course_id varchar(255) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL;', + reverse_sql=migrations.RunSQL.noop, + ), ] diff --git a/common/djangoapps/split_modulestore_django/tests/__init__.py b/common/djangoapps/split_modulestore_django/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/common/djangoapps/split_modulestore_django/tests/test_models.py b/common/djangoapps/split_modulestore_django/tests/test_models.py new file mode 100644 index 000000000000..2205f7ab5532 --- /dev/null +++ b/common/djangoapps/split_modulestore_django/tests/test_models.py @@ -0,0 +1,45 @@ +""" Unit tests for SplitModulestoreCourseIndex """ +from datetime import datetime + +from bson.objectid import ObjectId +from django.test import TestCase +from opaque_keys.edx.keys import CourseKey + +from common.djangoapps.split_modulestore_django.models import SplitModulestoreCourseIndex +from xmodule.modulestore import ModuleStoreEnum + + +class SplitModulestoreCourseIndexTest(TestCase): + """ Unit tests for SplitModulestoreCourseIndex """ + + def test_course_id_case_sensitive(self): + """ + Make sure the course_id column is case sensitive. + + Although the platform code generally tries to prevent having two courses whose IDs differ only by case + (e.g. https://git.io/J6voR , note `ignore_case=True`), we found at least one pair of courses on stage that + differs only by case in its `org` ID (`edx` vs `edX`). So for backwards compatibility with MongoDB and to avoid + issues for anyone else with similar course IDs that differ only by case, we've made the new version case + sensitive too. The system still tries to prevent creation of courses that differ only by course (that hasn't + changed), but now the MySQL version won't break if that has somehow happened. + """ + course_index_common = { + "course": "TL101", + "run": "2015", + "edited_by": ModuleStoreEnum.UserID.mgmt_command, + "edited_on": datetime.now(), + "last_update": datetime.now(), + "versions": {}, + "schema_version": 1, + "search_targets": {"wiki_slug": "TLslug"}, + } + course_index_1 = {**course_index_common, "_id": ObjectId("553115a9d15a010b5c6f7228"), "org": "edx"} + course_index_2 = {**course_index_common, "_id": ObjectId("550869e42d00970b5b082d2a"), "org": "edX"} + data1 = SplitModulestoreCourseIndex.fields_from_v1_schema(course_index_1) + data2 = SplitModulestoreCourseIndex.fields_from_v1_schema(course_index_2) + SplitModulestoreCourseIndex(**data1).save() + # This next line will fail if the course_id column is not case-sensitive: + SplitModulestoreCourseIndex(**data2).save() + # Also check deletion, to ensure the course_id historical record is not unique or case sensitive: + SplitModulestoreCourseIndex.objects.get(course_id=CourseKey.from_string("course-v1:edx+TL101+2015")).delete() + SplitModulestoreCourseIndex.objects.get(course_id=CourseKey.from_string("course-v1:edX+TL101+2015")).delete() From 134959cc71cfe5f2cf4b871aa022dfbf6478ed8b Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 18 Oct 2021 16:58:17 -0700 Subject: [PATCH 03/13] refactor: Remove data migration for now, read from Mongo, write to MySQL+Mongo --- .../migrations/0002_data_migration.py | 41 ------------------- .../split_mongo/mongo_connection.py | 8 ++++ 2 files changed, 8 insertions(+), 41 deletions(-) delete mode 100644 common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py diff --git a/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py b/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py deleted file mode 100644 index 148e1b6b751a..000000000000 --- a/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py +++ /dev/null @@ -1,41 +0,0 @@ -from django.db import migrations, models -from django.db.utils import IntegrityError - - -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore - -from ..models import SplitModulestoreCourseIndex as SplitModulestoreCourseIndex_Real - - -def forwards_func(apps, schema_editor): - """ - Copy all course index data from MongoDB to MySQL. - """ - db_alias = schema_editor.connection.alias - SplitModulestoreCourseIndex = apps.get_model("split_modulestore_django", "SplitModulestoreCourseIndex") - split_modulestore = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.split) - - for course_index in split_modulestore.db_connection.find_matching_course_indexes(force_mongo=True): - data = SplitModulestoreCourseIndex_Real.fields_from_v1_schema(course_index) - - SplitModulestoreCourseIndex(**data).save(using=db_alias) - -def reverse_func(apps, schema_editor): - """ - Reverse the data migration, deleting all entries in this table. - """ - db_alias = schema_editor.connection.alias - SplitModulestoreCourseIndex = apps.get_model("split_modulestore_django", "SplitModulestoreCourseIndex") - SplitModulestoreCourseIndex.objects.using(db_alias).all().delete() - - -class Migration(migrations.Migration): - - dependencies = [ - ('split_modulestore_django', '0001_initial'), - ] - - operations = [ - migrations.RunPython(forwards_func, reverse_func), - ] diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index e51dc9e0dfb0..867e877f7083 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -575,6 +575,10 @@ 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: # 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 @@ -615,6 +619,10 @@ 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( From 87e51f14a5a639b6c93d353712d19c7e7c456bb6 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 19 Oct 2021 11:17:07 -0700 Subject: [PATCH 04/13] fix: Some false positive collisions were causing bugs without the data migration --- .../split_mongo/mongo_connection.py | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 867e877f7083..5c651ef6a5f2 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -467,7 +467,8 @@ 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: - self.course_index.replace_one(query, course_index, upsert=False,) + result = self.course_index.replace_one(query, course_index, upsert=False,) + return result.modified_count == 1 def delete_course_index(self, course_key): """ @@ -670,17 +671,31 @@ def update_course_index(self, course_index, from_index=None, course_context=None # See https://github.com/edx/edx-platform/pull/5200 for context course_index['last_update'] = datetime.datetime.now(pytz.utc) # Find the SplitModulestoreCourseIndex entry that we'll be updating: - index_obj = SplitModulestoreCourseIndex.objects.get(objectid=course_index["_id"]) + 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 + ####################### # Check for collisions: - 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 + # 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 # Apply updates to the index entry. While doing so, track which branch versions were changed (if any). changed_branches = [] @@ -703,10 +718,11 @@ 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: - super().update_course_index(course_index, from_index, course_context, last_update_already_set=True) + 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() def delete_course_index(self, course_key): """ From ae3328f8f017581ad89d5f673363142429a9a000 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 22 Oct 2021 12:13:35 -0700 Subject: [PATCH 05/13] fix: Temporarily update query counts to match migration path refactor see "Remove data migration for now, read from Mongo, write to MySQL+Mongo" --- .../contentstore/tests/test_course_listing.py | 8 ++--- .../views/tests/test_course_index.py | 12 +++---- .../contentstore/views/tests/test_item.py | 2 +- .../modulestore/tests/test_libraries.py | 2 +- .../tests/test_mixed_modulestore.py | 32 +++++++++---------- .../tests/test_mongo_call_count.py | 18 +++++------ .../tests/test_field_override_performance.py | 32 +++++++++---------- lms/djangoapps/ccx/tests/test_models.py | 6 ++-- .../course_api/blocks/tests/test_api.py | 4 +-- .../courseware/tests/test_module_render.py | 4 +-- lms/djangoapps/courseware/tests/test_views.py | 2 +- lms/djangoapps/courseware/testutils.py | 6 ++-- .../django_comment_client/base/tests.py | 4 +-- .../discussion/rest_api/tests/test_views.py | 6 ++-- lms/djangoapps/discussion/tests/test_views.py | 8 ++--- lms/djangoapps/grades/tests/test_tasks.py | 10 +++--- .../grades/tests/test_transformer.py | 2 +- .../tests/views/test_instructor_dashboard.py | 2 +- .../tests/test_tasks_helper.py | 2 +- .../djangoapps/bookmarks/tests/test_models.py | 18 +++++------ .../djangoapps/bookmarks/tests/test_tasks.py | 8 ++--- .../tests/test_course_overviews.py | 2 +- .../tests/views/test_course_home.py | 4 +-- .../tests/views/test_course_updates.py | 4 +-- .../completion_integration/test_models.py | 6 ++-- 25 files changed, 102 insertions(+), 102 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index dcffbd795497..ef4c8d46691c 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -157,8 +157,8 @@ def test_courses_list_with_ccx_courses(self): self.assertEqual(len(list(courses_iter)), 0) @ddt.data( - (ModuleStoreEnum.Type.split, 2), - (ModuleStoreEnum.Type.mongo, 1) + (ModuleStoreEnum.Type.split, 3), + (ModuleStoreEnum.Type.mongo, 2) ) @ddt.unpack def test_staff_course_listing(self, default_store, mongo_calls): @@ -239,8 +239,8 @@ def test_get_course_list_with_invalid_course_location(self, store): ) @ddt.data( - (ModuleStoreEnum.Type.split, 2, 2), - (ModuleStoreEnum.Type.mongo, 1, 1) + (ModuleStoreEnum.Type.split, 3, 3), + (ModuleStoreEnum.Type.mongo, 2, 2) ) @ddt.unpack def test_course_listing_performance(self, store, courses_list_from_group_calls, courses_list_calls): diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index dff64735d53a..b2182b9ec913 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -391,13 +391,13 @@ def check_index_page(self, separate_archived_courses, org): @ddt.data( # Staff user has course staff access - (True, 'staff', None, 1, 20), - (False, 'staff', None, 1, 20), + (True, 'staff', None, 3, 18), + (False, 'staff', None, 3, 18), # Base user has global staff access - (True, 'user', ORG, 1, 20), - (False, 'user', ORG, 1, 20), - (True, 'user', None, 1, 20), - (False, 'user', None, 1, 20), + (True, 'user', ORG, 3, 18), + (False, 'user', ORG, 3, 18), + (True, 'user', None, 3, 18), + (False, 'user', None, 3, 18), ) @ddt.unpack def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index a5367abc245f..0d93d8b4545d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -2528,7 +2528,7 @@ def test_json_responses(self): self.validate_course_xblock_info(json_response, course_outline=True) @ddt.data( - (ModuleStoreEnum.Type.split, 3, 3), + (ModuleStoreEnum.Type.split, 4, 4), (ModuleStoreEnum.Type.mongo, 5, 7), ) @ddt.unpack diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py index d607c0e7e903..8e9818067ab4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py +++ b/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(0, 3), self.assertNumQueries(5): + with check_mongo_calls(2, 3), self.assertNumQueries(3): LibraryFactory.create(modulestore=self.store) def test_duplicate_library(self): 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 f847630403c7..e0557067dc82 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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, [1, 1], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, [1, 1], 0), (ModuleStoreEnum.Type.split, [2, 2], 0)) @ddt.unpack def test_has_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -391,7 +391,7 @@ 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, 1, [1, 1], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, [3, 2], 0), (ModuleStoreEnum.Type.split, 0, [2, 2], 0)) @ddt.unpack def test_get_item(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -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, 3, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 14, 0), (ModuleStoreEnum.Type.split, 0, 4, 0)) @ddt.unpack def test_get_items(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -522,7 +522,7 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 5), (ModuleStoreEnum.Type.split, 4, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 5), (ModuleStoreEnum.Type.split, 3, 3, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -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, 4, 2, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 2), (ModuleStoreEnum.Type.split, 3, 3, 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, 4, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 9, 2), (ModuleStoreEnum.Type.split, 3, 4, 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, 4, 1, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 4, 1), (ModuleStoreEnum.Type.split, 3, 2, 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, 1, 2, 0), (ModuleStoreEnum.Type.split, 3, 3, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 0), (ModuleStoreEnum.Type.split, 0, 6, 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, 1, 2, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 0, 3, 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, 1, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 2, 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, 1, [2, 1], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, [12, 3], 0), (ModuleStoreEnum.Type.split, 0, [3, 2], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends): """ @@ -1877,7 +1877,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, 1, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 2, 0)) @ddt.unpack def test_get_orphans(self, default_ms, num_mysql, max_find, max_send): """ @@ -2014,7 +2014,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, 1, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 0, 2, 0)) @ddt.unpack def test_get_courses_for_wiki(self, default_ms, num_mysql, max_find, max_send): """ @@ -2045,7 +2045,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, 4, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 6), (ModuleStoreEnum.Type.split, 3, 3, 2)) @ddt.unpack def test_unpublish(self, default_ms, num_mysql, max_find, max_send): """ @@ -2083,7 +2083,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, 1, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 2, 0)) @ddt.unpack def test_has_published_version(self, default_ms, mysql_queries, max_find, max_send): """ @@ -3784,7 +3784,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, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 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']) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py index ba618da8f329..636e8e8cea4c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py @@ -154,14 +154,14 @@ def _import_course(self, content_store, modulestore): (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, 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), + (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), ) @ddt.unpack def test_number_mongo_calls(self, store_builder, depth, lazy, access_all_block_fields, num_mongo_calls): @@ -178,7 +178,7 @@ def test_number_mongo_calls(self, store_builder, depth, lazy, access_all_block_f @ddt.data( (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 176), - (MIXED_SPLIT_MODULESTORE_BUILDER, 3), + (MIXED_SPLIT_MODULESTORE_BUILDER, 4), ) @ddt.unpack def test_lazy_when_course_previously_cached(self, store_builder, num_mongo_calls): diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 0970ab8c990f..8cfca804d33a 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -271,22 +271,22 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 34 + QUERY_COUNT = 33 TEST_DATA = { - ('no_overrides', 1, True, False): (QUERY_COUNT, 2), - ('no_overrides', 2, True, False): (QUERY_COUNT, 2), - ('no_overrides', 3, True, False): (QUERY_COUNT, 2), - ('ccx', 1, True, False): (QUERY_COUNT, 2), - ('ccx', 2, True, False): (QUERY_COUNT, 2), - ('ccx', 3, True, False): (QUERY_COUNT, 2), - ('ccx', 1, True, True): (QUERY_COUNT + 3, 2), - ('ccx', 2, True, True): (QUERY_COUNT + 3, 2), - ('ccx', 3, True, True): (QUERY_COUNT + 3, 2), - ('no_overrides', 1, False, False): (QUERY_COUNT, 2), - ('no_overrides', 2, False, False): (QUERY_COUNT, 2), - ('no_overrides', 3, False, False): (QUERY_COUNT, 2), - ('ccx', 1, False, False): (QUERY_COUNT, 2), - ('ccx', 2, False, False): (QUERY_COUNT, 2), - ('ccx', 3, False, False): (QUERY_COUNT, 2), + ('no_overrides', 1, True, False): (QUERY_COUNT, 3), + ('no_overrides', 2, True, False): (QUERY_COUNT, 3), + ('no_overrides', 3, True, False): (QUERY_COUNT, 3), + ('ccx', 1, True, False): (QUERY_COUNT, 3), + ('ccx', 2, True, False): (QUERY_COUNT, 3), + ('ccx', 3, True, False): (QUERY_COUNT, 3), + ('ccx', 1, True, True): (QUERY_COUNT + 2, 3), + ('ccx', 2, True, True): (QUERY_COUNT + 2, 3), + ('ccx', 3, True, True): (QUERY_COUNT + 2, 3), + ('no_overrides', 1, False, False): (QUERY_COUNT, 3), + ('no_overrides', 2, False, False): (QUERY_COUNT, 3), + ('no_overrides', 3, False, False): (QUERY_COUNT, 3), + ('ccx', 1, False, False): (QUERY_COUNT, 3), + ('ccx', 2, False, False): (QUERY_COUNT, 3), + ('ccx', 3, False, False): (QUERY_COUNT, 3), } diff --git a/lms/djangoapps/ccx/tests/test_models.py b/lms/djangoapps/ccx/tests/test_models.py index 97c8761b5946..006ba60de568 100644 --- a/lms/djangoapps/ccx/tests/test_models.py +++ b/lms/djangoapps/ccx/tests/test_models.py @@ -46,7 +46,7 @@ def test_ccx_course_is_correct_course(self): def test_ccx_course_caching(self): """verify that caching the propery works to limit queries""" - with check_mongo_calls(2): + with check_mongo_calls(3): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. @@ -72,7 +72,7 @@ def test_ccx_start_caching(self): """verify that caching the start property works to limit queries""" now = datetime.now(utc) self.set_ccx_override('start', now) - with check_mongo_calls(2): + with check_mongo_calls(3): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. @@ -97,7 +97,7 @@ def test_ccx_due_caching(self): """verify that caching the due property works to limit queries""" expected = datetime.now(utc) self.set_ccx_override('due', expected) - with check_mongo_calls(2): + with check_mongo_calls(3): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 5297f7f9f2aa..6c19b89551fd 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -226,8 +226,8 @@ def test_query_counts_cached(self, store_type, with_storage_backing): @ddt.data( (ModuleStoreEnum.Type.mongo, 5, True, 23), (ModuleStoreEnum.Type.mongo, 5, False, 13), - (ModuleStoreEnum.Type.split, 2, True, 24), - (ModuleStoreEnum.Type.split, 2, False, 14), + (ModuleStoreEnum.Type.split, 3, True, 23), + (ModuleStoreEnum.Type.split, 3, False, 13), ) @ddt.unpack def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries): diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index e6acde7d225d..9b5f6051fe2d 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1016,7 +1016,7 @@ def setup_request_and_course(self, num_finds, num_sends): # - 1 for 5 definitions # Split makes 1 MySQL query to render the toc: # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) @ddt.unpack def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): @@ -1056,7 +1056,7 @@ def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_fi # - 1 for 5 definitions # Split makes 1 MySQL query to render the toc: # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) @ddt.unpack def test_toc_toy_from_section(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 350bf95eda7c..ddafdde6563e 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -389,7 +389,7 @@ class IndexQueryTestCase(ModuleStoreTestCase): @ddt.data( (ModuleStoreEnum.Type.mongo, 10, 164), - (ModuleStoreEnum.Type.split, 3, 161), + (ModuleStoreEnum.Type.split, 4, 160), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index e3fb0132ed82..6c4caa6be16e 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -162,9 +162,9 @@ def verify_response(self, expected_response_code=200, url_params=None): @ddt.data( ('vertical_block', ModuleStoreEnum.Type.mongo, 13), - ('vertical_block', ModuleStoreEnum.Type.split, 4), + ('vertical_block', ModuleStoreEnum.Type.split, 6), ('html_block', ModuleStoreEnum.Type.mongo, 14), - ('html_block', ModuleStoreEnum.Type.split, 4), + ('html_block', ModuleStoreEnum.Type.split, 6), ) @ddt.unpack def test_courseware_html(self, block_name, default_store, mongo_calls): @@ -192,7 +192,7 @@ def test_courseware_html(self, block_name, default_store, mongo_calls): @ddt.data( (ModuleStoreEnum.Type.mongo, 5), - (ModuleStoreEnum.Type.split, 4), + (ModuleStoreEnum.Type.split, 5), ) @ddt.unpack def test_success_enrolled_staff(self, default_store, mongo_calls): diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index efc96a325dc9..efb018980908 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -402,7 +402,7 @@ def inner(self, default_store, module_count, mongo_calls, sql_queries, *args, ** @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 4, 39), - (ModuleStoreEnum.Type.split, 3, 8, 44), + (ModuleStoreEnum.Type.split, 3, 13, 39), ) @ddt.unpack @count_queries @@ -411,7 +411,7 @@ def test_create_thread(self, mock_request): @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 3, 35), - (ModuleStoreEnum.Type.split, 3, 6, 39), + (ModuleStoreEnum.Type.split, 3, 10, 35), ) @ddt.unpack @count_queries diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index ffcffbef6cc5..61103af01c01 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -413,10 +413,10 @@ def test_basic(self): (2, ModuleStoreEnum.Type.mongo, 2, {"Test Topic 1": {"id": "test_topic_1"}}), (2, ModuleStoreEnum.Type.mongo, 2, {"Test Topic 1": {"id": "test_topic_1"}, "Test Topic 2": {"id": "test_topic_2"}}), - (2, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}}), - (2, ModuleStoreEnum.Type.split, 2, + (2, ModuleStoreEnum.Type.split, 3, {"Test Topic 1": {"id": "test_topic_1"}}), + (2, ModuleStoreEnum.Type.split, 3, {"Test Topic 1": {"id": "test_topic_1"}, "Test Topic 2": {"id": "test_topic_2"}}), - (10, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}}), + (10, ModuleStoreEnum.Type.split, 3, {"Test Topic 1": {"id": "test_topic_1"}}), ) @ddt.unpack def test_bulk_response(self, modules_count, module_store, mongo_calls, topics): diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index fb1da7cb6822..0cf438e2319a 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -483,15 +483,15 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 2, 2, 22, 8), - (ModuleStoreEnum.Type.split, False, 50, 2, 2, 22, 8), + (ModuleStoreEnum.Type.split, False, 1, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, False, 50, 3, 3, 21, 8), # Enabling Enterprise integration should have no effect on the number of mongo queries made. (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 2, 2, 22, 8), - (ModuleStoreEnum.Type.split, True, 50, 2, 2, 22, 8), + (ModuleStoreEnum.Type.split, True, 1, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, True, 50, 3, 3, 21, 8), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index cfee479e46ae..c68b17f37bc5 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -164,8 +164,8 @@ def test_block_structure_created_only_once(self): @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39, True), (ModuleStoreEnum.Type.mongo, 1, 39, False), - (ModuleStoreEnum.Type.split, 2, 40, True), - (ModuleStoreEnum.Type.split, 2, 40, False), + (ModuleStoreEnum.Type.split, 3, 39, True), + (ModuleStoreEnum.Type.split, 3, 39, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -178,7 +178,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39), - (ModuleStoreEnum.Type.split, 2, 40), + (ModuleStoreEnum.Type.split, 3, 39), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -224,7 +224,7 @@ def test_other_inaccessible_subsection(self, mock_subsection_signal): @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 22), - (ModuleStoreEnum.Type.split, 2, 23), + (ModuleStoreEnum.Type.split, 3, 22), ) @ddt.unpack def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): @@ -239,7 +239,7 @@ def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_ @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 40), - (ModuleStoreEnum.Type.split, 2, 41), + (ModuleStoreEnum.Type.split, 3, 40), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/grades/tests/test_transformer.py b/lms/djangoapps/grades/tests/test_transformer.py index 98685ea2d534..f2bf8741b8ac 100644 --- a/lms/djangoapps/grades/tests/test_transformer.py +++ b/lms/djangoapps/grades/tests/test_transformer.py @@ -430,7 +430,7 @@ def setUp(self): self.client.login(username=self.student.username, password=password) @ddt.data( - (ModuleStoreEnum.Type.split, 2), + (ModuleStoreEnum.Type.split, 3), (ModuleStoreEnum.Type.mongo, 2), ) @ddt.unpack diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 6be624e9fd9c..90a35f1d6b0b 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -673,6 +673,6 @@ def test_spoc_gradebook_mongo_calls(self): # check MongoDB calls count url = reverse('spoc_gradebook', kwargs={'course_id': self.course.id}) - with check_mongo_calls(6): + with check_mongo_calls(9): response = self.client.get(url) assert response.status_code == 200 diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index c3dd3eb9afb2..3f709480ab45 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -372,7 +372,7 @@ def test_certificate_eligibility(self): @ddt.data( (ModuleStoreEnum.Type.mongo, 4, 46), - (ModuleStoreEnum.Type.split, 2, 47), + (ModuleStoreEnum.Type.split, 3, 46), ) @ddt.unpack def test_query_counts(self, store_type, mongo_count, expected_query_count): diff --git a/openedx/core/djangoapps/bookmarks/tests/test_models.py b/openedx/core/djangoapps/bookmarks/tests/test_models.py index df5d89c5f5fc..c16331f738e0 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_models.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_models.py @@ -276,11 +276,11 @@ def get_bookmark_data(self, block, user=None): (ModuleStoreEnum.Type.mongo, 'sequential_1', ['chapter_1'], 4), (ModuleStoreEnum.Type.mongo, 'vertical_1', ['chapter_1', 'sequential_1'], 6), (ModuleStoreEnum.Type.mongo, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 7), - (ModuleStoreEnum.Type.split, 'course', [], 2), - (ModuleStoreEnum.Type.split, 'chapter_1', [], 1), - (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 1), - (ModuleStoreEnum.Type.split, 'vertical_1', ['chapter_1', 'sequential_1'], 1), - (ModuleStoreEnum.Type.split, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 1), + (ModuleStoreEnum.Type.split, 'course', [], 3), + (ModuleStoreEnum.Type.split, 'chapter_1', [], 2), + (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 2), + (ModuleStoreEnum.Type.split, 'vertical_1', ['chapter_1', 'sequential_1'], 2), + (ModuleStoreEnum.Type.split, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 2), ) @ddt.unpack def test_path_and_queries_on_create(self, store_type, block_to_bookmark, ancestors_attrs, expected_mongo_calls): @@ -376,11 +376,11 @@ def test_path(self, seconds_delta, paths, get_path_call_count, mock_get_path): # (ModuleStoreEnum.Type.mongo, 6, 3, 3), Too slow. (ModuleStoreEnum.Type.mongo, 2, 4, 4), # (ModuleStoreEnum.Type.mongo, 4, 4, 4), - (ModuleStoreEnum.Type.split, 2, 2, 1), - (ModuleStoreEnum.Type.split, 4, 2, 1), - (ModuleStoreEnum.Type.split, 2, 3, 1), + (ModuleStoreEnum.Type.split, 2, 2, 2), + (ModuleStoreEnum.Type.split, 4, 2, 2), + (ModuleStoreEnum.Type.split, 2, 3, 2), # (ModuleStoreEnum.Type.split, 4, 3, 1), - (ModuleStoreEnum.Type.split, 2, 4, 1), + (ModuleStoreEnum.Type.split, 2, 4, 2), ) @ddt.unpack def test_get_path_queries(self, store_type, children_per_block, depth, expected_mongo_calls): diff --git a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py index 615d1b5d8a4e..f033e6bc1e49 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py @@ -108,10 +108,10 @@ def setUp(self): (ModuleStoreEnum.Type.mongo, 4, 3, 5), (ModuleStoreEnum.Type.mongo, 2, 4, 6), # (ModuleStoreEnum.Type.mongo, 4, 4, 6), Too slow. - (ModuleStoreEnum.Type.split, 2, 2, 2), - (ModuleStoreEnum.Type.split, 4, 2, 2), - (ModuleStoreEnum.Type.split, 2, 3, 2), - (ModuleStoreEnum.Type.split, 2, 4, 2), + (ModuleStoreEnum.Type.split, 2, 2, 3), + (ModuleStoreEnum.Type.split, 4, 2, 3), + (ModuleStoreEnum.Type.split, 2, 3, 3), + (ModuleStoreEnum.Type.split, 2, 4, 3), ) @ddt.unpack def test_calculate_course_xblocks_data_queries(self, store_type, children_per_block, depth, expected_mongo_calls): diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 39b30bd4174f..425f2c89a154 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -378,7 +378,7 @@ def test_malformed_grading_policy(self): course_overview = CourseOverview._create_or_update(course) # pylint: disable=protected-access assert course_overview.lowest_passing_grade is None - @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 3, 3)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index e8499de9fb76..36dd785966e2 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -205,8 +205,8 @@ def test_queries(self): # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(73, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): - with check_mongo_calls(3): + with self.assertNumQueries(72, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with check_mongo_calls(4): url = course_home_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index a246486e2db8..a3abea4b3db8 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -49,7 +49,7 @@ def test_queries(self): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(51, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): - with check_mongo_calls(3): + with self.assertNumQueries(50, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url) diff --git a/openedx/tests/completion_integration/test_models.py b/openedx/tests/completion_integration/test_models.py index 74c61de25461..c5fa3c75c16e 100644 --- a/openedx/tests/completion_integration/test_models.py +++ b/openedx/tests/completion_integration/test_models.py @@ -62,7 +62,7 @@ def setUp(self): self.set_up_completion() def test_changed_value(self): - with self.assertNumQueries(SELECT + UPDATE + 2 * SAVEPOINT + 4 * OTHER): + with self.assertNumQueries(SELECT + UPDATE + 2 * SAVEPOINT + 2 * OTHER): # OTHER = user exists, completion exists, 2x look up course in splitmodulestorecourseindex completion, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, @@ -88,7 +88,7 @@ def test_unchanged_value(self): def test_new_user(self): newuser = UserFactory() - with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 2 * OTHER): + with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 0 * OTHER): _, isnew = models.BlockCompletion.objects.submit_completion( user=newuser, block_key=self.block_key, @@ -99,7 +99,7 @@ def test_new_user(self): def test_new_block(self): newblock = UsageKey.from_string('block-v1:edx+test+run+type@video+block@puppers') - with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 2 * OTHER): + with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 0 * OTHER): _, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, block_key=newblock, From 04f8cf71625ef85d3aa6ecd7c82529de06f5adcb Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Mon, 25 Oct 2021 15:54:39 -0400 Subject: [PATCH 06/13] feat: cache active_versions calls --- .../split_mongo/mongo_connection.py | 18 +++++++++++++++--- .../xmodule/modulestore/split_mongo/split.py | 4 ++++ .../tests/test_mixed_modulestore.py | 13 +++++++------ .../xmodule/modulestore/xml_exporter.py | 2 ++ 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 5c651ef6a5f2..cb1c9f282ae5 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -25,7 +25,9 @@ from xmodule.modulestore import BlockData from xmodule.modulestore.split_mongo import BlockKey from xmodule.mongo_utils import connect_to_mongodb, create_collection_index - +from opaque_keys.edx.locator import CourseLocator +from openedx.core.lib.cache_utils import request_cached +from edx_django_utils.cache import RequestCache log = logging.getLogger(__name__) @@ -259,6 +261,7 @@ def __init__( """ # Set a write concern of 1, which makes writes complete successfully to the primary # only before returning. Also makes pymongo report write errors. + RequestCache(namespace="course_index_cache").clear() kwargs['w'] = 1 self.database = connect_to_mongodb( @@ -532,6 +535,7 @@ def close_connections(self): """ Closes any open connections to the underlying databases """ + RequestCache(namespace="course_index_cache").clear() self.database.client.close() def _drop_database(self, database=True, collections=True, connections=True): @@ -546,6 +550,7 @@ def _drop_database(self, database=True, collections=True, connections=True): If connections is True, then close the connection to the database as well. """ + RequestCache(namespace="course_index_cache").clear() connection = self.database.client if database: @@ -571,7 +576,10 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend): # Structures and definitions are only supported in MongoDB for now. # Course indexes are read from MySQL and written to both MongoDB and MySQL - + # Course indexes are cached within the process using their key and ignore_case atrributes as keys. + @request_cached( + "course_index_cache", + arg_map_function=lambda arg: str(arg) if isinstance(arg, (CourseLocator, bool)) else "") def get_course_index(self, key, ignore_case=False): """ Get the course_index from the persistence mechanism whose id is the given key @@ -586,7 +594,7 @@ def get_course_index(self, key, ignore_case=False): # compatible with that. # e.g. test_split_modulestore.py:SplitModuleCourseTests.test_get_course -> get_course(key with only version) # > _load_items > cache_items > begin bulk operations > get_course_index > results in this situation. - log.warning("DjangoFlexPersistenceBackend: get_course_index without org/course/run will always return None") + log.info("DjangoFlexPersistenceBackend: get_course_index without org/course/run will always return None") return None # We never include the branch or the version in the course key in the SplitModulestoreCourseIndex table: key = key.for_branch(None).version_agnostic() @@ -650,6 +658,7 @@ def insert_course_index(self, course_index, course_context=None): # pylint: dis """ Create the course_index in the db """ + RequestCache(namespace="course_index_cache").clear() course_index['last_update'] = datetime.datetime.now(pytz.utc) new_index = SplitModulestoreCourseIndex(**SplitModulestoreCourseIndex.fields_from_v1_schema(course_index)) new_index.save() @@ -669,6 +678,7 @@ def update_course_index(self, course_index, from_index=None, course_context=None # "last_update not only tells us when this course was last updated but also helps prevent collisions" # This code is just copying the behavior of the existing MongoPersistenceBackend # See https://github.com/edx/edx-platform/pull/5200 for context + 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: @@ -728,6 +738,7 @@ def delete_course_index(self, course_key): """ Delete the course_index from the persistence mechanism whose id is the given course_index """ + RequestCache(namespace="course_index_cache").clear() SplitModulestoreCourseIndex.objects.filter(course_id=course_key).delete() # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: super().delete_course_index(course_key) @@ -736,6 +747,7 @@ def _drop_database(self, database=True, collections=True, connections=True): """ Reset data for testing. """ + RequestCache(namespace="course_index_cache").clear() try: SplitModulestoreCourseIndex.objects.all().delete() except TransactionManagementError as err: diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7878b85d076f..7e4aed8b9a41 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -108,6 +108,8 @@ from ..exceptions import ItemNotFoundError from .caching_descriptor_system import CachingDescriptorSystem +from edx_django_utils.cache import RequestCache + log = logging.getLogger(__name__) # ============================================================================== @@ -1410,6 +1412,8 @@ def get_course_index_info(self, course_key): if not (course_key.course and course_key.run and course_key.org): return None + #clear the course index request cache to smooth out collisions with cloning + RequestCache(namespace="course_index_cache").clear() index = self.get_course_index(course_key) return index 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 e0557067dc82..a8d481aceb30 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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, 2], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, [1, 1], 0), (ModuleStoreEnum.Type.split, [2, 1], 0)) @ddt.unpack def test_has_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -391,7 +391,7 @@ 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, 2], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, [3, 2], 0), (ModuleStoreEnum.Type.split, 0, [2, 1], 0)) @ddt.unpack def test_get_item(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -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, 4, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 14, 0), (ModuleStoreEnum.Type.split, 0, 3, 0)) @ddt.unpack def test_get_items(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -522,7 +522,7 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 5), (ModuleStoreEnum.Type.split, 3, 3, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) @ddt.unpack def test_update_item(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, 6, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 0), (ModuleStoreEnum.Type.split, 0, 5, 0)) @ddt.unpack def test_get_courses(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -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, 2], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, [12, 3], 0), (ModuleStoreEnum.Type.split, 0, [3, 1], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends): """ @@ -1802,6 +1802,7 @@ def test_reset_course_to_version(self): # Find the version_guid of our course by diving into Split Mongo. split = self._get_split_modulestore() course_index = split.get_course_index(self.course.location.course_key) + log.warning(f"Banana course index: {course_index}") original_version_guid = course_index["versions"]["published-branch"] # Reset course to currently-published version. diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 767be91cffd5..811e93eb49c9 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -20,6 +20,7 @@ from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots +from edx_django_utils.cache import RequestCache DRAFT_DIR = "drafts" PUBLISHED_DIR = "published" @@ -31,6 +32,7 @@ def _export_drafts(modulestore, course_key, export_fs, xml_centric_course_key): """ Exports course drafts. """ + RequestCache(namespace="course_index_cache").clear() # NOTE: we need to explicitly implement the logic for setting the vertical's parent # and index here since the XML modulestore cannot load draft modules with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): From ffa32b2c8897e1e345042bb29a20924237094357 Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Tue, 26 Oct 2021 15:40:10 -0400 Subject: [PATCH 07/13] fix: query counts + argmap func --- .../xmodule/modulestore/split_mongo/mongo_connection.py | 2 +- lms/djangoapps/course_api/blocks/tests/test_api.py | 4 ++-- lms/djangoapps/courseware/tests/test_module_render.py | 2 +- lms/djangoapps/courseware/testutils.py | 4 ++-- .../discussion/django_comment_client/base/tests.py | 4 ++-- lms/djangoapps/discussion/tests/test_views.py | 8 ++++---- lms/djangoapps/grades/tests/test_tasks.py | 8 ++++---- .../instructor/tests/views/test_instructor_dashboard.py | 2 +- openedx/core/djangoapps/bookmarks/tests/test_models.py | 8 ++++---- .../course_overviews/tests/test_course_overviews.py | 2 +- 10 files changed, 22 insertions(+), 22 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index cb1c9f282ae5..2ef0677f6402 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -579,7 +579,7 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend): # Course indexes are cached within the process using their key and ignore_case atrributes as keys. @request_cached( "course_index_cache", - arg_map_function=lambda arg: str(arg) if isinstance(arg, (CourseLocator, bool)) else "") + arg_map_function=lambda arg: str(arg) if not isinstance(arg,DjangoFlexPersistenceBackend) else "") def get_course_index(self, key, ignore_case=False): """ Get the course_index from the persistence mechanism whose id is the given key diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 6c19b89551fd..8c3c71d1fc48 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -226,8 +226,8 @@ def test_query_counts_cached(self, store_type, with_storage_backing): @ddt.data( (ModuleStoreEnum.Type.mongo, 5, True, 23), (ModuleStoreEnum.Type.mongo, 5, False, 13), - (ModuleStoreEnum.Type.split, 3, True, 23), - (ModuleStoreEnum.Type.split, 3, False, 13), + (ModuleStoreEnum.Type.split, 2, True, 23), + (ModuleStoreEnum.Type.split, 2, False, 13), ) @ddt.unpack def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries): diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 9b5f6051fe2d..e734cc5b8489 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1016,7 +1016,7 @@ def setup_request_and_course(self, num_finds, num_sends): # - 1 for 5 definitions # Split makes 1 MySQL query to render the toc: # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) @ddt.unpack def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index 6c4caa6be16e..c624221e3c8f 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -162,9 +162,9 @@ def verify_response(self, expected_response_code=200, url_params=None): @ddt.data( ('vertical_block', ModuleStoreEnum.Type.mongo, 13), - ('vertical_block', ModuleStoreEnum.Type.split, 6), + ('vertical_block', ModuleStoreEnum.Type.split, 5), ('html_block', ModuleStoreEnum.Type.mongo, 14), - ('html_block', ModuleStoreEnum.Type.split, 6), + ('html_block', ModuleStoreEnum.Type.split, 5), ) @ddt.unpack def test_courseware_html(self, block_name, default_store, mongo_calls): diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index efb018980908..3b65c4636533 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -402,7 +402,7 @@ def inner(self, default_store, module_count, mongo_calls, sql_queries, *args, ** @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 4, 39), - (ModuleStoreEnum.Type.split, 3, 13, 39), + (ModuleStoreEnum.Type.split, 3, 11, 39), ) @ddt.unpack @count_queries @@ -411,7 +411,7 @@ def test_create_thread(self, mock_request): @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 3, 35), - (ModuleStoreEnum.Type.split, 3, 10, 35), + (ModuleStoreEnum.Type.split, 3, 9, 35), ) @ddt.unpack @count_queries diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 0cf438e2319a..553d2602799e 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -483,15 +483,15 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 21, 8), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, False, 1, 2, 2, 21, 8), + (ModuleStoreEnum.Type.split, False, 50, 2, 2, 21, 8), # Enabling Enterprise integration should have no effect on the number of mongo queries made. (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 21, 8), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, True, 1, 2, 2, 21, 8), + (ModuleStoreEnum.Type.split, True, 50, 2, 2, 21, 8), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index c68b17f37bc5..c2d3033cc51d 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -164,8 +164,8 @@ def test_block_structure_created_only_once(self): @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39, True), (ModuleStoreEnum.Type.mongo, 1, 39, False), - (ModuleStoreEnum.Type.split, 3, 39, True), - (ModuleStoreEnum.Type.split, 3, 39, False), + (ModuleStoreEnum.Type.split, 2, 39, True), + (ModuleStoreEnum.Type.split, 2, 39, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -178,7 +178,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39), - (ModuleStoreEnum.Type.split, 3, 39), + (ModuleStoreEnum.Type.split, 2, 39), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -239,7 +239,7 @@ def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_ @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 40), - (ModuleStoreEnum.Type.split, 3, 40), + (ModuleStoreEnum.Type.split, 2, 40), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 90a35f1d6b0b..70763e100cd8 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -673,6 +673,6 @@ def test_spoc_gradebook_mongo_calls(self): # check MongoDB calls count url = reverse('spoc_gradebook', kwargs={'course_id': self.course.id}) - with check_mongo_calls(9): + with check_mongo_calls(7): response = self.client.get(url) assert response.status_code == 200 diff --git a/openedx/core/djangoapps/bookmarks/tests/test_models.py b/openedx/core/djangoapps/bookmarks/tests/test_models.py index c16331f738e0..34c21ad1275f 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_models.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_models.py @@ -376,11 +376,11 @@ def test_path(self, seconds_delta, paths, get_path_call_count, mock_get_path): # (ModuleStoreEnum.Type.mongo, 6, 3, 3), Too slow. (ModuleStoreEnum.Type.mongo, 2, 4, 4), # (ModuleStoreEnum.Type.mongo, 4, 4, 4), - (ModuleStoreEnum.Type.split, 2, 2, 2), - (ModuleStoreEnum.Type.split, 4, 2, 2), - (ModuleStoreEnum.Type.split, 2, 3, 2), + (ModuleStoreEnum.Type.split, 2, 2, 1), + (ModuleStoreEnum.Type.split, 4, 2, 1), + (ModuleStoreEnum.Type.split, 2, 3, 1), # (ModuleStoreEnum.Type.split, 4, 3, 1), - (ModuleStoreEnum.Type.split, 2, 4, 2), + (ModuleStoreEnum.Type.split, 2, 4, 1), ) @ddt.unpack def test_get_path_queries(self, store_type, children_per_block, depth, expected_mongo_calls): diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 425f2c89a154..39b30bd4174f 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -378,7 +378,7 @@ def test_malformed_grading_policy(self): course_overview = CourseOverview._create_or_update(course) # pylint: disable=protected-access assert course_overview.lowest_passing_grade is None - @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 2, 2)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ From 80a0e88f78c95677daf26c25d59d054e030ae4bc Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Tue, 26 Oct 2021 16:20:53 -0400 Subject: [PATCH 08/13] fix: code quality fixes --- .../xmodule/xmodule/modulestore/split_mongo/mongo_connection.py | 2 +- .../xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 0f134bb19737..eddd897d96c2 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -579,7 +579,7 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend): # Course indexes are cached within the process using their key and ignore_case atrributes as keys. @request_cached( "course_index_cache", - arg_map_function=lambda arg: str(arg) if not isinstance(arg,DjangoFlexPersistenceBackend) else "") + arg_map_function=lambda arg: str(arg) if not isinstance(arg, DjangoFlexPersistenceBackend) else "") def get_course_index(self, key, ignore_case=False): """ Get the course_index from the persistence mechanism whose id is the given key 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 b4d084688c56..a8d481aceb30 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -369,7 +369,6 @@ def test_duplicate_course_error_with_different_case_ids(self, default_store): # 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.unpack def test_has_item(self, default_ms, max_find, max_send): self.initdb(default_ms) From 6901f7a0a3656f3801b3fbfc6c5856728e1179bf Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Wed, 27 Oct 2021 09:01:31 -0400 Subject: [PATCH 09/13] Fix: enable reset of caches in testing --- common/lib/xmodule/xmodule/modulestore/tests/django_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index bd94c642d851..6eb8cc822965 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -275,7 +275,7 @@ def my_test(self): """ MODULESTORE = functools.partial(mixed_store_config, mkdtemp_clean(), {}) CONTENTSTORE = functools.partial(contentstore_config) - ENABLED_CACHES = ['default', 'mongo_metadata_inheritance', 'loc_cache'] + ENABLED_CACHES = ['default', 'mongo_metadata_inheritance', 'loc_cache', 'course_index_cache'] # List of modulestore signals enabled for this test. Defaults to an empty # list. The list of signals available is found on the SignalHandler class, From 0f681d15d447559ef7441e9b1db327302f13d280 Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Wed, 27 Oct 2021 09:21:38 -0400 Subject: [PATCH 10/13] fix: tick up query counts for cleared cache tests --- .../course_api/blocks/tests/test_api.py | 8 ++++---- .../courseware/tests/test_module_render.py | 2 +- lms/djangoapps/grades/tests/test_tasks.py | 18 +++++++++--------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index c9e3fa671087..71c5905b8c52 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -224,10 +224,10 @@ def test_query_counts_cached(self, store_type, with_storage_backing): ) @ddt.data( - (ModuleStoreEnum.Type.mongo, 5, True, 23), - (ModuleStoreEnum.Type.mongo, 5, False, 13), - (ModuleStoreEnum.Type.split, 2, True, 23), - (ModuleStoreEnum.Type.split, 2, False, 13), + (ModuleStoreEnum.Type.mongo, 5, True, 24), + (ModuleStoreEnum.Type.mongo, 5, False, 14), + (ModuleStoreEnum.Type.split, 2, True, 24), + (ModuleStoreEnum.Type.split, 2, False, 14), ) @ddt.unpack def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries): diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index e734cc5b8489..e6acde7d225d 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1056,7 +1056,7 @@ def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_fi # - 1 for 5 definitions # Split makes 1 MySQL query to render the toc: # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) @ddt.unpack def test_toc_toy_from_section(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 89bc44f0d3fd..cd4aab75d331 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -162,10 +162,10 @@ def test_block_structure_created_only_once(self): assert mock_block_structure_create.call_count == 1 @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 39, True), - (ModuleStoreEnum.Type.mongo, 1, 39, False), - (ModuleStoreEnum.Type.split, 2, 39, True), - (ModuleStoreEnum.Type.split, 2, 39, False), + (ModuleStoreEnum.Type.mongo, 1, 40, True), + (ModuleStoreEnum.Type.mongo, 1, 40, False), + (ModuleStoreEnum.Type.split, 2, 40, True), + (ModuleStoreEnum.Type.split, 2, 40, False), ) @ddt.unpack @@ -178,8 +178,8 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 39), - (ModuleStoreEnum.Type.split, 2, 39), + (ModuleStoreEnum.Type.mongo, 1, 40), + (ModuleStoreEnum.Type.split, 2, 40), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -225,7 +225,7 @@ def test_other_inaccessible_subsection(self, mock_subsection_signal): @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 23), - (ModuleStoreEnum.Type.split, 3, 23), + (ModuleStoreEnum.Type.split, 2, 23), ) @ddt.unpack def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): @@ -239,8 +239,8 @@ def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_ assert len(PersistentSubsectionGrade.bulk_read_grades(self.user.id, self.course.id)) == 0 @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 40), - (ModuleStoreEnum.Type.split, 2, 40), + (ModuleStoreEnum.Type.mongo, 1, 41), + (ModuleStoreEnum.Type.split, 2, 41), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): From e2e558c10f0b61ed25bd373f075dd497d77e7ca4 Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Wed, 27 Oct 2021 09:56:54 -0400 Subject: [PATCH 11/13] fix: remove unused import --- .../xmodule/xmodule/modulestore/split_mongo/mongo_connection.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index eddd897d96c2..2cb31e7d97e9 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -25,7 +25,6 @@ from xmodule.modulestore import BlockData from xmodule.modulestore.split_mongo import BlockKey from xmodule.mongo_utils import connect_to_mongodb, create_collection_index -from opaque_keys.edx.locator import CourseLocator from openedx.core.lib.cache_utils import request_cached from edx_django_utils.cache import RequestCache From b8f8f7a2c566fba4c5a2e239df6f87b5ed438f0e Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Wed, 27 Oct 2021 11:41:19 -0400 Subject: [PATCH 12/13] fix: consolidate invalidations in the mongo layer --- .../xmodule/modulestore/split_mongo/mongo_connection.py | 9 ++++++++- .../lib/xmodule/xmodule/modulestore/split_mongo/split.py | 2 -- common/lib/xmodule/xmodule/modulestore/xml_exporter.py | 1 - 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 2cb31e7d97e9..de1ecc2ec13c 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -260,9 +260,11 @@ def __init__( """ # Set a write concern of 1, which makes writes complete successfully to the primary # only before returning. Also makes pymongo report write errors. - RequestCache(namespace="course_index_cache").clear() kwargs['w'] = 1 + #make sure the course index cache is fresh. + RequestCache(namespace="course_index_cache").clear() + self.database = connect_to_mongodb( db, host, port=port, tz_aware=tz_aware, user=user, password=password, @@ -576,6 +578,9 @@ class DjangoFlexPersistenceBackend(MongoPersistenceBackend): # Structures and definitions are only supported in MongoDB for now. # Course indexes are read from MySQL and written to both MongoDB and MySQL # Course indexes are cached within the process using their key and ignore_case atrributes as keys. + # This method is request cached. The keys to the cache are the arguements to the method. + # The `self` arguement is discarded as a key using an isinstance check. + # This is because the DjangoFlexPersistenceBackend could be different in reference to the same course key. @request_cached( "course_index_cache", arg_map_function=lambda arg: str(arg) if not isinstance(arg, DjangoFlexPersistenceBackend) else "") @@ -657,6 +662,8 @@ def insert_course_index(self, course_index, course_context=None): # pylint: dis """ Create the course_index in the db """ + # clear the whole course_index request cache, required for sucessfully cloning a course. + # This is a relatively large hammer for the problem, but we mostly only use one course at a time. RequestCache(namespace="course_index_cache").clear() course_index['last_update'] = datetime.datetime.now(pytz.utc) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7e4aed8b9a41..fd3f42e0f738 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1412,8 +1412,6 @@ def get_course_index_info(self, course_key): if not (course_key.course and course_key.run and course_key.org): return None - #clear the course index request cache to smooth out collisions with cloning - RequestCache(namespace="course_index_cache").clear() index = self.get_course_index(course_key) return index diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 811e93eb49c9..5f98d745c56d 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -32,7 +32,6 @@ def _export_drafts(modulestore, course_key, export_fs, xml_centric_course_key): """ Exports course drafts. """ - RequestCache(namespace="course_index_cache").clear() # NOTE: we need to explicitly implement the logic for setting the vertical's parent # and index here since the XML modulestore cannot load draft modules with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): From eb71c84c53e96913d9249d5bde3ba318fc62ccfe Mon Sep 17 00:00:00 2001 From: connorhaugh Date: Wed, 27 Oct 2021 12:18:33 -0400 Subject: [PATCH 13/13] fix: remove unused import --- common/lib/xmodule/xmodule/modulestore/split_mongo/split.py | 2 -- common/lib/xmodule/xmodule/modulestore/xml_exporter.py | 1 - 2 files changed, 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index fd3f42e0f738..7878b85d076f 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -108,8 +108,6 @@ from ..exceptions import ItemNotFoundError from .caching_descriptor_system import CachingDescriptorSystem -from edx_django_utils.cache import RequestCache - log = logging.getLogger(__name__) # ============================================================================== diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 5f98d745c56d..767be91cffd5 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -20,7 +20,6 @@ from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots -from edx_django_utils.cache import RequestCache DRAFT_DIR = "drafts" PUBLISHED_DIR = "published"