1159220 - Remove dependency on SQLAlchemy

Status: RESOLVED WONTFIX

(Marketplace Graveyard :: Code Quality, enhancement, P4)

Description

[Mathieu Pillard [:mat]]

django-mysql-pool is gone from zamboni with the django 1.7 upgrade.

SQLAlchemy is still here because services/utils.py needs it, because services/verify.py wants to connect to the database manually bypassing the django ORM.

We should consider whether bypassing the ORM is really worth it (might be premature optimization) but more importantly, we should try to use the django connection API instead of manually connecting with SQLAlchemy, there is no point in having a separate dependency just to connect to the database.

Comment 1

[Mathieu Pillard [:mat]]

https://github.com/diox/zamboni/commit/6af1fecb4088a300e29d9779d6f3e607d60b8c69 should work, but how can I test that I'm not introducing some perf issues with the verifier ?

Comment 2

[Wil Clouser [:clouserw]]

andym or kumar might have some ideas

Comment 3

[Kumar McMillan [:kumar]]

IIRC this was done as actual optimization to the AMO version check service (when it was serving 10,000 requests/sec). But that got ported over to the Marketplace receipt check which doesn't get traffic anywhere near that. 

So... I suppose we could move that service back to Django. That would be a lot of work though. I don't know how easy it would be to replace sqlalchemy with the Django ORM in that service because the service is not Django.

Comment 4

[Andy McKay]

+1 on moving it back in to zamboni and making it Django only. I've suggested this happen (sure I filed bugs on it, can't find them now), it will simplify so many things. SQLAlchemy was used because it was there and convenient, there's no reason you need to keep it around if you can do it a different wya.

Comment 5

[Andy McKay]

(In reply to Kumar McMillan [:kumar] (needinfo for quickness) from comment #3)
> So... I suppose we could move that service back to Django. That would be a
> lot of work though. I don't know how easy it would be to replace sqlalchemy
> with the Django ORM in that service because the service is not Django.

Its well tested, with a full unit test suite in Django, all tables are accessible from models in Django. Shouldn't be too tough, just a bit of grunt work.

Comment 6

[Mathieu Pillard [:mat]]

Thanks for the info. I've opened bug 1159746 to deal with migrating it fully to django because it's more work than just getting rid of sqlalchemy (which I'll take right now because it's an easy win).

Comment 7

[Mathieu Pillard [:mat]]

Fixed in https://github.com/mozilla/zamboni/commit/e4dcdc9842853fb71e7e05c9ceefa0b4e846289f

QA:
- Please verify that payment flow still works

Comment 8

[Mathieu Pillard [:mat]]

Follow up because of errors (2013, 'Lost connection to MySQL server during query') on stage after a while: https://github.com/mozilla/zamboni/commit/2fab0ae58685c4b95d5e53629c9dad9023126fba

Comment 9

[Mathieu Pillard [:mat]]

Those 2 commits were reverted in https://github.com/mozilla/zamboni/commit/6c97a47b3e8af6d9699b42687db0e3987e38fdae because dev & stage were still having errors.

Comment 10

[David Durst [:ddurst]]

Payments is trending to close...