1150466 - [elmo][a10n][master-ball][locale-inspector] update transaction handling to django to 1.6

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect)

Description

[Axel Hecht [:Pike]]

+++ This bug was initially created as a clone of Bug #1141023 +++

To update django to 1.6, we need to handle transactions new.

Sad news is that what I did back then was horrible, and probably showed both my complete lack of understanding and django's not-so-great behavior and api.

The intent back then was "get me a current state of the db" much more than "put these things into a transaction". Thus this is mostly removing the old handling, and adding back transactions for a limited set.

Sadly this is all over the place, I'll add comments for each module.

Comment 1

[Axel Hecht [:Pike]]

Attachment: locale-inspector: drop transactions, autocommit is fine

For locale-inspector, using autocommit is just fine to get the latest status of the db.

Comment 2

[Axel Hecht [:Pike]]

... the patch to a10n is boring, updating the requirements and the elmo reference, so I won't bore you with that, ...

Comment 3

[Axel Hecht [:Pike]]

Attachment: elmo: only use transactions for changes and sourcestamps, autocommit everywhere else

Change and SourceStamp are rather complex objects in the database, with changes having files and properties, and sourcestamps having changes. Thus wrapping those in transactions.

For everything else, autocommit should be just fine.

Comment 4

[Axel Hecht [:Pike]]

Attachment: elmo: use a transaction per push

More elmo: Push is also a complex db object, putting each of those in a transaction.

Comment 5

[Axel Hecht [:Pike]]

I published both repos with branches on https://github.com/Pike/locale-inspector/compare/django-1.6-transactions and https://github.com/mozilla/elmo/compare/develop...Pike:feature/bug-1141023-update-django-1.6, it's probably more useful to look at the result than the patch

Comment 6

[Axel Hecht [:Pike]]

Michael, I'm still trying to wrap my head around which branches to actually pull against, mostly because deploying 1.6 is a dead end security-update-wise, and I'm blocked on py2.7.

Are the patches here insightful to you?

More context, I've been contemplating to use ATOMIC, but given that parts of the django code run in long-running automation processes (weeks), that sounded like a bad idea. I think that autocommit is the right default for elmo.

I've been mulling over the patches here for a bit more, and I guess I could use an @transaction.atomic for a few of my richer POST views like in https://github.com/mozilla/elmo/blob/master/apps/shipping/views/__init__.py would be good to have.

Shouldn't block us from making progress on the existing work, though.

Comment 7

[Osmose [:osmose, :mkelly]]

(In reply to Axel Hecht [:Pike] from comment #6)
> Michael, I'm still trying to wrap my head around which branches to actually
> pull against, mostly because deploying 1.6 is a dead end
> security-update-wise, and I'm blocked on py2.7.
> 
> Are the patches here insightful to you?

I went through the larger queue mentioned in bug 1141023, which included these patches, and it all seemed to make sense to me. I left a few comments but everything was nitpicking.
 
> More context, I've been contemplating to use ATOMIC, but given that parts of
> the django code run in long-running automation processes (weeks), that
> sounded like a bad idea. I think that autocommit is the right default for
> elmo.
> 
> I've been mulling over the patches here for a bit more, and I guess I could
> use an @transaction.atomic for a few of my richer POST views like in
> https://github.com/mozilla/elmo/blob/master/apps/shipping/views/__init__.py
> would be good to have.
> 
> Shouldn't block us from making progress on the existing work, though.

I dunno what kind of traffic levels elmo gets, but if it's not too large and you're not concerned about being lightning fast, I don't see why you couldn't try enabling ATOMIC on your database and see if it affects the site's performance at all. If you don't have New Relic or something similar set up yet you probably want to try landing that first just to get some data to compare to.

Comment 8

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/5abd6ada303beb0221b171304dfa238cba996068
bug 1150466, transactions and other ORM fixes for buildbot interactions

Only use transactions for DB constructs that are complex, notably
changes (with files and properties) and sourcestamps (with changes, sic).

Everything else is fine with just autocommit, which we now have with
Django 1.6.

This is part of the bigger update work in bug 1141023.

Also, this patch is a bit more precise on when and if to save objects
after modifying manytomany relations. 'Cause then you don't have to.