From 72ed987dff60bb27bb830e7c229cf3a97d0aaa4e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 11 Jun 2018 10:27:52 -0400 Subject: [PATCH] Enable model-caching of discussions ID mapping. Doing modulestore lookups is expensive, so commit 695b036 created a course_publish listener that would materialize the discussion ID to XBlock usage key mapping into the CourseDiscussionSettings model. However, the signal wasn't hooked up to the Studio process, so that async task was never called. When hooking it up, I also discovered that bok choy tests related to partitioning were failing because of a race condition where multiple processes are overwriting the discussion settings. To make sure this wasn't an issue, I moved the mapping to its own table. This is part of ARCH-111, and the overall Course Structures API deprecation. --- .../migrations/0007_discussionsidmapping.py | 24 +++++++++++++++++++ .../django_comment_common/models.py | 21 ++++++++++++++++ .../djangoapps/django_comment_common/utils.py | 3 --- lms/djangoapps/discussion/apps.py | 5 +++- lms/djangoapps/discussion/tasks.py | 3 ++- .../discussion/tests/test_signals.py | 7 ++---- .../django_comment_client/base/tests.py | 8 +++---- setup.py | 6 +++++ 8 files changed, 63 insertions(+), 14 deletions(-) create mode 100644 common/djangoapps/django_comment_common/migrations/0007_discussionsidmapping.py diff --git a/common/djangoapps/django_comment_common/migrations/0007_discussionsidmapping.py b/common/djangoapps/django_comment_common/migrations/0007_discussionsidmapping.py new file mode 100644 index 0000000000..c41d9cebe3 --- /dev/null +++ b/common/djangoapps/django_comment_common/migrations/0007_discussionsidmapping.py @@ -0,0 +1,24 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.13 on 2018-06-13 12:10 +from __future__ import unicode_literals + +from django.db import migrations +import jsonfield.fields +import opaque_keys.edx.django.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('django_comment_common', '0006_coursediscussionsettings_discussions_id_map'), + ] + + operations = [ + migrations.CreateModel( + name='DiscussionsIdMapping', + fields=[ + ('course_id', opaque_keys.edx.django.models.CourseKeyField(db_index=True, max_length=255, primary_key=True, serialize=False)), + ('mapping', jsonfield.fields.JSONField(help_text=b'Key/value store mapping discussion IDs to discussion XBlock usage keys.')), + ], + ), + ] diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index 1f7b16a6ec..0d27aa9a07 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -204,3 +204,24 @@ class CourseDiscussionSettings(models.Model): def divided_discussions(self, value): """Un-Jsonify the divided_discussions""" self._divided_discussions = json.dumps(value) + + +class DiscussionsIdMapping(models.Model): + """This model is a performance optimization, updated on course publish.""" + course_id = CourseKeyField(db_index=True, primary_key=True, max_length=255) + mapping = JSONField( + help_text="Key/value store mapping discussion IDs to discussion XBlock usage keys.", + ) + + @classmethod + def update_mapping(cls, course_key, discussions_id_map): + """Update the mapping of discussions IDs to XBlock usage key strings.""" + mapping_entry, created = cls.objects.get_or_create( + course_id=course_key, + defaults={ + 'mapping': discussions_id_map, + }, + ) + if not created: + mapping_entry.mapping = discussions_id_map + mapping_entry.save() diff --git a/common/djangoapps/django_comment_common/utils.py b/common/djangoapps/django_comment_common/utils.py index dc810c33bb..30384fbee3 100644 --- a/common/djangoapps/django_comment_common/utils.py +++ b/common/djangoapps/django_comment_common/utils.py @@ -139,8 +139,6 @@ def set_course_discussion_settings(course_key, **kwargs): divided_discussions (list): List of discussion ids. division_scheme (str): `CourseDiscussionSettings.NONE`, `CourseDiscussionSettings.COHORT`, or `CourseDiscussionSettings.ENROLLMENT_TRACK` - discussions_id_map (dict): Dict containing discussion IDs as keys and the associated discussion - XBlock usage keys as values. Returns: A CourseDiscussionSettings object. @@ -149,7 +147,6 @@ def set_course_discussion_settings(course_key, **kwargs): 'division_scheme': basestring, 'always_divide_inline_discussions': bool, 'divided_discussions': list, - 'discussions_id_map': dict, } course_discussion_settings = get_course_discussion_settings(course_key) for field, field_type in fields.items(): diff --git a/lms/djangoapps/discussion/apps.py b/lms/djangoapps/discussion/apps.py index 5e98707ef6..43f7d9d1f6 100644 --- a/lms/djangoapps/discussion/apps.py +++ b/lms/djangoapps/discussion/apps.py @@ -25,9 +25,12 @@ class DiscussionConfig(AppConfig): } }, PluginSettings.CONFIG: { + ProjectType.CMS: { + SettingsType.COMMON: {PluginSettings.RELATIVE_PATH: u'settings.common'}, + }, ProjectType.LMS: { SettingsType.COMMON: {PluginSettings.RELATIVE_PATH: u'settings.common'}, - } + }, } } diff --git a/lms/djangoapps/discussion/tasks.py b/lms/djangoapps/discussion/tasks.py index 00615027f6..b01fe419b4 100644 --- a/lms/djangoapps/discussion/tasks.py +++ b/lms/djangoapps/discussion/tasks.py @@ -13,6 +13,7 @@ from django.contrib.sites.models import Site from celery_utils.logged_task import LoggedTask from django_comment_common.utils import set_course_discussion_settings +from django_comment_common.models import DiscussionsIdMapping from edx_ace import ace from edx_ace.utils import date from edx_ace.recipient import Recipient @@ -48,7 +49,7 @@ def update_discussions_map(context): discussion_block.discussion_id: unicode(discussion_block.location) for discussion_block in discussion_blocks } - set_course_discussion_settings(course_key, discussions_id_map=discussions_id_map) + DiscussionsIdMapping.update_mapping(course_key, discussions_id_map) class ResponseNotification(BaseMessageType): diff --git a/lms/djangoapps/discussion/tests/test_signals.py b/lms/djangoapps/discussion/tests/test_signals.py index d15dbdd04c..d99aee9d5c 100644 --- a/lms/djangoapps/discussion/tests/test_signals.py +++ b/lms/djangoapps/discussion/tests/test_signals.py @@ -70,9 +70,6 @@ class CoursePublishHandlerTestCase(ModuleStoreTestCase): course_key_args = dict(org='org', course='number', run='run') course_key = self.store.make_course_key(**course_key_args) - with self.assertRaises(models.CourseDiscussionSettings.DoesNotExist): - models.CourseDiscussionSettings.objects.get(course_id=course_key) - # create course course = CourseFactory(emit_signals=True, **course_key_args) self.assertEqual(course.id, course_key) @@ -92,5 +89,5 @@ class CoursePublishHandlerTestCase(ModuleStoreTestCase): """ Verifies the discussion ID map for the given course matches the expected value. """ - discussion_settings = models.CourseDiscussionSettings.objects.get(course_id=course_key) - self.assertDictEqual(discussion_settings.discussions_id_map, expected_map) + mapping_entry = models.DiscussionsIdMapping.objects.get(course_id=course_key) + self.assertDictEqual(mapping_entry.mapping, expected_map) diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index a8b86e0f4a..72640ab2a9 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -405,8 +405,8 @@ class ViewsQueryCountTestCase( return inner @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 3, 30), - (ModuleStoreEnum.Type.split, 3, 11, 30), + (ModuleStoreEnum.Type.mongo, 3, 4, 35), + (ModuleStoreEnum.Type.split, 3, 13, 35), ) @ddt.unpack @count_queries @@ -414,8 +414,8 @@ class ViewsQueryCountTestCase( self.create_thread_helper(mock_request) @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 2, 26), - (ModuleStoreEnum.Type.split, 3, 8, 26), + (ModuleStoreEnum.Type.mongo, 3, 3, 31), + (ModuleStoreEnum.Type.split, 3, 10, 31), ) @ddt.unpack @count_queries diff --git a/setup.py b/setup.py index 3728fa5d83..7051f3b445 100644 --- a/setup.py +++ b/setup.py @@ -79,6 +79,12 @@ setup( ], "cms.djangoapp": [ "ace_common = openedx.core.djangoapps.ace_common.apps:AceCommonConfig", + # Importing an LMS app into the Studio process is not a good + # practice. We're ignoring this for Discussions here because its + # placement in LMS is a historical artifact. The eventual goal is to + # consolidate the multiple discussions-related Django apps and + # either put them in the openedx/ dir, or in another repo entirely. + "discussion = lms.djangoapps.discussion.apps:DiscussionConfig", "plugins = openedx.core.djangoapps.plugins.apps:PluginsConfig", "schedules = openedx.core.djangoapps.schedules.apps:SchedulesConfig", "theming = openedx.core.djangoapps.theming.apps:ThemingConfig",