ConnectionPool accepts spec key 'checkout_timeout' (Backport) #7684

Merged
merged 1 commit into from Sep 25, 2012

Conversation

Projects
None yet
4 participants
@jrochkind
Contributor

jrochkind commented Sep 18, 2012

Backport of applicable parts of #6441 cb6f839, in a backwards compatible way.

Previously, ConnectionPool uses connection spec key 'wait_timeout' (default 5 seconds) to decide how long a thread can block waiting for a connection to be avail before raising.

However, mysql2 adapter uses this same 'wait_timeout' key for an entirely different value -- setting the MySQL server's own 'wait timeout', how long the server will allow a connection to be idle before closing it. Default is several hours.

Extreme differences in default shows you often do not want these two values to be the same. But if you don't want them to be default, there is no supported API to change one without changing the other to be the same, since they both use 'wait_timeout'.

So this change maeks ConnectionPool use checkout_timeout in the spec, prefering it over wait_timeout. But wait_timeout IS (unlike in master) still used if there without checkout_timeout. If you want mysql2's wait_timeout to be differnet than ConnectionPool's timeout, you can (and have to) set them both -- mysql2 will use the wait_timeout, ConnectionPool will use the checkout_timeout. But prior to this patch there was no way to do that.

The actual code change is very straightforward. But I wasn't sure how to write the test. The test is a bit wonky, if someone has a suggestion of how to organize the test better, please do share. @rafaelfranca , any interest/feedback?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 21, 2012

Member

This no longer merges cleanly, and will need a rebase.

Member

steveklabnik commented Sep 21, 2012

This no longer merges cleanly, and will need a rebase.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Sep 21, 2012

Contributor

had some trouble figuring out how to get this properly rebased, but I think I've done it right. thanks @steveklabnik

Contributor

jrochkind commented Sep 21, 2012

had some trouble figuring out how to get this properly rebased, but I think I've done it right. thanks @steveklabnik

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 22, 2012

Member

You have! :D

Member

steveklabnik commented Sep 22, 2012

You have! :D

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Sep 24, 2012

Contributor

so, um, what would it take to get this merged into 3-2-stable (or feedback given on why it's not suitable for such)? Anything I can do?

Contributor

jrochkind commented Sep 24, 2012

so, um, what would it take to get this merged into 3-2-stable (or feedback given on why it's not suitable for such)? Anything I can do?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 24, 2012

Member

Needs someone from Rails Core to review. I'm not allowed to use my merge privledges.

I'd bet either @tenderlove or @jonleighton would be good people here.

Member

steveklabnik commented Sep 24, 2012

Needs someone from Rails Core to review. I'm not allowed to use my merge privledges.

I'd bet either @tenderlove or @jonleighton would be good people here.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 24, 2012

Member

It seems fine to me. I just want to know if @tenderlove have something against this change.

Member

rafaelfranca commented Sep 24, 2012

It seems fine to me. I just want to know if @tenderlove have something against this change.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 24, 2012

Member

We will need a CHANGELOG entry

Member

rafaelfranca commented Sep 24, 2012

We will need a CHANGELOG entry

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Sep 24, 2012

Contributor

I can add a CHANGELOG entry to the pull request commit (I wasn't sure if
ALL changes were supposed to have CHANGELOG or not). The problem is if I do
that, and then this sits for another two weeks, the CHANGELOG prob wont'
merge anymore since there will have been other changes to the top of the
CHANGELOG, right? I will add a CHANGELOG entry to the commit if I get some
indication that it'll be merged after that instead of going stale, does
that make sense?

On Mon, Sep 24, 2012 at 1:27 PM, Rafael Mendonça França <
notifications@github.com> wrote:

We will need a CHANGELOG entry


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/7684#issuecomment-8827006.

Contributor

jrochkind commented Sep 24, 2012

I can add a CHANGELOG entry to the pull request commit (I wasn't sure if
ALL changes were supposed to have CHANGELOG or not). The problem is if I do
that, and then this sits for another two weeks, the CHANGELOG prob wont'
merge anymore since there will have been other changes to the top of the
CHANGELOG, right? I will add a CHANGELOG entry to the commit if I get some
indication that it'll be merged after that instead of going stale, does
that make sense?

On Mon, Sep 24, 2012 at 1:27 PM, Rafael Mendonça França <
notifications@github.com> wrote:

We will need a CHANGELOG entry


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/7684#issuecomment-8827006.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 24, 2012

Member

I'll merge it as soon you add a CHANGELOG.

Member

rafaelfranca commented Sep 24, 2012

I'll merge it as soon you add a CHANGELOG.

ConnectionPool accepts spec key 'checkout_timeout'
Backport of #6441 cb6f839 . Old 'wait_timeout' is still supported,
but conflicts with mysql2 using that spec key for different thing.
'checkout_timeout' can now be used taking precedence for ConnectionPool
over 'wait_timeout'.
@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Sep 25, 2012

Contributor

done. and yeah,I can rebase again if I need to just let me know.

Contributor

jrochkind commented Sep 25, 2012

done. and yeah,I can rebase again if I need to just let me know.

rafaelfranca added a commit that referenced this pull request Sep 25, 2012

Merge pull request #7684 from jrochkind/connection_pool_timeout_key_b…
…ackport

ConnectionPool accepts spec key 'checkout_timeout' (Backport)

@rafaelfranca rafaelfranca merged commit 8800aae into rails:3-2-stable Sep 25, 2012

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 25, 2012

Member

Thanks

Member

rafaelfranca commented Sep 25, 2012

Thanks

@@ -54,8 +54,11 @@ module ConnectionAdapters
# your database connection configuration:
#
# * +pool+: number indicating size of connection pool (default 5)
- # * +wait_timeout+: number of seconds to block and wait for a connection
- # before giving up and raising a timeout error (default 5 seconds).
+ # * +checkout _timeout+: number of seconds to block and wait for a

This comment has been minimized.

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