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

Merge pull request #19964 from cpennington/fbe-query-perf

Browse Source 
Sort in python and add an index to improve StackedConfigModel.current…
This commit is contained in:
Calen Pennington
2019-03-11 12:22:59 -04:00
committed by GitHub
parent 129438420e a2a340bcd0
commit 5d3c9b36df
9 changed files with 118 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
common/djangoapps/django_comment_common/models.py
View File

@@ -121,7 +121,14 @@ class Role(models.Model):
"""
Returns True if the user has one of the given roles for the given course
"""
return Role.objects.filter(course_id=course_id, name__in=role_names, users=user).exists()
if 'roles' in getattr(user, '_prefetched_objects_cache', {}):
# Don't blow up the prefetch cache
return any(
role.course_id == course_id and role.name in role_names
for role in user.roles.all()
)
else:
return user.roles.filter(course_id=course_id, name__in=role_names).exists()
class Permission(models.Model):

20
common/djangoapps/student/models.py
View File

@@ -1195,13 +1195,19 @@ class CourseEnrollment(models.Model):
if user.is_anonymous:
return None
try:
query = cls.objects
if select_related is not None:
query = query.select_related(*select_related)
return query.get(
user=user,
course_id=course_key
)
if 'courseenrollment' in getattr(user, '_prefetched_objects_cache', {}):
for enrollment in user.courseenrollment_set.all():
if enrollment.course_id == course_key:
return enrollment
return None
else:
query = cls.objects
if select_related is not None:
query = query.select_related(*select_related)
return query.get(
user=user,
course_id=course_key
)
except cls.DoesNotExist:
return None

14
lms/djangoapps/course_api/tests/test_views.py
View File

@@ -277,6 +277,7 @@ class CourseDetailViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase
})
@override_settings(SEARCH_ENGINE="search.tests.mock_search_engine.MockSearchEngine")
@override_settings(COURSEWARE_INDEX_NAME=TEST_INDEX_NAME)
@ddt.ddt
class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, SearcherMixin):
"""
Tests the search functionality of the courses API.
@@ -355,7 +356,8 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear
self.assertEqual(res.data['pagination']['count'], 3)
self.assertEqual(len(res.data['results']), 1) # Should return a single course
def test_too_many_courses(self):
@ddt.data(True, False)
def test_too_many_courses(self, with_username):
"""
Test that search results are limited to 100 courses, and that they don't
blow up the database.
@@ -394,8 +396,14 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear
for page in range(1, 12):
RequestCache.clear_all_namespaces()
with self.assertNumQueries(query_counts[page - 1]):
response = self.verify_response(params={'page': page, 'page_size': 30})
expected_query_count = query_counts[page - 1]
if with_username:
expected_query_count += 4
with self.assertNumQueries(expected_query_count):
params = {'page': page, 'page_size': 30}
if with_username:
params['username'] = self.audit_user.username
response = self.verify_response(params=params)
self.assertIn('results', response.data)
self.assertEqual(response.data['pagination']['count'], 303)

5
lms/djangoapps/courseware/courses.py
View File

@@ -25,7 +25,7 @@ from courseware.model_data import FieldDataCache
from courseware.module_render import get_module
from course_modes.models import CourseMode
from django.conf import settings
from django.db.models import Prefetch
from django.db.models import Prefetch, prefetch_related_objects
from django.urls import reverse
from django.http import Http404, QueryDict
from enrollment.api import get_course_enrollment_details
@@ -474,7 +474,8 @@ def get_courses(user, org=None, filter_=None):
'COURSE_CATALOG_VISIBILITY_PERMISSION',
settings.COURSE_CATALOG_VISIBILITY_PERMISSION
)
if user.is_authenticated:
prefetch_related_objects([user], 'roles', 'courseenrollment_set', 'experimentdata_set')
return LazySequence(
(c for c in courses if has_access(user, permission_name, c)),
est_len=courses.count()

28
openedx/core/djangoapps/config_model_utils/models.py
View File

@@ -43,6 +43,11 @@ class StackedConfigurationModel(ConfigurationModel):
"""
class Meta(object):
abstract = True
indexes = [
# This index optimizes the .object.current_set() query
# by preventing a filesort
models.Index(fields=['site', 'org', 'course'])
]
KEY_FIELDS = ('site', 'org', 'course')
STACKABLE_FIELDS = ('enabled',)
@@ -134,23 +139,18 @@ class StackedConfigurationModel(ConfigurationModel):
site_override_q |
org_override_q |
course_override_q
).order_by(
# Sort nulls first, and in reverse specificity order
# so that the overrides are in the order of general to specific.
#
# Site | Org | Course
# --------------------
# Null | Null | Null
# site | Null | Null
# Null | org | Null
# Null | Null | Course
F('course').desc(nulls_first=True),
F('org').desc(nulls_first=True),
F('site').desc(nulls_first=True),
)
provenances = defaultdict(lambda: Provenance.default)
for override in overrides:
# We are sorting in python to avoid doing a filesort in the database for
# what will only be 4 rows at maximum
for override in sorted(
overrides,
# This particular sort order sorts None before not-None (because False < True)
# It sorts global first (because all entries are None), then site entries
# (because course_id and org are None), then org and course (by the same logic)
key=lambda o: (o.course_id is not None, o.org is not None, o.site_id is not None)
):
for field in stackable_fields:
value = field.value_from_object(override)
if value != field_defaults[field.name]:

28
openedx/core/djangoapps/config_model_utils/utils.py
View File

@@ -12,14 +12,24 @@ def is_in_holdback(user):
"""
in_holdback = False
if user and user.is_authenticated:
try:
holdback_value = ExperimentData.objects.get(
user=user,
experiment_id=EXPERIMENT_ID,
key=EXPERIMENT_DATA_HOLDBACK_KEY,
).value
in_holdback = holdback_value == 'True'
except ExperimentData.DoesNotExist:
pass
if 'experimentdata' in getattr(user, '_prefetched_objects_cache', {}):
for experiment_data in user.experimentdata_set.all():
if (
experiment_data.experiment_id == EXPERIMENT_ID and
experiment_data.key == EXPERIMENT_DATA_HOLDBACK_KEY
):
in_holdback = experiment_data.value == 'True'
break
else:
try:
holdback_value = ExperimentData.objects.get(
user=user,
experiment_id=EXPERIMENT_ID,
key=EXPERIMENT_DATA_HOLDBACK_KEY,
).value
except ExperimentData.DoesNotExist:
pass
else:
in_holdback = holdback_value == 'True'
return in_holdback

19
openedx/features/content_type_gating/migrations/0006_auto_20190308_1447.py Normal file
View File

@@ -0,0 +1,19 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.20 on 2019-03-08 14:47
from __future__ import unicode_literals
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('content_type_gating', '0005_auto_20190306_1547'),
]
operations = [
migrations.AddIndex(
model_name='contenttypegatingconfig',
index=models.Index(fields=['site', 'org', 'course'], name='content_typ_site_id_e91576_idx'),
),
]

19
openedx/features/course_duration_limits/migrations/0006_auto_20190308_1447.py Normal file
View File

@@ -0,0 +1,19 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.20 on 2019-03-08 14:47
from __future__ import unicode_literals
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('course_duration_limits', '0005_auto_20190306_1546'),
]
operations = [
migrations.AddIndex(
model_name='coursedurationlimitconfig',
index=models.Index(fields=['site', 'org', 'course'], name='course_dura_site_id_424016_idx'),
),
]

18
openedx/features/course_duration_limits/tests/test_models.py
View File

@@ -138,10 +138,10 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase):
@ddt.data(
# Generate all combinations of setting each configuration level to True/False/None
*itertools.product(*[(True, False, None)] * 4)
*itertools.product(*([(True, False, None)] * 4 + [(True, False)]))
)
@ddt.unpack
def test_config_overrides(self, global_setting, site_setting, org_setting, course_setting):
def test_config_overrides(self, global_setting, site_setting, org_setting, course_setting, reverse_order):
"""
Test that the stacked configuration overrides happen in the correct order and priority.
@@ -170,10 +170,16 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase):
test_course = CourseOverviewFactory.create(org='test-org')
test_site_cfg = SiteConfigurationFactory.create(values={'course_org_filter': test_course.org})
CourseDurationLimitConfig.objects.create(enabled=global_setting)
CourseDurationLimitConfig.objects.create(course=test_course, enabled=course_setting)
CourseDurationLimitConfig.objects.create(org=test_course.org, enabled=org_setting)
CourseDurationLimitConfig.objects.create(site=test_site_cfg.site, enabled=site_setting)
if reverse_order:
CourseDurationLimitConfig.objects.create(site=test_site_cfg.site, enabled=site_setting)
CourseDurationLimitConfig.objects.create(org=test_course.org, enabled=org_setting)
CourseDurationLimitConfig.objects.create(course=test_course, enabled=course_setting)
CourseDurationLimitConfig.objects.create(enabled=global_setting)
else:
CourseDurationLimitConfig.objects.create(enabled=global_setting)
CourseDurationLimitConfig.objects.create(course=test_course, enabled=course_setting)
CourseDurationLimitConfig.objects.create(org=test_course.org, enabled=org_setting)
CourseDurationLimitConfig.objects.create(site=test_site_cfg.site, enabled=site_setting)
expected_global_setting = self._resolve_settings([global_setting])
expected_site_setting = self._resolve_settings([global_setting, site_setting])