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

automaticall return Jedis instances to the pool when closed #44

Closed
costin opened this issue Nov 12, 2010 · 11 comments
Closed

automaticall return Jedis instances to the pool when closed #44

costin opened this issue Nov 12, 2010 · 11 comments

Comments

@costin
Copy link

costin commented Nov 12, 2010

Would it be possible to automatically return Jedis instances to the pool when they are 'closed' (disconnected/quit)? Currently one needs to keep track of the connection usage if JedisPool is used which makes it hard to properly do cleanup.
It's a lot easier and consistent to simply ask the client to work with a Jedis instance (no matter if JedisPool is used or not).

@xetorthio
Copy link
Contributor

I don't how we can make this possible. If we hide connection pooling within Jedis class, we will have to borrow a connection on every executed command and maybe even deal with synchronization issues. It will make things even harder.
Now we moved to the Apache Pool, take a look into the docs since it has bunch of features that might help you to deal with keeping track of connection usage.
http://commons.apache.org/pool/apidocs/org/apache/commons/pool/impl/GenericObjectPool.html

@xetorthio
Copy link
Contributor

Or you could use an approach like this:
https://github.com/xetorthio/jedis/issues#issue/36

@hamsterready
Copy link
Contributor

Or use simple template/callback "pattern" just like jdbctemplate or hibernatetemplate.

Something like:
public class JedisTemplate implements InitializingBean {
private JedisPool jedisPool = null;

  @Override
  public void afterPropertiesSet() throws Exception {
    Assert.notNull(jedisPool, "Property jedisPool is required");
  }

  public <T> T doInJedis(JedisCallback<T> jedisCallback) {
    boolean broken = false;
    Jedis j = null;
    try {
      j = jedisPool.getResource();
      onDoInJedis(j, jedisCallback);
      return jedisCallback.run(j);
    } catch (Exception e) {
      broken = true;
      throw new RuntimeException(e.getMessage(), e);
    } finally {
      if (j != null) {
        if (broken) {
          jedisPool.returnBrokenResource(j);
        } else {
          jedisPool.returnResource(j);
        }
      }
    }

  }

  /**
   * Extension point.
   * 
   * @param j
   * @param jedisCallback
   */
  protected void onDoInJedis(Jedis j, JedisCallback<?> jedisCallback) {
    // do nothing
  }

  public void setJedisPool(JedisPool jedisPool) {
    this.jedisPool = jedisPool;
  }
}

And here is simple callback:

public interface JedisCallback<T> {

  T run(Jedis j);
}

There is spring-data project which has redis support. I was going to use it and have played for a while but had to fall back to above solution as it is much simpler and spring-data is not mature at the moment.

@xetorthio
Copy link
Contributor

Maybe it make sense to put this in the Wiki or even in a Gist. If lots of people use we could add this to a contrib project, I don't know if I would add this to the core.

@hamsterready
Copy link
Contributor

Yeah, this is not for core. Just wrapper - and spring data sounds like ultimate solution.

Maybe I will extract my wrapper classes and create spring-jedis or jedis-spring project someday :)

@Iker-Jimenez
Copy link

I would love to see that. I'm using Spring and Jedis too.
Any clever way of configuring the apache pool from Spring xml?

@hamsterready
Copy link
Contributor

Oh, I see I need to drop it somewhere:

public class JedisPoolFactoryBean implements FactoryBean<JedisPool>, DisposableBean {

  private String host;
  private int port = Protocol.DEFAULT_PORT;
  private JedisPool pool;

  @Override
  public void destroy() throws Exception {
    pool.destroy();
  }

  @Override
  public JedisPool getObject() throws Exception {
    pool = new JedisPool(host, port);
    pool.init();
    return pool;
  }

  @Override
  public Class<?> getObjectType() {
    return JedisPool.class;
  }

  @Override
  public boolean isSingleton() {
    return true;
  }

  public void setHost(String host) {
    this.host = host;
  }

  public void setPort(int port) {
    this.port = port;
  }

}

and here is xml:

<bean id="jedisPool" class="com.sentaca.support.redis.JedisPoolFactoryBean">
  <property name="host" value="localhost"/>
</bean>

and that's almost 90% in terms of LOC of whole "project".

@Iker-Jimenez
Copy link

Well, In my case I'm using a ShardedJedisPool, this is with the apache pool, not the old version. I need to set up things like testWhileIdle or maxActive and it looks like I need to use an instance of org.apache.commons.pool.impl.GenericObjectPool.Config for that.

This class doesn't have setters or getters so I had to extend it to provide setters so I could configure an instance and pass as a constructor-arg to my redis.clients.jedis.ShardedJedisPool. I was wondering if there was another way of doing it without extending the Config class.

@hamsterready
Copy link
Contributor

I would just pull code, fix it and kindly ask mr xetorthio to merge :)

@costin
Copy link
Author

costin commented Dec 2, 2010

Even better it would be to simply depend only on the ObjectPool interface since one might want to use her own implementation or use the stack-based implementation.
As for Spring configurations, take a look at the Jedis config classes in Spring Data (along with the RedisTemplate & co).

@xetorthio
Copy link
Contributor

I am closing this issue since it seems we are talking about addind getters and setters to Config object instead of adding a Proxy class.
And I'm still waiting for the pull request! :)

This issue 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

No branches or pull requests

4 participants