Opened 17 years ago
Closed 17 years ago
#5106 closed (fixed)
django.db.backend.*.base refactoring
Reported by: | (removed) | Owned by: | Malcolm Tredinnick |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Design decision needed | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Splitting connection pooling up into two patches; this particular ticket holds refactoring of django.db.backend.*.base.DatabaseWrapper classes to remove redundancy, and give an easier spot to control connection pooling.
Very least, the misc backend implementation are a bit redundant- deriving from a common base class is a bit saner in this case for debugging and extension reasons.
Attachments (3)
Change History (37)
by , 17 years ago
Attachment: | backend-refactoring.patch added |
---|
comment:1 by , 17 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 17 years ago
via http://groups.google.com/group/django-developers/browse_thread/thread/ab7649e4abd4e589 , might suggest getting the first patch posted into vcs; the second is following and is far more intrusive; actual api changes (no more django.db.backend access primarily), but constricts access to backend capabilities/ops/client/introspection through the wrapper object, thus allowing for far easier/intrusive hooking/override- specifically to just layer connection pooling over a backend, rather then having to write a pooler specific to each backend.
follow-up: 5 comment:3 by , 17 years ago
If you mean that code won't have access to django.db.backend any longer (or shouldn't be accessing it), you will want to start a design discussion on django-dev first. That's a very intrusive change and will break a lot of code, so we need to discuss the change in abstract first before wading through lots of lines of code.
comment:4 by , 17 years ago
Owner: | changed from | to
---|
Oops... last comment was me, by the way.
I'm generally +1 on the current patch, since it looks clean. Still want to look at it once or twice more, but it'll go in soon, I would expect.
by , 17 years ago
Attachment: | backend-refactoring-v2.patch added |
---|
version/stage 2 of the refactoring; api breaking, but molds existing old style layout into new automatically, and allows for actual OOP trickery.
comment:5 by , 17 years ago
Replying to mtredinnick:
If you mean that code won't have access to django.db.backend any longer (or shouldn't be accessing it), you will want to start a design discussion on django-dev first. That's a very intrusive change and will break a lot of code, so we need to discuss the change in abstract first before wading through lots of lines of code.
Still has access to it, just has access to a compartmentalized version of it. Already converted existing code over to it w/in django; atm, getting v2 up in the tracker (upstream for the patch is bzr branch at http://pkgcore.org/~ferringb/django/connection-pooling) prior to kicking off the discussion; it's worth noting that with a rather simple getattr, it's possible to preserve the old api (preferably firing off a warning on access).
comment:6 by , 17 years ago
Since we don't have an official voting system yet, just adding a comment that I'm +1 for this patch (the first or the second, I'll be happy with either), it would make the SQL logging additions much cleaner.
by , 17 years ago
Attachment: | backend-refactoring-v2-correct-bitrot.patch added |
---|
re-cut v2, since adrians core/management horked v2 from being applied.
comment:7 by , 17 years ago
I'm inclined to check in the first patch, as it's a simpler / more atomic change, and I'd rather not check in substantial patches like the second patch in a single commit (mostly because they're hard to follow). I'm on it.
comment:8 by , 17 years ago
comment:9 by , 17 years ago
First patch is checked into trunk. Working on the second patch now; I'm going to split it over several commits, for sanity's sake.
comment:11 by , 17 years ago
comment:12 by , 17 years ago
comment:13 by , 17 years ago
comment:14 by , 17 years ago
comment:15 by , 17 years ago
comment:16 by , 17 years ago
comment:17 by , 17 years ago
comment:18 by , 17 years ago
comment:19 by , 17 years ago
comment:20 by , 17 years ago
comment:21 by , 17 years ago
comment:22 by , 17 years ago
comment:23 by , 17 years ago
comment:24 by , 17 years ago
comment:25 by , 17 years ago
comment:26 by , 17 years ago
comment:27 by , 17 years ago
(In [5972]) Moved postgresql backend DatabaseOperations class into a new module, postgresql/operations.py, so that it can be imported by both the postgresql and postgresql_psycopg2 backends. Hence the two backends no longer have a duplicated DatabaseOperations class
comment:28 by , 17 years ago
(In [5974]) Implemented BaseDatabaseFeatures and changed all code to access it -- connection.features.foo instead of backend.foo
comment:29 by , 17 years ago
Note that I used "features" instead of "capabilities" because the latter just takes too long to type.
comment:30 by , 17 years ago
comment:31 by , 17 years ago
comment:32 by , 17 years ago
comment:33 by , 17 years ago
comment:34 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
OK, I think we can safely mark this as fixed. I've made 95% of the changes from the patch, with the exception of making DatabaseError
, IntegrityError
and the introspection and creation functionality accessible via django.db.connection
object. Those changes can happen in the near future, if they need to be made. (Please open separate tickets w/ patches for those changes if you think they're worth doing.)
I ended up making small naming/implementation changes in places throughout this fix, but Brian's core design for this refactoring remained intact. Thanks for the great suggestions and patch, Brian!
The whole backends area is a bit messy. This is a good start to tidying it up.
I was tempted to push straight to checkin, but I'll let some more eyes look at it before then.