From e3c66f55e0c0e9228d3d2623245a3d5d3b6fca61 Mon Sep 17 00:00:00 2001 From: Kevin Falcone Date: Wed, 26 Aug 2015 23:37:03 -0400 Subject: [PATCH] Optimize memory and CPU usage. The print_out_all_courses() routine consumes a ton of memory (2G and causes noticable mongo usage spikes). This actually causes other processes on production boxes to be memory starved and killed (such as worker children on edge when this was run recently). The behavior of this script on production is * Print several hundred courses * Ask if you want to delete the one you specified * print several hundred courses minus one On a sandbox with 5 courses, you could tell by eye that 1 is gone, but not in production (or even in stage). The original PLAT-619 ticket for this suggested printing a course listing on error, but instead it always printed the course listing. Even in the error case, hundreds of course ids is confusing and obscures the error message saying that your course_id is invalid. You should be getting the course id from the UI or from ./manage.py lms dump_course_ids, not by searching a list. Adjusted the test accordingly Remove get_courses_keys --- AUTHORS | 3 ++- .../management/commands/delete_course.py | 16 +--------------- .../commands/tests/test_delete_course.py | 19 ++++++++----------- .../lib/xmodule/xmodule/modulestore/mixed.py | 15 --------------- 4 files changed, 11 insertions(+), 42 deletions(-) diff --git a/AUTHORS b/AUTHORS index 5e22bf1b08..e2b7df5476 100644 --- a/AUTHORS +++ b/AUTHORS @@ -233,4 +233,5 @@ Dongwook Yoon Awais Qureshi Eric Fischer Brian Beggs -Bill DeRusha \ No newline at end of file +Bill DeRusha +Kevin Falcone diff --git a/cms/djangoapps/contentstore/management/commands/delete_course.py b/cms/djangoapps/contentstore/management/commands/delete_course.py index 16a6c88330..ec42635cc2 100644 --- a/cms/djangoapps/contentstore/management/commands/delete_course.py +++ b/cms/djangoapps/contentstore/management/commands/delete_course.py @@ -18,18 +18,6 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore -def print_out_all_courses(): - """ - Print out all the courses available in the course_key format so that - the user can correct any course_key mistakes - """ - courses = modulestore().get_courses_keys() - print 'Available courses:' - for course in courses: - print str(course) - print '' - - class Command(BaseCommand): """ Delete a MongoDB backed course @@ -58,8 +46,6 @@ class Command(BaseCommand): elif len(args) > 2: raise CommandError("Too many arguments! Expected ") - print_out_all_courses() - if not modulestore().get_course(course_key): raise CommandError("Course with '%s' key not found." % args[0]) @@ -67,4 +53,4 @@ class Command(BaseCommand): if query_yes_no("Deleting course {0}. Confirm?".format(course_key), default="no"): if query_yes_no("Are you sure. This action cannot be undone!", default="no"): delete_course_and_groups(course_key, ModuleStoreEnum.UserID.mgmt_command) - print_out_all_courses() + print "Deleted course {}".format(course_key) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py index 3cda8e29ea..d15698719d 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_delete_course.py @@ -5,6 +5,7 @@ Unittests for deleting a course in an chosen modulestore import unittest import mock +from opaque_keys.edx.locations import SlashSeparatedCourseKey from django.core.management import CommandError from contentstore.management.commands.delete_course import Command # pylint: disable=import-error from contentstore.tests.utils import CourseTestCase # pylint: disable=import-error @@ -94,27 +95,23 @@ class DeleteCourseTest(CourseTestCase): run=course_run ) - def test_courses_keys_listing(self): - """ - Test if the command lists out available course key courses - """ - courses = [str(key) for key in modulestore().get_courses_keys()] - self.assertIn("TestX/TS01/2015_Q1", courses) - def test_course_key_not_found(self): """ Test for when a non-existing course key is entered """ errstring = "Course with 'TestX/TS01/2015_Q7' key not found." with self.assertRaisesRegexp(CommandError, errstring): - self.command.handle("TestX/TS01/2015_Q7", "commit") + self.command.handle('TestX/TS01/2015_Q7', "commit") def test_course_deleted(self): """ Testing if the entered course was deleted """ + + #Test if the course that is about to be deleted exists + self.assertIsNotNone(modulestore().get_course(SlashSeparatedCourseKey("TestX", "TS01", "2015_Q1"))) + with mock.patch(self.YESNO_PATCH_LOCATION) as patched_yes_no: patched_yes_no.return_value = True - self.command.handle("TestX/TS01/2015_Q1", "commit") - courses = [unicode(key) for key in modulestore().get_courses_keys()] - self.assertNotIn("TestX/TS01/2015_Q1", courses) + self.command.handle('TestX/TS01/2015_Q1', "commit") + self.assertIsNone(modulestore().get_course(SlashSeparatedCourseKey("TestX", "TS01", "2015_Q1"))) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index a49c7dbd9a..65bb345e1d 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -280,21 +280,6 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): courses[course_id] = course return courses.values() - @strip_key - def get_courses_keys(self, **kwargs): - ''' - Returns a list containing the top level XModuleDescriptors keys of the courses in this modulestore. - ''' - courses = {} - for store in self.modulestores: - # filter out ones which were fetched from earlier stores but locations may not be == - for course in store.get_courses(**kwargs): - course_id = self._clean_locator_for_mapping(course.id) - if course_id not in courses: - # course is indeed unique. save it in result - courses[course_id] = course - return courses.keys() - @strip_key def get_libraries(self, **kwargs): """