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

chore: use after_connect hook within connection pool for each new connection #186

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

kamilpavlicko
Copy link

@kamilpavlicko kamilpavlicko commented Apr 12, 2022

Motivation

Use after_connect hook in the proper way with each connection within the connection pool (sequel doc)

Actual behavior

Currently, there is after_connect executed only once for the Sequel connection instance, but it's not executed when creating new connection within connection pool

Issue

With using sequel-rails with rails and multithreaded webserver (for example Puma) there is a need to utilize a connection pool for multiple connections.
Let's say that we want to set statement_timeout for all connections in the connection pool.

config.sequel.after_connect = proc do |db|
  db.execute('SET statement_timeout = 30000;')
end

So with max_connections => 5 and running

5.times{ Thread.new{ puts DB['SHOW statement_timeout'].to_a }}

there should be the same timeout -> 30s for all connections in the connection pool

@JonathanTron @JosephHalter
Could you please take a look at that to verify all the changes? :)

@JonathanTron
Copy link
Member

Hi @kamilpavlicko, thanks for the PR.

My only concern is that we're changing the behaviour for after_connect in sequel-rails.

It's been a long time, but from what I recall, at the time, we intentionally didn't implement it using sequel after_connect. The current behaviour means this hook is called only once when we start a new rails process, while the new behaviour would make those existing hooks called once per new connection (which is not necessarily on rails boot, if I recall correctly).

What I propose is:

  1. keep the existing after_connect as it is, called once per rails process.
  2. add a new after_each_connect (or after_every_connect, or whatever else you prefer) which we can hook into sequel's after_connect.

What do you think?

@kamilpavlicko
Copy link
Author

@JonathanTron
Yep, totally agree.
Then I would go with after_new_connection approach. Will change the code and will update Readme.

@kamilpavlicko
Copy link
Author

@JonathanTron
PR updated 👍

@JonathanTron
Copy link
Member

Thanks for the changes @kamilpavlicko !

@JonathanTron JonathanTron merged commit b91f8d8 into TalentBox:master Apr 13, 2022
@kamilpavlicko
Copy link
Author

Is there any date or plan, when it is going to be released?

@JonathanTron
Copy link
Member

@kamilpavlicko I can make a new release next week. My concern is that we don't have CI running anymore since TravisCI is no longer free for OSS, I started a branch to migrate to Github Actions... but I never finished.

@kamilpavlicko
Copy link
Author

@JonathanTron
I've noticed new release there ... awesome job, thank you 👍

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.

2 participants