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

Expose the connection pool #22

Open
moxious opened this issue Apr 5, 2016 · 3 comments
Open

Expose the connection pool #22

moxious opened this issue Apr 5, 2016 · 3 comments

Comments

@moxious
Copy link

moxious commented Apr 5, 2016

Right now this module keeps track of the sol-redis-pool instance under _pool. If you have an application that needs cache manager for simple get/put, but also wants to use redis for things like pub/sub, it would be nice to make this item not private, or provide an "approved" method for accessing the pool to draw other connections from it for other purposes. It seems unnecessary to create one connection pool for the cache-manager, and a second pool for other purposes.

Would you be open to accepting a pull request to define say a function getPool() or to rename pool so that it doesn't start with an underscore and isn't considered private to permit this?

@jkernech
Copy link
Contributor

jkernech commented Apr 5, 2016

Hi @moxious
Thanks for your feedback, your use case is interesting.

I prefer to expose a getPool() method, I think it's more cleaner and testable. I'm of course open to accept pull request, just make sure it's well tested to keep the coverage to 100% :-)

@moxious
Copy link
Author

moxious commented Apr 5, 2016

So...this would be absolutely no problem to write up and cover with a test, but after I forked/cloned this repo to do it, for me the test suite doesn't pass as-is.

[mda@zeke:~/sw/node-cache-manager-redis]1 npm test

> [email protected] test /Users/mda/sw/node-cache-manager-redis
> jasmine

Started
....node_redis: Deprecated: The SETEX command contains a "undefined" argument.
This is converted to a "undefined" string now and will return an error from v.3.0 on.
Please handle this in your code to make sure everything works as you intended it to.
undefined:1
undefined
^

SyntaxError: Unexpected token u
    at Object.parse (native)
    at Command.callback (/Users/mda/sw/node-cache-manager-redis/index.js:74:23)
    at normal_reply (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:662:21)
    at RedisClient.return_reply (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:722:9)
    at JavascriptReplyParser.RedisClient.reply_parser.Parser.returnReply (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:146:18)
    at JavascriptReplyParser.run (/Users/mda/sw/node-cache-manager-redis/node_modules/redis-parser/lib/javascript.js:137:18)
    at JavascriptReplyParser.execute (/Users/mda/sw/node-cache-manager-redis/node_modules/redis-parser/lib/javascript.js:112:10)
    at Socket.<anonymous> (/Users/mda/sw/node-cache-manager-redis/node_modules/redis/index.js:223:27)
    at emitOne (events.js:90:13)
    at Socket.emit (events.js:182:7)
npm ERR! Test failed.  See above for more details.

I'm using a docker image of redis 2.8.23 -- and yes I did configure config.json in spec to make sure my host/port settings were right.

@jkernech jkernech added the ready label Jun 1, 2016
@jkernech jkernech removed the ready label Jun 15, 2016
@mrister
Copy link
Contributor

mrister commented Jul 27, 2016

@moxious Tests should no longer be failing on the latest master.

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

No branches or pull requests

3 participants