From fd71afe94f67b80b455832bdbabd7eb6b1056e05 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Tue, 13 Sep 2016 14:53:09 -0400 Subject: [PATCH] Make Robust Grades Robust Again As part of the Robust Grades rollout, we expect to see some DatabaseErrors in various methods that write to the database. Since the success of this write operation is not needed for the end-user, we just log and swallow the error. In the future, we'll also enqueue an async task to finish the write operation that failed. --- .../tests/test_field_override_performance.py | 54 +++++++++---------- lms/djangoapps/courseware/tests/test_views.py | 6 +-- lms/djangoapps/grades/new/subsection_grade.py | 28 ++++++++-- lms/djangoapps/grades/tests/test_new.py | 27 +++++++++- .../tests/test_tasks_helper.py | 2 +- 5 files changed, 81 insertions(+), 36 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index d06db8fd6f..faf2bfa01d 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -229,18 +229,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (20, 6), - ('no_overrides', 2, True, False): (20, 6), - ('no_overrides', 3, True, False): (20, 6), - ('ccx', 1, True, False): (20, 6), - ('ccx', 2, True, False): (20, 6), - ('ccx', 3, True, False): (20, 6), - ('no_overrides', 1, False, False): (20, 6), - ('no_overrides', 2, False, False): (20, 6), - ('no_overrides', 3, False, False): (20, 6), - ('ccx', 1, False, False): (20, 6), - ('ccx', 2, False, False): (20, 6), - ('ccx', 3, False, False): (20, 6), + ('no_overrides', 1, True, False): (22, 6), + ('no_overrides', 2, True, False): (22, 6), + ('no_overrides', 3, True, False): (22, 6), + ('ccx', 1, True, False): (22, 6), + ('ccx', 2, True, False): (22, 6), + ('ccx', 3, True, False): (22, 6), + ('no_overrides', 1, False, False): (22, 6), + ('no_overrides', 2, False, False): (22, 6), + ('no_overrides', 3, False, False): (22, 6), + ('ccx', 1, False, False): (22, 6), + ('ccx', 2, False, False): (22, 6), + ('ccx', 3, False, False): (22, 6), } @@ -252,19 +252,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (20, 3), - ('no_overrides', 2, True, False): (20, 3), - ('no_overrides', 3, True, False): (20, 3), - ('ccx', 1, True, False): (20, 3), - ('ccx', 2, True, False): (20, 3), - ('ccx', 3, True, False): (20, 3), - ('ccx', 1, True, True): (21, 3), - ('ccx', 2, True, True): (21, 3), - ('ccx', 3, True, True): (21, 3), - ('no_overrides', 1, False, False): (20, 3), - ('no_overrides', 2, False, False): (20, 3), - ('no_overrides', 3, False, False): (20, 3), - ('ccx', 1, False, False): (20, 3), - ('ccx', 2, False, False): (20, 3), - ('ccx', 3, False, False): (20, 3), + ('no_overrides', 1, True, False): (22, 3), + ('no_overrides', 2, True, False): (22, 3), + ('no_overrides', 3, True, False): (22, 3), + ('ccx', 1, True, False): (22, 3), + ('ccx', 2, True, False): (22, 3), + ('ccx', 3, True, False): (22, 3), + ('ccx', 1, True, True): (23, 3), + ('ccx', 2, True, True): (23, 3), + ('ccx', 3, True, True): (23, 3), + ('no_overrides', 1, False, False): (22, 3), + ('no_overrides', 2, False, False): (22, 3), + ('no_overrides', 3, False, False): (22, 3), + ('ccx', 1, False, False): (22, 3), + ('ccx', 2, False, False): (22, 3), + ('ccx', 3, False, False): (22, 3), } diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index a73ae740c0..08e1d0ce91 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1345,17 +1345,17 @@ class ProgressPageTests(ModuleStoreTestCase): """Test that query counts remain the same for self-paced and instructor-paced courses.""" SelfPacedConfiguration(enabled=self_paced_enabled).save() self.setup_course(self_paced=self_paced) - with self.assertNumQueries(37), check_mongo_calls(4): + with self.assertNumQueries(39), check_mongo_calls(4): self._get_progress_page() def test_progress_queries(self): self.setup_course() - with self.assertNumQueries(37), check_mongo_calls(4): + with self.assertNumQueries(39), check_mongo_calls(4): self._get_progress_page() # subsequent accesses to the progress page require fewer queries. for _ in range(2): - with self.assertNumQueries(20), check_mongo_calls(4): + with self.assertNumQueries(22), check_mongo_calls(4): self._get_progress_page() @patch( diff --git a/lms/djangoapps/grades/new/subsection_grade.py b/lms/djangoapps/grades/new/subsection_grade.py index 5b8ea8761a..b9c6e140aa 100644 --- a/lms/djangoapps/grades/new/subsection_grade.py +++ b/lms/djangoapps/grades/new/subsection_grade.py @@ -2,6 +2,9 @@ SubsectionGrade Class """ from collections import OrderedDict +from contextlib import contextmanager +from django.db import transaction +from django.db.utils import DatabaseError from lazy import lazy from logging import getLogger from courseware.model_data import ScoresClient @@ -10,6 +13,7 @@ from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade from lms.djangoapps.grades.config.models import PersistentGradesEnabledFlag from student.models import anonymous_id_for_user, User from submissions import api as submissions_api +from traceback import format_exc from xmodule import block_metadata_utils, graders from xmodule.graders import Score @@ -17,6 +21,20 @@ from xmodule.graders import Score log = getLogger(__name__) +@contextmanager +def persistence_safe_fallback(): + """ + A context manager intended to be used to wrap robust grades database-specific code that can be ignored on failure. + """ + try: + with transaction.atomic(): + yield + except DatabaseError: + # Error deliberately logged, then swallowed. It's assumed that the wrapped code is not guaranteed to succeed. + # TODO: enqueue a celery task to finish the task asynchronously, see TNL-5471 + log.warning("Persistent Grades: Persistence Error, falling back.\n{}".format(format_exc())) + + class SubsectionGrade(object): """ Class for Subsection Grades. @@ -254,8 +272,9 @@ class SubsectionGradeFactory(object): if read_only: self._unsaved_subsection_grades.append(subsection_grade) else: - grade_model = subsection_grade.create_model(self.student) - self._update_saved_subsection_grade(subsection.location, grade_model) + with persistence_safe_fallback(): + grade_model = subsection_grade.create_model(self.student) + self._update_saved_subsection_grade(subsection.location, grade_model) return subsection_grade def bulk_create_unsaved(self): @@ -264,8 +283,9 @@ class SubsectionGradeFactory(object): """ self._log_event(log.warning, u"bulk_create_unsaved") - SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id) - self._unsaved_subsection_grades = [] + with persistence_safe_fallback(): + SubsectionGrade.bulk_create_models(self.student, self._unsaved_subsection_grades, self.course.id) + self._unsaved_subsection_grades = [] def update(self, subsection, block_structure=None): """ diff --git a/lms/djangoapps/grades/tests/test_new.py b/lms/djangoapps/grades/tests/test_new.py index 777f4f37dc..eaa8fe7bda 100644 --- a/lms/djangoapps/grades/tests/test_new.py +++ b/lms/djangoapps/grades/tests/test_new.py @@ -4,6 +4,7 @@ Test saved subsection grade functionality. import ddt from django.conf import settings +from django.db.utils import DatabaseError from mock import patch from capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory @@ -117,7 +118,7 @@ class SubsectionGradeFactoryTest(GradeTestBase): 'lms.djangoapps.grades.new.subsection_grade.SubsectionGradeFactory._get_saved_grade', wraps=self.subsection_grade_factory._get_saved_grade # pylint: disable=protected-access ) as mock_get_saved_grade: - with self.assertNumQueries(12): + with self.assertNumQueries(14): grade_a = self.subsection_grade_factory.create(self.sequence) self.assertTrue(mock_get_saved_grade.called) self.assertTrue(mock_create_grade.called) @@ -133,6 +134,30 @@ class SubsectionGradeFactoryTest(GradeTestBase): self.assertEqual(grade_a.url_name, grade_b.url_name) self.assertEqual(grade_a.all_total, grade_b.all_total) + @ddt.data( + ( + 'lms.djangoapps.grades.new.subsection_grade.SubsectionGrade.create_model', + lambda self: self.subsection_grade_factory.create(self.sequence) + ), + ( + 'lms.djangoapps.grades.new.subsection_grade.SubsectionGrade.bulk_create_models', + lambda self: self.subsection_grade_factory.bulk_create_unsaved() + ), + ) + @ddt.unpack + def test_fallback_handling(self, underlying_method, method_to_test): + """ + Tests that the persistent grades fallback handler functions as expected. + """ + with patch('lms.djangoapps.grades.new.subsection_grade.log') as log_mock: + with patch(underlying_method) as underlying: + underlying.side_effect = DatabaseError("I'm afraid I can't do that") + method_to_test(self) + # By making it this far, we implicitly assert "the factory method swallowed the exception correctly" + self.assertTrue( + log_mock.warning.call_args_list[0].startswith("Persistent Grades: Persistence Error, falling back.") + ) + @patch.dict(settings.FEATURES, {'PERSISTENT_GRADES_ENABLED_FOR_ALL_TESTS': False}) @ddt.data( (True, True), diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 21f693d246..94be388f4b 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1670,7 +1670,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): 'failed': 3, 'skipped': 2 } - with self.assertNumQueries(175): + with self.assertNumQueries(191): self.assertCertificatesGenerated(task_input, expected_results) expected_results = {