Shared DB pools #1217
Shared DB pools #1217
Conversation
| @@ -57,11 +56,11 @@ query(Connection, Query, Timeout) when is_binary(Query) -> | |||
| query(Connection, Query, Timeout) -> | |||
| parse(odbc:sql_query(Connection, Query, Timeout)). | |||
|
|
|||
| -spec prepare(Host :: ejabberd:server(), Connection :: term(), Name :: atom(), Table :: binary(), | |||
| -spec prepare(Pool :: mongoose_rdbms:pool(), Connection :: term(), Name :: atom(), Table :: binary(), | |||
There was a problem hiding this comment.
According to Elvis:
Line 59 is too long: -spec prepare(Pool :: mongoose_rdbms:pool(), Connection :: term(), Name :: atom(), Table :: binary(),.
| @@ -51,10 +51,10 @@ disconnect(Connection) -> | |||
| query(Connection, Query, _Timeout) -> | |||
| mysql_to_odbc(mysql:query(Connection, Query), Connection). | |||
|
|
|||
| -spec prepare(Host :: ejabberd:server(), Connection :: term(), Name :: atom(), Table :: binary(), | |||
| -spec prepare(Pool :: mongoose_rdbms:pool(), Connection :: term(), Name :: atom(), Table :: binary(), | |||
There was a problem hiding this comment.
According to Elvis:
Line 54 is too long: -spec prepare(Pool :: mongoose_rdbms:pool(), Connection :: term(), Name :: atom(), Table :: binary(),.
| @@ -51,10 +51,10 @@ disconnect(Connection) -> | |||
| query(Connection, Query, _Timeout) -> | |||
| pgsql_to_odbc(epgsql:squery(Connection, Query)). | |||
|
|
|||
| -spec prepare(Host :: ejabberd:server(), Connection :: term(), Name :: atom(), Table :: binary(), | |||
| -spec prepare(Pool :: mongoose_rdbms:pool(), Connection :: term(), Name :: atom(), Table :: binary(), | |||
There was a problem hiding this comment.
According to Elvis:
Line 54 is too long: -spec prepare(Pool :: mongoose_rdbms:pool(), Connection :: term(), Name :: atom(), Table :: binary(),.
doc/Advanced-configuration.md
Outdated
| * **odbc_pool** (local) | ||
| * **Description:** Name of the default connection pool used to connect to the database. | ||
| * **Syntax:** `{odbc_pool, PoolName}` | ||
| * **Default:** `"deafult"` |
To declare a DB connection pool, use on of the following formats:
{pool, odbc, PoolName}.
{pool, odbc, PoolName, Options}.
PoolName is an atom and Options is a list of ODBC options. Options can be still specified at the top level.
1. All hosts use the ‘default’ pool by default.
To change this, use the {odbc_pool, Option} option (top-level or per-host).
2. This means that setting ‘odbc_server’ is not enough for a DB connection to work. The default pool has to be declared as well.
3. The ODBC options specified per-host are not allowed anymore. They have been replaced with the Options element of the pool declaration.
To update the previous config with ODBC enabled to the new format, add the following line to ejabberd.cfg:
{pool, odbc, default}.
chrzaszcz
force-pushed
the
shared-db-pools
branch
from
15f7014
to
ac3d079
Compare
chrzaszcz
force-pushed
the
shared-db-pools
branch
from
07e5066
to
2ea991f
Compare
There was a problem hiding this comment.
Thanks a lot for these changes @chrzaszcz. This was long waited feature!
I've put two comments which I'd like to get clarified.
| Size | ||
| end. | ||
|
|
||
| -spec get_option(mongoose_rdbms:pool(), atom()) -> term(). | ||
| get_option(Pool, Option) -> |
There was a problem hiding this comment.
Would it feel more natural if this function was moved to mongooseim_rdbms module?
There was a problem hiding this comment.
The motivation here was to avoid cyclic dependencies between modules - I would be less reluctant to allow mongoose_rdbms_sup call mongoose_rdbms if the worker logic was extracted from the latter - which I would recommend.
| @@ -86,10 +85,10 @@ keepalive_exit(Config) -> | |||
| meck_config(Server, KeepaliveInterval) -> | |||
| meck:new(ejabberd_config, [no_link]), | |||
| meck:expect(ejabberd_config, get_local_option, | |||
| fun({odbc_keepalive_interval, _Host}) -> KeepaliveInterval; | |||
| ({odbc_start_interval, _Host}) -> 30; | |||
| fun({odbc_keepalive_interval, odbc_pool, default}) -> KeepaliveInterval; | |||
There was a problem hiding this comment.
I know this is a test. It's also a code we read and this 3 element tuple is not intuitive at first glance. Would it be possible to mock mongoose_rdbms_sup:get_option here instead of ejabberd_config:get_local_option?
That's OK if you don't have time for this @chrzaszcz now, I'd just like to know that.
There was a problem hiding this comment.
Your suggestion makes sense. I don't feel it's necessary, but it would improve the readability and unit test isolation indeed.
|
Thanks! |
For version > 2.0.1 esl/MongooseIM#1217
Make DB connection pool configuration independent from the host config.
To declare a DB connection pool, use on of the following formats:
PoolName is an atom and Options is a list of ODBC options. Options can be still specified at the top level.
All hosts use the ‘default’ pool by default.
To change this, use the {odbc_pool, Option} option (top-level or per-host).
This means that setting ‘odbc_server’ is not enough for a DB connection to work. The default pool has to be declared as well.
The ODBC options specified per-host are not allowed anymore. They have been replaced with the Options element of the pool declaration.
To update the previous config with ODBC enabled to the new format, add the following line to ejabberd.cfg:
{pool, odbc, default}.