Changing connect function to support more options #9

Open
matthias-endler opened this Issue Jun 9, 2013 · 20 comments

Projects

None yet

3 participants

Contributor

Hi,

it would be nice, to change the connect function, so an arbitrary number of parameters can be passed. This would also eliminate the need for an init_fun in bank.

This would be helpful, i.e. when you want to pass in the desired charset and collation upon connect. Or once transaction support is completed, you could pass in if you like to have auto commit set or not.

whatever() ->
  ConnectParams = [
    {driver, bank_mysql},
    {host, "db.test.com"},
    {user, "dbuser"},
    {password, "secret"},
    {database, "test"},
    {charset, utf_general_ci},
    {auto_commit, off}
  ],
  bank:start_pool(my_pool, 10, ConnectParams).

There would also be a chance to support meaningful defaults per bank driver. I.e. for bank_mysql there could be a default for the port number.

What are your thoughts?

'matthias'.

Owner
essen commented Jun 9, 2013

We need init_fun, because this is what allows us to prepare statements once on connect (and for every subsequent reconnects).

Owner
essen commented Jun 9, 2013

Note this doesn't mean we can't make this simpler.

Contributor

Okay, that makes absolute sense. I'll think a little about how things could be simplified. Any plans on implementing bank drivers for other RDBMSes? I ask, because I'm thinking of building one for PostgreSQL. The wire protocol is really nice and clean compared to the one of MySQL.

Owner
essen commented Jun 9, 2013

I need one for pgsql but I just started working on an async HTTP/SPDY/Websocket client that will occupy my time for a while, so feel free to start.

Owner
essen commented Jun 9, 2013

I also wanted to have some simple ct tests before starting the pgsql driver.

Contributor

Tests for bank_mysql are the next thing I wanted to do. I'm not in a hurry for the pgsql driver, but I know that I need one in the not too distant future.

At the moment I'm working on a customer project, where I need mysql support. And I didn't really like Emysql nor erlang-mysql-driver. This doesn't mean they are bad, but I think their architecture is flawed.

I really like the bank architecture, because it's simple and DRY!

Owner
essen commented Jun 9, 2013

Probably can make tests for bank in bank that can be reused across all drivers easily.

Contributor

That's a good idea, so you can also test, that all bank drivers behave in a consistent way.

Contributor

What about passing in the init_fun like this:

whatever() ->
  ConnectParams = [
    {driver, bank_mysql},
    {host, "db.test.com"},
    {user, "dbuser"},
    {password, "secret"},
    {database, "test"},
    {charset, utf_general_ci},
    {auto_commit, off},
    {init_fun, {?MODULE, init_fun, []}}
  ],
  bank:start_pool(my_pool, 10, ConnectParams).
Contributor
bfrog commented Jun 11, 2013

I think thats fine, a post connect init_fun hook, and I like the simplified options here.

Contributor

Looking at it I'd rather use true and false instead of on and off for boolean parameters. Other than that I think it's okay. Will implement it this way.

Contributor

I think init_fun should be called from bank and not the implementation of bank_client. It should also pass the Ref of the Pool as a parameter. So the init_fun is not a connection parameter, but a pool parameter.

connect_to_db() ->
  PoolParams = [{initial_pool_size, 10}, {driver, bank_mysql}, {init_fun, fun ?MODULE:init_fun/1}],
  ConnParams = [{user, "dbuser"}, {password, "secret"}, {database, "test"}],
  ok = bank:start_pool(my_db, PoolParams, ConnParams).

init_fun(Ref) ->
  ok = bank:prepare(Ref, create_blah, "INSERT INTO blah (name) VALUES (?)",
  ok = bank:prepare(Ref, read_blah, "SELECT id, name FROM blah WHERE id = ?"),
  ok = bank:prepare(Ref, update_blah, "UPDATE blah SET name = ? WHERE id = ?"),
  ok = bank:prepare(Ref, delete_blah, "DELETE FROM blah WHERE id = ?").

Any thoughts on that?

Owner
essen commented Jun 12, 2013

Probably call it onconnect instead btw.

Contributor

Yes... and probably it would be a good idea to pass the connection parameters to the driver once to check for sanity, pass back an opaque data structure to bank, to use for connect. This would avoid rechecking the parameters on every new connect or reconnect.

-module(bank_client).

%% [snip]

-type valid_opts() :: any().

-export_type([valid_opts/0]).

-callback validate_opts(opts()) -> {ok, valid_opts()} | {error, atom(), string()}.
-callback connect(valid_opts())
    -> {ok, state()}
    | {error, timeout | auth | atom(), string()}.

%% [snip]
Contributor

The data structure passed back would usually be a tuple (record).

Contributor

With @essen 's and @bfrog 's suggestions:

connect_to_db() ->
  WorkerOpts = [{onconnect, fun ?MODULE:db_onconnect/1}],
  ClientOpts = [{user, "dbuser"}, {password, "secret"}, {database, "test"}],
  ok = bank:start_pool(my_db, 10, bank_mysql, ClientOpts, WorkerOpts).

db_onconnect(Ref) ->
  ok = bank:prepare(Ref, create_blah, "INSERT INTO blah (name) VALUES (?)",
  ok = bank:prepare(Ref, read_blah, "SELECT id, name FROM blah WHERE id = ?"),
  ok = bank:prepare(Ref, update_blah, "UPDATE blah SET name = ? WHERE id = ?"),
  ok = bank:prepare(Ref, delete_blah, "DELETE FROM blah WHERE id = ?").
Contributor
bfrog commented Jun 12, 2013

I think this is good, with regard to the valid_opts param I don't think its a huge deal to require that, but it wouldn't be bad either.

Contributor

You are probably right... I think the valid_opts param could be an example of premature optimization. I'll do it without for the moment.

Owner
essen commented Jun 19, 2013

I'd say don't use two separate arguments, but instead do something like {driver, bank_mysql, DriverOpts}. This way it's more obvious what is what.

Contributor

Yes, makes sense, because it's a worker option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment