Skip to content

Fixes redic-pool incompatabilty with latest ohm version (2.3.0)#5

Open
rwrede wants to merge 13 commits intodjanowski:masterfrom
rwrede:master
Open

Fixes redic-pool incompatabilty with latest ohm version (2.3.0)#5
rwrede wants to merge 13 commits intodjanowski:masterfrom
rwrede:master

Conversation

@rwrede
Copy link

@rwrede rwrede commented Sep 30, 2015

  1. The latest ohm version (2.3.0) uses the new redic API which includes a method Redic#call! (bang method). In order to conform to that new expected API this method was also added to Redic::Pool in this PR. After this PR is merged it should be possible to use redic-pool with ohm 2.3.0.
  2. Travis support was added. Some additional commits were necessary in order to make travis bundle and build successfully.
  3. The gemspec file now specifies gem dependency versions explicitly, which should be good practice. (Though, I am not totally sure if I am too strict here or which exact versions should be specified. Maybe you can help @djanowski )

@rwrede
Copy link
Author

rwrede commented Sep 30, 2015

@djanowski, if you are ok with adding travis support you might want to add the travis badge to the README after enabling travis support on Travis CI for your repo.

@rwrede
Copy link
Author

rwrede commented Sep 30, 2015

Also noticed the build on Travis CI to be a bit unstable because, it seems, sometimes the open connection are < 10, e.g. only 8. Maybe you have an idea on how to improve the specs.

@scalp42
Copy link

scalp42 commented Sep 28, 2016

@djanowski any chance you could look at this PR please ?

@mwpastore
Copy link

mwpastore commented Oct 24, 2017

@rwrede This patch is not correct. Redic::Pool#call! should delegate to Redic#call!, not Redic::Pool#call. Ohm expects call! to raise a RuntimeError (instead of just returning it).

I know this is an old PR and a lot has probably changed over the past two years, but generally speaking, you may have better luck getting PRs looked at if you break them into smaller pieces, e.g. one PR for the gemspec/Gemfile changes, one PR to add Travis, and one PR for call!. You may also consider squashing some of the commits to clean up the history (GitHub can do this automatically now, but because there are multiple changes in a single PR, it may not make sense to squash them all together).

All that being said, it looks like this project is dead, which is unfortunate because it appears to be the only sane option for using Redic and Ohm in a multithreaded environment (in my case, Puma and Sidekiq) with connection pooling. So none of that matters. I'm looking at maintaining my own fork until @djanowski makes his triumphant return!

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.

4 participants