From f4e69275eeba08c07c3921d0264fb763dc30f69a Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 10 Dec 2013 12:15:16 -0500 Subject: [PATCH] Use mongo indices for all queries STUD-1039 --- cms/djangoapps/contentstore/views/course.py | 3 +- .../xmodule/modulestore/loc_mapper_store.py | 64 +++++++++++++------ .../modulestore/tests/test_location_mapper.py | 10 +-- lms/djangoapps/courseware/roles.py | 9 ++- mongo_indexes.md | 21 ++++++ 5 files changed, 79 insertions(+), 28 deletions(-) create mode 100644 mongo_indexes.md diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index c7e379869b..39695fc66e 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -116,7 +116,8 @@ def course_listing(request): """ List all courses available to the logged in user """ - courses = modulestore('direct').get_items(['i4x', None, None, 'course', None]) + # there's an index on category which will be used if none of its antecedents are set + courses = modulestore('direct').get_items([None, None, None, 'course', None]) # filter out courses that we don't have access too def course_filter(course): diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index c7355c2f29..4bf5f1c64b 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -4,10 +4,10 @@ Method for converting among our differing Location/Locator whatever reprs from random import randint import re import pymongo +import bson.son from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, DuplicateItemError from xmodule.modulestore.locator import BlockUsageLocator -from xmodule.modulestore.mongo import draft from xmodule.modulestore import Location import urllib @@ -91,9 +91,10 @@ class LocMapperStore(object): else: course_id = "{0.org}.{0.course}".format(course_location) # very like _interpret_location_id but w/o the _id - location_id = {'org': course_location.org, 'course': course_location.course} - if course_location.category == 'course': - location_id['name'] = course_location.name + location_id = self._construct_location_son( + course_location.org, course_location.course, + course_location.name if course_location.category == 'course' else None + ) self.location_map.insert({ '_id': location_id, @@ -128,20 +129,25 @@ class LocMapperStore(object): """ location_id = self._interpret_location_course_id(old_style_course_id, location) - maps = self.location_map.find(location_id).sort('_id.name', pymongo.ASCENDING) - if maps.count() == 0: + maps = self.location_map.find(location_id) + maps = list(maps) + if len(maps) == 0: if add_entry_if_missing: # create a new map - course_location = location.replace(category='course', name=location_id['_id.name']) + course_location = location.replace(category='course', name=location_id['_id']['name']) self.create_map_entry(course_location) entry = self.location_map.find_one(location_id) else: raise ItemNotFoundError() - elif maps.count() > 1: - # if more than one, prefer the one w/o a name if that exists. Otherwise, choose the first (alphabetically) + elif len(maps) == 1: entry = maps[0] else: + # find entry w/o name, if any; otherwise, pick arbitrary entry = maps[0] + for item in maps: + if 'name' not in item['_id']: + entry = item + break if published: branch = entry['prod_branch'] @@ -242,11 +248,11 @@ class LocMapperStore(object): location_id = self._interpret_location_course_id(old_course_id, location) maps = self.location_map.find(location_id) - if maps.count() == 0: - raise ItemNotFoundError() - # turn maps from cursor to list map_list = list(maps) + if len(map_list) == 0: + raise ItemNotFoundError() + encoded_location_name = self._encode_for_mongo(location.name) # check whether there's already a usage_id for this location (and it agrees w/ any passed in or found) for map_entry in map_list: @@ -279,7 +285,10 @@ class LocMapperStore(object): ) map_entry['block_map'].setdefault(encoded_location_name, {})[location.category] = computed_usage_id - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) return computed_usage_id @@ -317,7 +326,10 @@ class LocMapperStore(object): if location.category in map_entry['block_map'].setdefault(encoded_location_name, {}): map_entry['block_map'][encoded_location_name][location.category] = usage_id - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) return usage_id @@ -338,7 +350,10 @@ class LocMapperStore(object): del map_entry['block_map'][encoded_location_name] else: del map_entry['block_map'][encoded_location_name][location.category] - self.location_map.update({'_id': map_entry['_id']}, {'$set': {'block_map': map_entry['block_map']}}) + self.location_map.update( + {'_id': self._construct_location_son(**map_entry['_id'])}, + {'$set': {'block_map': map_entry['block_map']}} + ) def _add_to_block_map(self, location, location_id, block_map): '''add the given location to the block_map and persist it''' @@ -357,7 +372,7 @@ class LocMapperStore(object): def _interpret_location_course_id(self, course_id, location): """ - Take the old style course id (org/course/run) and return a dict for querying the mapping table. + Take the old style course id (org/course/run) and return a dict w/ a SON for querying the mapping table. If the course_id is empty, it uses location, but this may result in an inadequate id. :param course_id: old style 'org/course/run' id from Location.course_id where Location.category = 'course' @@ -367,12 +382,21 @@ class LocMapperStore(object): if course_id: # re doesn't allow ?P<_id.org> and ilk matched = re.match(r'([^/]+)/([^/]+)/([^/]+)', course_id) - return dict(zip(['_id.org', '_id.course', '_id.name'], matched.groups())) + return {'_id': self._construct_location_son(*matched.groups())} - location_id = {'_id.org': location.org, '_id.course': location.course} if location.category == 'course': - location_id['_id.name'] = location.name - return location_id + return {'_id': self._construct_location_son(location.org, location.course, location.name)} + else: + return bson.son.SON([('_id.org', location.org), ('_id.course', location.course)]) + + def _construct_location_son(self, org, course, name=None): + """ + Construct the SON needed to repr the location for either a query or an insertion + """ + if name: + return bson.son.SON([('org', org), ('course', course), ('name', name)]) + else: + return bson.son.SON([('org', org), ('course', course)]) def _block_id_is_guid(self, name): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 2fbff10423..f550526493 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -36,8 +36,9 @@ class TestLocationMapper(unittest.TestCase): org = 'foo_org' course = 'bar_course' loc_mapper().create_map_entry(Location('i4x', org, course, 'course', 'baz_run')) + # pylint: disable=protected-access entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course, 'name': 'baz_run'} + '_id': loc_mapper()._construct_location_son(org, course, 'baz_run') }) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], '{}.{}.baz_run'.format(org, course)) @@ -48,8 +49,9 @@ class TestLocationMapper(unittest.TestCase): # ensure create_entry does the right thing when not given a course (creates org/course # rather than org/course/run course_id) loc_mapper().create_map_entry(Location('i4x', org, course, 'vertical', 'baz_vert')) + # find the one which has no name entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course} + '_id' : loc_mapper()._construct_location_son(org, course, None) }) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], '{}.{}'.format(org, course)) @@ -63,9 +65,7 @@ class TestLocationMapper(unittest.TestCase): 'wip', 'live', block_map) - entry = loc_mapper().location_map.find_one({ - '_id': {'org': org, 'course': course} - }) + entry = loc_mapper().location_map.find_one({'_id.org': org, '_id.course': course}) self.assertIsNotNone(entry, "Didn't find entry") self.assertEqual(entry['course_id'], 'foo_org.geek_dept.quux_course.baz_run') self.assertEqual(entry['draft_branch'], 'wip') diff --git a/lms/djangoapps/courseware/roles.py b/lms/djangoapps/courseware/roles.py index 6cb913a521..7d8f56ad00 100644 --- a/lms/djangoapps/courseware/roles.py +++ b/lms/djangoapps/courseware/roles.py @@ -137,6 +137,11 @@ class CourseRole(GroupBasedRole): A named role in a particular course """ def __init__(self, role, location, course_context=None): + """ + Location may be either a Location, a string, dict, or tuple which Location will accept + in its constructor, or a CourseLocator. Handle all these giving some preference to + the preferred naming. + """ # TODO: figure out how to make the group name generation lazy so it doesn't force the # loc mapping? if not hasattr(location, 'course_id'): @@ -153,14 +158,14 @@ class CourseRole(GroupBasedRole): # pylint: disable=no-member if isinstance(location, Location): - # least preferred legacy role_course format - groupnames.append('{0}_{1}'.format(role, location.course)) try: locator = loc_mapper().translate_location(location.course_id, location, False, False) groupnames.append('{0}_{1}'.format(role, locator.course_id)) except (InvalidLocationError, ItemNotFoundError): # if it's never been mapped, the auth won't be via the Locator syntax pass + # least preferred legacy role_course format + groupnames.append('{0}_{1}'.format(role, location.course)) elif isinstance(location, CourseLocator): # handle old Location syntax old_location = loc_mapper().translate_locator_to_location(location, get_course=True) diff --git a/mongo_indexes.md b/mongo_indexes.md new file mode 100644 index 0000000000..5133674737 --- /dev/null +++ b/mongo_indexes.md @@ -0,0 +1,21 @@ +These are the indexes each mongo db should have in order to perform well. +Each section states the collection name and then the indexes. To create an index, +you'll typically either use the mongohq type web interface or a standard terminal console. +If a terminal, this assumes you've logged in and gotten to the mongo prompt +``` +mongo mydatabasename +``` + +If using the terminal, to add an index to a collection, you'll need to prefix ```ensureIndex``` with +``` +db.collection_name +``` +as in ```db.location_map.ensureIndex({'course_id': 1}{background: true})``` + +location_map: +============= + +``` +ensureIndex({​'_id.org': 1, '_id.course': 1}) +ensureIndex({​'course_id': 1}) +```