Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

ConnectionPool checkin should put connection back into the pool #227

Closed
wants to merge 2 commits into from

Conversation

arthurnn
Copy link
Contributor

(this PR depends on #226 being merged first)

We need to checkin the connection(put back in the pool) after use.
We cannot only depend on the reaper to get rid of connection, as threads could be sleeping or blocked by something else. If we make the connections rotate in the pool, we could have more threads using less connections.

just for an example, other gems are going the same thing:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L370
https://github.com/zk-ruby/zk/blob/master/lib/zk/pool.rb#L203
https://github.com/mperham/connection_pool/blob/master/lib/connection_pool.rb#L79

[fixes #223]

review @durran

@coveralls
Copy link

Coverage Status

Coverage increased (+0.11%) when pulling 5822e82 on conn_pool_checkin into 95f6ac9 on connection_pool_queue.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.11%) when pulling 5822e82 on conn_pool_checkin into 95f6ac9 on connection_pool_queue.

@blackxored
Copy link

@arthurnn When you mentioned #226 needs to be merged, is it on this branch?
I'm experiencing some weird disconnection issues after I pointed to this branch on my Gemfile. It solved the problem with tests, but now I'm getting errors similar to this:

Moped::Errors::ConnectionFailure (Could not connect to a primary node for replica set #<Moped::Cluste /data/apps/<OMITTED>/shared/bundle/ruby/2.0.0/bin/sidekiq:23:in `<main>'
r:-568130558 @seeds=[<Moped::Node resolved_address="127.0.0.1:27017">, <Moped::Node resolved_address= 2013-10-30T04:41:14Z 5223 TID--a0dkeq INFO: Booting Sidekiq 2.3.3 with Redis at redis://localhost:6379
"138.91.90.230:27017">, <Moped::Node resolved_address="127.0.1.1:27017">, <Moped::Node resolved_addre /0
ss="138.91.90.230:27017">]>):                                                                         
2013-10-30T04:41:14Z 5223 TID--a0dkeq INFO: Running in ruby 2.0.0p247 (2013-06-27 revision 41674) [i68
  app/controllers/static_controller.rb:5:in `index'                        

It most likely has to do with sidekiq reconnecting, but since I'm using master branch there and I don't see how moped would affect it, I'm wondering whether the root cause has to do with this branch.
Thanks.

@arthurnn
Copy link
Contributor Author

arthurnn commented Nov 2, 2013

@blackxored this PR is #226 + 1 commit (note the PR is not against master) .
So I am not sure if I understand your issue: The place that is raising the error is the sidekiq worker, correct? on that ruby instead are you pointing moped to master or this branch? let me know, if this branch is failing and master is working. So I can fix this before I merge it to master. thanks

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7e9b992 on conn_pool_checkin into 95f6ac9 on connection_pool_queue.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7e9b992 on conn_pool_checkin into 95f6ac9 on connection_pool_queue.

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

Successfully merging this pull request may close these issues.

None yet

3 participants