{"action":"created","issue":{"url":"https://api.github.com/repos/edx/edx-platform/issues/10014","labels_url":"https://api.github.com/repos/edx/edx-platform/issues/10014/labels{/name}","comments_url":"https://api.github.com/repos/edx/edx-platform/issues/10014/comments","events_url":"https://api.github.com/repos/edx/edx-platform/issues/10014/events","html_url":"https://github.com/edx/edx-platform/pull/10014","id":109486237,"number":10014,"title":"Transactions logic upgrade","user":{"login":"symbolist","id":5305552,"avatar_url":"https://avatars.githubusercontent.com/u/5305552?v=3","gravatar_id":"","url":"https://api.github.com/users/symbolist","html_url":"https://github.com/symbolist","followers_url":"https://api.github.com/users/symbolist/followers","following_url":"https://api.github.com/users/symbolist/following{/other_user}","gists_url":"https://api.github.com/users/symbolist/gists{/gist_id}","starred_url":"https://api.github.com/users/symbolist/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/symbolist/subscriptions","organizations_url":"https://api.github.com/users/symbolist/orgs","repos_url":"https://api.github.com/users/symbolist/repos","events_url":"https://api.github.com/users/symbolist/events{/privacy}","received_events_url":"https://api.github.com/users/symbolist/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":14,"created_at":"2015-10-02T12:56:29Z","updated_at":"2015-10-27T13:12:17Z","closed_at":"2015-10-27T13:12:17Z","pull_request":{"url":"https://api.github.com/repos/edx/edx-platform/pulls/10014","html_url":"https://github.com/edx/edx-platform/pull/10014","diff_url":"https://github.com/edx/edx-platform/pull/10014.diff","patch_url":"https://github.com/edx/edx-platform/pull/10014.patch"},"body":"https://openedx.atlassian.net/browse/TNL-3428\r\n\r\nAs part of the Django upgrade, we need to move to the transaction framework that was introduced in Django 1.7.\r\n\r\nhttps://docs.djangoproject.com/en/1.8/topics/db/transactions/\r\n\r\nThis Stackoverflow answer explains the differences of atomicity and durability between the two APIs: http://stackoverflow.com/a/24108491\r\n\r\nThe different cases I came across and how they are being handled is as follows:\r\n\r\nThe 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.\r\n\r\n1. 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.\r\n2. 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.\r\n3. 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.\r\n4. 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.\r\n5. 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."},"comment":{"url":"https://api.github.com/repos/edx/edx-platform/issues/comments/151490258","html_url":"https://github.com/edx/edx-platform/pull/10014#issuecomment-151490258","issue_url":"https://api.github.com/repos/edx/edx-platform/issues/10014","id":151490258,"user":{"login":"symbolist","id":5305552,"avatar_url":"https://avatars.githubusercontent.com/u/5305552?v=3","gravatar_id":"","url":"https://api.github.com/users/symbolist","html_url":"https://github.com/symbolist","followers_url":"https://api.github.com/users/symbolist/followers","following_url":"https://api.github.com/users/symbolist/following{/other_user}","gists_url":"https://api.github.com/users/symbolist/gists{/gist_id}","starred_url":"https://api.github.com/users/symbolist/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/symbolist/subscriptions","organizations_url":"https://api.github.com/users/symbolist/orgs","repos_url":"https://api.github.com/users/symbolist/repos","events_url":"https://api.github.com/users/symbolist/events{/privacy}","received_events_url":"https://api.github.com/users/symbolist/received_events","type":"User","site_admin":false},"created_at":"2015-10-27T13:12:17Z","updated_at":"2015-10-27T13:12:17Z","body":"Closing and creating a new PR against the upgrade branch."}}