Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add pool_timeout and max_connections parameters #11

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

lcx
Copy link

@lcx lcx commented May 6, 2015

Added configuration option for pool_timeout and max_connections to pass over to sequel


@@connection = establish_connection connection_string(params)
@@connection = establish_connection(connection_string(params),max_connections,pool_timeout)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit, but would you please add spaces after the commas?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it occurs to me that it might be better to pass this as a hash to #establish_connection, in case we get more params in the future. The calling code would then look something like

@@connection = establish_connection connection_string(params), max_connections: max_connections, pool_timeout: pool_timeout

@bklang
Copy link
Owner

bklang commented May 6, 2015

A few code nits, but thank you for your contribution!

@lcx
Copy link
Author

lcx commented May 7, 2015

Odd, no idea why I used ruby 1.8 syntax, I occasionally do that :)
Should I fix the issues or will you fix it?
Survived in production today without PoolTimeout exception with a bunch of calls to a radio station. So looks stable I would say.

@vangberg
Copy link

vangberg commented May 7, 2015

There are a lot of adapter specific connection options[1], so I'm wondering if it wouldn't make sense to take a connection_options configuration key, and pass verbatim to Sequel.connect?

[1] http://sequel.jeremyevans.net/rdoc/files/doc/opening_databases_rdoc.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants