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

JedisPoolConfig: Helper class for Spring configuration of object pool Config #79

Closed
wants to merge 1 commit into from

Conversation

sehugg
Copy link
Contributor

@sehugg sehugg commented Jan 12, 2011

No description provided.

@gsharma
Copy link

gsharma commented Jan 15, 2011

I was just going to make an issue to wrap the Config class in order to fix the ripple effect caused by exposing this to all Jedis clients who now have to add commons-pool dependency just to use the JedisPool. We can use Steven's JedisPoolConfig wrapper to internally populate the commons-pool Config and change JedisPool constructor to use JedisPoolConfig instead. This will also necessitate JedisPoolConfig not extending Config. The basic idea is for Jedis to manage its external library dependencies internally and not leak them via its interfaces to clients (leaky abstractions).

@xetorthio
Copy link
Contributor

This is OK also I need this to set some default to the pool to make it better for Redis.
Thanks a lot and sorry for the delay, was on vacations.

@gsharma
Copy link

gsharma commented Jan 17, 2011

What I was implying was to remove the commons-pool Config from JedisPool constructor so the dependency does not leak. Actually, a jedis.conf file can have details for commons-pool configuration where all the parameters for Config can be specified and Jedis can internally create the commons-pool Config object instead of clients having to inject it. That way we don't spill Jedis's dependency to its clients.

@xetorthio
Copy link
Contributor

pushed to master with pool defaults as described in this issue https://github.com/xetorthio/jedis/issues#issue/73

This pull request was closed.
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.

3 participants