Add database config class #6513
Conversation
erikjohnston
force-pushed
the
erikj/split_database_config
branch
3 times, most recently
from
3137d27 to
210b520
Compare
This encapsulates config for a given database and is the way to get new connections.
erikjohnston
force-pushed
the
erikj/split_database_config
branch
from
210b520 to
427c9e3
Compare
erikjohnston
force-pushed
the
erikj/split_database_config
branch
from
427c9e3 to
24bed14
Compare
|
This is still quite large I'm afraid, though a fair chunk of it is just fixing up tests. |
synapse/config/database.py
Outdated
| from twisted.enterprise import adbapi | ||
|
|
||
| from synapse.config._base import Config, ConfigError | ||
| from synapse.storage.engines import create_engine |
There was a problem hiding this comment.
having config import from synapse.storage sounds like a great way to end up with horrible import loops (I found a fun one yesterday where if you import synapse.storage first you're ok, but if you import synapse.state it all explodes).
It seems to me that DatabaseConnectionConfig is doing more than a config object should: why can't the call to create_engine, and the get_pool and make_conn and co move into the Database class?
An alternative looks like it might be to move create_engine into config, but that feels wrong to me.
There was a problem hiding this comment.
(that said: a big +1 to moving these things out of the HomeServer god object)
There was a problem hiding this comment.
I don't disagree, it is a pain. At the moment the Database object represents a fully prepared and configured database, we could move the preparation into the __init__, but then we need want to reuse the connection to give to the data stores, which is fiddly if we create the connection in the Database :(
There was a problem hiding this comment.
then we need want to reuse the connection to give to the data stores, which is fiddly if we create the connection in the Database :(
I'm not quite following this?
There was a problem hiding this comment.
Basically its a bit hard to see how to rewrite https://github.com/matrix-org/synapse/pull/6513/files#diff-23406f76811c2c98a6ed1758c0bda2abR42-R54 if we move make_conn to Database
There was a problem hiding this comment.
I see. Make it a top-level function, in synapse.storage.database (or similar), which takes a DatabaseConfig as a param?
There was a problem hiding this comment.
Hmm, ok, I can give that a go
There was a problem hiding this comment.
So 439e043 is my attempt at that, the tests are a bit hacky but that's true of them already.
erikjohnston
force-pushed
the
erikj/split_database_config
branch
2 times, most recently
from
7318971 to
7b297a4
Compare
erikjohnston
force-pushed
the
erikj/split_database_config
branch
from
7b297a4 to
439e043
Compare
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class DatabaseConnectionConfig: |
There was a problem hiding this comment.
this might make more sense as an attr.s ? Just a thought
There was a problem hiding this comment.
Yeah, I thought about that, but then I find the validation stuff non intuitive.
This seems to have been broken since #6513.
This seems to have been broken since #6513.
* commit '2284eb3a5': Add database config class (#6513) too many parens
Replaces #6481.
Based on #6510, #6511 and #6512.