Transactions logic upgrade #10014

symbolist · 2015-10-02T12:56:29Z

https://openedx.atlassian.net/browse/TNL-3428

As part of the Django upgrade, we need to move to the transaction framework that was introduced in Django 1.7.

https://docs.djangoproject.com/en/1.8/topics/db/transactions/

This Stackoverflow answer explains the differences of atomicity and durability between the two APIs: http://stackoverflow.com/a/24108491

The different cases I came across and how they are being handled is as follows:

The most important constraint to keep in mind is that we cannot have commit() or rollback() calls inside an atomic block. This means if a function has either of these calls any view which can call it cannot be wrapped in atomic.

Most of the views did not have any custom transaction management. We do not need to do anything besides enabling the new API for wrapping the views.
Some of the functions which were calling commit() or rollback() can just use the atomic context manager. This does change the behaviour sometimes because previously a commit would happen when commit() was called but if the atomic context manager is nested inside another, the commit would happen at the end of the outer atomic. I think in some cases this a reasonable thing to do. student/view.py:confirm_email_change is one such case.
In 3 cases we are doing a commit() if an IntegrityError happens followed by a database read. In all these cases this was quite deep in the call stack and those particular functions can be called from multiple places. I cannot think of a good way to retain this behaviour and have removed it for now.
In grades.py we do commit calls frequently to lesson the chance of overlap between multiple concurrent requests and hence IntegrityErrors. Similarly in the instructor_task app, we commit after creating InstructorTask objects and before sending off the task to celery. In these cases, I think the right approach would be to not have these views wrapped in atomic and instead have a context manager which behaves similarly to the current commit_on_success() which can wrap the views and any functions in the call graph. The goal is that transaction managements in these views should continue to behave like it does now.
A number of other functions in the code base are decorated with commit_on_success(). But since these are called from multiple views, for now these have been wrapped in atomic(). However, this atomic is nested inside the atomic which wraps the calling view and so the commits would happen once the view returns instead of when these functions returns as it does now. The only alternative is use the solution for 4 in a lot of places and I have opted to avoid this if I think we can.

macdiesel · 2015-10-05T20:11:00Z

@symbolist is this ready for review?

symbolist · 2015-10-06T15:17:01Z

@macdiesel Not yet.

symbolist · 2015-10-07T12:25:50Z

common/djangoapps/student/management/commands/transfer_students.py

@@ -52,7 +52,6 @@ class Command(TrackedCommand):
                    help="If True, try to transfer certificate items to the new course.")
    )

-    @transaction.commit_manually


No need for this.

Nit: Maybe just change this to @transaction.atomic and remove the commit_on_success context mgr below?

Actually I have already replaced commit_on_success with atomic below. The reason for keeping it inside the loop is that change for each student should get committed as it happens. Otherwise an error would revert all of the work. Or do you think we should be doing that?

@symbolist @doctoryes since the transaction isn't in a try block. Does that mean that if command fails on a user part way through the transaction the database would end up in an inconsistent state as there is nothing to catch the exception? You would end up with all the students to that point transferred but the remainder not transferred.

Also would this end up failing on the rerun as the student has already been transferred to the new course?

We have two routes of action here:

add the try/catch and migrate students with no errors and report the ones that were not moved
or

fail the entire transaction and keep all students in a consistent state.

I think that #2 is better as it would then allow us to troubleshoot and fix the issue without any inconsistencies in the data.

@macdiesel "if command fails on a user part way through the transaction" atomic() will 1. catch the exception, 2. rollback the transaction and 3. re-raise the exception. And yes, that means some of the students would have been transferred and the others not. But this is how it currently works too.

Actually, in Django 1.4 commit_manually is only used to disable automatic transaction management for views. Commands do not need it. So its not doing anything right now either.
https://docs.djangoproject.com/en/1.4/topics/db/transactions/#django.db.transaction.commit_manually

@doctoryes I created a branch, reverted @nedbat's commit and then created this PR against it. So all the migration is visible in this PR. I thought it would give us a picture of all of the transaction related logic in one place.

I do not know how this all plays out in the large scheme of how students and courses are handled in edx. I realize we would be changing behavior of this command, but I would rather see us keep the state of our data consistent.

Let me pose this question what will be easier to track down in the future:

a command that fails to move all the students and provides a clear and concise error message
or

a command that succeeds, maybe puts something to stdout or stderr that someone will read or maybe not, then later when someone realizes that not all the students are moved they call in support or devops. Then someone has to comb through the data looking for said students, then they have to look to the code to try and figure out what happened.

I just want to see us strive for both consistency in both our data and program behavior.

/soapbox

Hey - let me on that soapbox! 😄

Totally agree that 1) is better code/procedure. Just worried about the "hey - this command changed - rollback the release!" scenario. If we're willing to take on some of that risk (maybe release notes would help?), let's do it.

I have added a transaction.atomic on the command itself. This is a rarely used command so it shouldn't be an issue.

symbolist · 2015-10-07T12:56:45Z

lms/djangoapps/instructor_task/subtasks.py

        dog_stats_api.increment('instructor_task.subtask.update_exception')
        raise
-    else:
-        TASK_LOG.debug("about to commit....")
-        transaction.commit()


The atomic context manager takes care of the commit/rollback.

doctoryes · 2015-10-07T21:37:59Z

lms/djangoapps/instructor_task/subtasks.py

@@ -494,7 +494,6 @@ def update_subtask_status(entry_id, current_task_id, new_subtask_status, retry_c
        _release_subtask_lock(current_task_id)


-@transaction.commit_manually


Why not just use the @transaction.atomic decorator here and avoid the reformatting below?

BTW, this change from the deprecated commit_manually to atomic will fix a few bulk email False is not true failures.

Yes, I'll do that.

Updated below.

doctoryes · 2015-10-08T19:35:55Z

PHEW! I've finished my first pass and left comments. Some points:

I agree that the added commit_on_success decorator will mirror the current Django 1.4 behavior.
I'd prefer to make all transactions conform to Django1.8-style savepoints and the like. But it's unclear how much work that would be - likely significant.
We'll still be able to identify the places where we've done this - and fix them in the FUUUUTUUURE (yes - a future that may never exist) if we choose to do so.
More testing, demonstrating expected behavior in different nesting situations, is needed.

symbolist · 2015-10-13T17:52:13Z

lms/djangoapps/ccx/overrides.py

-            transaction.commit()
-            kwargs = {'ccx': ccx, 'location': block.location, 'field': name}
-            override = CcxFieldOverride.objects.get(**kwargs)
+        else:


Instead of trying to create an object first and do a get on an IntegrityError, lets just do a get first.

@symbolist , would this increase the likelihood of race conditions?

@adampalay Yes, to an extent. But this function is only called from 3 Instructor dashboard CCX tab views so I doubt we will be seeing many parallel requests causing IntegrityErrors. If we do we can refactor the views a bit so that we can put outer_atomic around this whole function or maybe shift to READ COMMITTED for the views which do create IntegrityErrors.

I have put in a ticket for this: https://openedx.atlassian.net/browse/TNL-3614

symbolist · 2015-10-14T11:41:43Z

lms/djangoapps/verify_student/models.py

-            transaction.commit()
-            return cls.objects.get(course_id=course_id, checkpoint_location=checkpoint_location)
+        checkpoint, __ = cls.objects.get_or_create(course_id=course_id, checkpoint_location=checkpoint_location)
+        return checkpoint


I have changed the view below to use read_committed so we shouldn't need this anymore.

symbolist · 2015-10-14T13:48:05Z

@doctoryes @macdiesel I have addressed all comments. Besides that, I have:

Made tweaks in a number of places.
Reduced the usages of commit_on_success to just the progress page views in courseware.
Fixed and added tests for commit_on_success and outer_atomic.

This is ready for another review.

symbolist · 2015-10-14T13:51:54Z

@adampalay Here is the PR I mentioned today. If you would like to discuss any of the changes, please ping me.

doctoryes · 2015-10-14T14:48:48Z

Nit: There's no docstring or comments explaining why the outer_atomic() decorator exists and the pattern that developers are expected to follow for transactions in Django 1.8 post-upgrade. Such comments would be useful - perhaps they could be added to tests showing how outer_atomic works and demonstrating the recommended pattern.

doctoryes · 2015-10-14T14:55:45Z

In other words, it should be clear what's deprecated.

doctoryes · 2015-10-14T14:59:32Z

Also, do you plan to create a ticket for commit_on_success removal for the progress page methods in lms/djangoapps/courseware/{grades|views}.py? What blocks those from being removed?

doctoryes · 2015-10-14T15:00:11Z

A few nits and follow-ons - but no blockers for merging: 👍

symbolist · 2015-10-15T12:38:42Z

common/djangoapps/util/db.py

+
+    Arguments:
+        using (str): the name of the database.
+        read_committed (bool): Whether to use read committed isolation level.


@doctoryes I have added more detailed documentation to outer_atomic().

symbolist · 2015-10-15T12:50:19Z

@doctoryes I have added the tests you suggested as well.

Yes, I have created a ticket for replacing the remaining instances of commit_on_success with atomic (https://openedx.atlassian.net/browse/TNL-3607). I haven't looked at the grade functions in courseware very closely yet. I don't see why it shouldn't be possible though we may need to refactor things a bit. But I would like to wait until we can get a sandbox hooked up to prod data before attempting that so that we can test with real data that everything works fine after the migration.

adampalay · 2015-10-15T13:03:32Z

common/djangoapps/util/db.py

+                    try:
+                        connection.rollback()
+                    except Error:
+                        # An error during rollback means that something


will we want to know if the rollback didn't work?

I'll put in some logging statements here. Though ideally we want to get rid of this commit_on_success decorator before we deploy the upgrade. Right now it is only being used in courseware/grades.py and on the progress page view. I have created a separate story for it.

symbolist · 2015-10-16T16:26:11Z

@doctoryes @adampalay I have also eliminated the rest of the instances of commit_on_success in lms/djangoapps/courseware/{grades|views}.py and refactored the code to use outer_atomic. You can see the changes in the last two commits.

A bit of context: The reason for adding commit_on_success to the progress page view and the helper functions for grading was that because calculating the grade for a course can take a long time – 10-15 seconds is normal for large courses – student's often click the progress page link multiple times because it does not look like it is responding and this creates a strong chance of two parallel requests which try to do get_or_create as they try to initialize xblocks and save data for them. Adding commit_on_success in these functions divided up the work for calculating the grade into multiple smaller transactions which had a much lower chance of collision. When migrating to the new API I have kept this in mind and used outer_atomic to divide this work into a number of smaller transaction.

symbolist · 2015-10-20T16:32:50Z

@doctoryes Can you review the changes in the last two commits?

macdiesel · 2015-10-27T12:28:14Z

👍 😎

symbolist · 2015-10-27T13:12:17Z

Closing and creating a new PR against the upgrade branch.

symbolist force-pushed the usman/transactions-update branch from 9029c9a to 3af138c Compare October 2, 2015 13:13

symbolist changed the title ~~New~~ Transactions logic upgrade Oct 2, 2015

symbolist force-pushed the usman/transactions-undo branch from 535f7c8 to 52a7f11 Compare October 6, 2015 15:22

symbolist force-pushed the usman/transactions-update branch 2 times, most recently from aedb2cc to 0210770 Compare October 7, 2015 12:20

symbolist reviewed Oct 7, 2015
View reviewed changes

symbolist mentioned this pull request Oct 7, 2015

module object has no attributec commit_manually #9880

Closed

symbolist force-pushed the usman/transactions-update branch from 0210770 to 9ec35d3 Compare October 7, 2015 12:51

symbolist reviewed Oct 7, 2015
View reviewed changes

symbolist force-pushed the usman/transactions-undo branch from 52a7f11 to cea828d Compare October 7, 2015 13:24

symbolist force-pushed the usman/transactions-update branch 4 times, most recently from 46f58a7 to 65a6e00 Compare October 7, 2015 16:29

doctoryes reviewed Oct 7, 2015
View reviewed changes

symbolist force-pushed the usman/transactions-update branch from a8f472b to d0756f9 Compare October 13, 2015 15:09

symbolist reviewed Oct 13, 2015
View reviewed changes

symbolist force-pushed the usman/transactions-update branch from e3c8b44 to f81e7b0 Compare October 13, 2015 18:13

symbolist reviewed Oct 14, 2015
View reviewed changes

Move to Django's new transaction API.

82f5ab3

symbolist force-pushed the usman/transactions-update branch from 2c5d4ba to 055f3b1 Compare October 15, 2015 12:36

symbolist reviewed Oct 15, 2015
View reviewed changes

adampalay reviewed Oct 15, 2015
View reviewed changes

symbolist force-pushed the usman/transactions-update branch from 055f3b1 to 5da08bf Compare October 16, 2015 12:35

symbolist added 2 commits October 16, 2015 17:50

Post-review feedback.

96d340c

Move grade functions in courseware to use atomic.

4e5d812

symbolist force-pushed the usman/transactions-update branch from 5da08bf to 4e5d812 Compare October 16, 2015 12:50

doctoryes mentioned this pull request Oct 26, 2015

Manually set transaction to rollback upon error. #10342

Closed

symbolist closed this Oct 27, 2015

symbolist mentioned this pull request Oct 27, 2015

Transactions update post-review changes #10375

Merged

anonim-01 mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities anonim-01/open-okul-lms#9

Open

sumodgeorge mentioned this pull request May 14, 2024

[Snyk] Fix for 2 vulnerabilities sumodgeorge/edx-platform#33

Open

		@@ -494,7 +494,6 @@ def update_subtask_status(entry_id, current_task_id, new_subtask_status, retry_c
		_release_subtask_lock(current_task_id)


		@transaction.commit_manually

Transactions logic upgrade #10014

Transactions logic upgrade #10014

Conversation

symbolist commented Oct 2, 2015

macdiesel commented Oct 5, 2015

symbolist commented Oct 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doctoryes commented Oct 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symbolist commented Oct 14, 2015

symbolist commented Oct 14, 2015

doctoryes commented Oct 14, 2015

doctoryes commented Oct 14, 2015

doctoryes commented Oct 14, 2015

doctoryes commented Oct 14, 2015

Choose a reason for hiding this comment

symbolist commented Oct 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symbolist commented Oct 16, 2015

symbolist commented Oct 20, 2015

macdiesel commented Oct 27, 2015

symbolist commented Oct 27, 2015