This commit addresses the first part discussed in #909.

To improve usability we're changing the visibility of `returnResoure` and `returnBrokenResource` in favor of `jedis.close()` which automatically returns the resource to the pool accordingly. As `Pool` and `Jedis` are currently in different packages i've created a JedisPoolAbstract class to provide a bridge between the two implementations
redis · Mar 9, 2015 · 6701b07 · 6701b07 · xiaoshuai · Apr 13, 2015
1 parent f43e3a0
commit 6701b07
Show file tree

Hide file tree

Showing 10 changed files with 88 additions and 54 deletions.
diff --git a/src/main/java/redis/clients/jedis/Jedis.java b/src/main/java/redis/clients/jedis/Jedis.java
@@ -1,19 +1,25 @@
 package redis.clients.jedis;
 
+import java.net.URI;
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+
 import redis.clients.jedis.BinaryClient.LIST_POSITION;
 import redis.clients.jedis.JedisCluster.Reset;
-import redis.clients.util.Pool;
 import redis.clients.util.SafeEncoder;
 import redis.clients.util.Slowlog;
 
-import java.net.URI;
-import java.util.*;
-import java.util.Map.Entry;
-
 public class Jedis extends BinaryJedis implements JedisCommands, MultiKeyCommands,
     AdvancedJedisCommands, ScriptingCommands, BasicCommands, ClusterCommands, SentinelCommands {
 
-  protected Pool<Jedis> dataSource = null;
+  protected JedisPoolAbstract dataSource = null;
 
   public Jedis() {
     super();
@@ -3169,7 +3175,7 @@ public void close() {
     }
   }
 
-  public void setDataSource(Pool<Jedis> jedisPool) {
+  public void setDataSource(JedisPoolAbstract jedisPool) {
     this.dataSource = jedisPool;
   }
 

diff --git a/src/main/java/redis/clients/jedis/JedisPool.java b/src/main/java/redis/clients/jedis/JedisPool.java
@@ -7,9 +7,8 @@
 
 import redis.clients.jedis.exceptions.JedisException;
 import redis.clients.util.JedisURIHelper;
-import redis.clients.util.Pool;
 
-public class JedisPool extends Pool<Jedis> {
+public class JedisPool extends JedisPoolAbstract {
 
   public JedisPool() {
     this(Protocol.DEFAULT_HOST, Protocol.DEFAULT_PORT);
@@ -88,13 +87,13 @@ public Jedis getResource() {
     return jedis;
   }
 
-  public void returnBrokenResource(final Jedis resource) {
+  protected void returnBrokenResource(final Jedis resource) {
     if (resource != null) {
       returnBrokenResourceObject(resource);
     }
   }
 
-  public void returnResource(final Jedis resource) {
+   protected void returnResource(final Jedis resource) {
     if (resource != null) {
       try {
         resource.resetState();

diff --git a/src/main/java/redis/clients/jedis/JedisPoolAbstract.java b/src/main/java/redis/clients/jedis/JedisPoolAbstract.java
@@ -0,0 +1,29 @@
+package redis.clients.jedis;
+
+import org.apache.commons.pool2.PooledObjectFactory;
+import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+
+import redis.clients.util.Pool;
+
+public class JedisPoolAbstract extends Pool<Jedis> {
+
+
+	public JedisPoolAbstract() {
+		super();
+	}
+
+	public JedisPoolAbstract(GenericObjectPoolConfig poolConfig,
+			PooledObjectFactory<Jedis> factory) {
+		super(poolConfig, factory);
+	}
+
+	@Override
+	protected void returnBrokenResource(Jedis resource) {
+		super.returnBrokenResource(resource);
+	}
+
+	@Override
+	protected void returnResource(Jedis resource) {
+		super.returnResource(resource);
+	}
+}
diff --git a/src/main/java/redis/clients/jedis/JedisSentinelPool.java b/src/main/java/redis/clients/jedis/JedisSentinelPool.java
@@ -13,7 +13,7 @@
 import redis.clients.jedis.exceptions.JedisException;
 import redis.clients.util.Pool;
 
-public class JedisSentinelPool extends Pool<Jedis> {
+public class JedisSentinelPool extends JedisPoolAbstract {
 
   protected GenericObjectPoolConfig poolConfig;
 

diff --git a/src/main/java/redis/clients/jedis/ShardedJedis.java b/src/main/java/redis/clients/jedis/ShardedJedis.java
@@ -9,11 +9,10 @@
 
 import redis.clients.jedis.BinaryClient.LIST_POSITION;
 import redis.clients.util.Hashing;
-import redis.clients.util.Pool;
 
 public class ShardedJedis extends BinaryShardedJedis implements JedisCommands, Closeable {
 
-  protected Pool<ShardedJedis> dataSource = null;
+  protected ShardedJedisPool dataSource = null;
 
   public ShardedJedis(List<JedisShardInfo> shards) {
     super(shards);
@@ -631,7 +630,7 @@ public void close() {
     }
   }
 
-  public void setDataSource(Pool<ShardedJedis> shardedJedisPool) {
+  public void setDataSource(ShardedJedisPool shardedJedisPool) {
     this.dataSource = shardedJedisPool;
   }
 

diff --git a/src/main/java/redis/clients/jedis/test/MainTest.java b/src/main/java/redis/clients/jedis/test/MainTest.java
@@ -0,0 +1,21 @@
+package redis.clients.jedis.test;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class MainTest {
+
+	public static void main(String[] args) {
+
+
+		List<Integer> list = new ArrayList<Integer>(1000000);
+		long start = System.currentTimeMillis();
+		for (int i = 0; i < 1000000; i++) {
+			list.add(i);
+		}
+
+		System.out.println("Took: " + (System.currentTimeMillis() - start) + " milliseconds");
+
+	}
+
+}
diff --git a/src/main/java/redis/clients/util/Pool.java b/src/main/java/redis/clients/util/Pool.java
@@ -62,13 +62,13 @@ public void returnResourceObject(final T resource) {
     }
   }
 
-  public void returnBrokenResource(final T resource) {
+  protected void returnBrokenResource(final T resource) {
     if (resource != null) {
       returnBrokenResourceObject(resource);
     }
   }
 
-  public void returnResource(final T resource) {
+  protected void returnResource(final T resource) {
     if (resource != null) {
       returnResourceObject(resource);
     }

diff --git a/src/test/java/redis/clients/jedis/tests/JedisPoolTest.java b/src/test/java/redis/clients/jedis/tests/JedisPoolTest.java
@@ -29,7 +29,7 @@ public void checkConnections() {
     jedis.auth("foobared");
     jedis.set("foo", "bar");
     assertEquals("bar", jedis.get("foo"));
-    pool.returnResource(jedis);
+    jedis.close();
     pool.destroy();
     assertTrue(pool.isClosed());
   }
@@ -41,7 +41,7 @@ public void checkCloseableConnections() throws Exception {
     jedis.auth("foobared");
     jedis.set("foo", "bar");
     assertEquals("bar", jedis.get("foo"));
-    pool.returnResource(jedis);
+    jedis.close();
     pool.close();
     assertTrue(pool.isClosed());
   }
@@ -53,7 +53,7 @@ public void checkConnectionWithDefaultPort() {
     jedis.auth("foobared");
     jedis.set("foo", "bar");
     assertEquals("bar", jedis.get("foo"));
-    pool.returnResource(jedis);
+    jedis.close();
     pool.destroy();
     assertTrue(pool.isClosed());
   }
@@ -65,12 +65,12 @@ public void checkJedisIsReusedWhenReturned() {
     Jedis jedis = pool.getResource();
     jedis.auth("foobared");
     jedis.set("foo", "0");
-    pool.returnResource(jedis);
+    jedis.close();
 
     jedis = pool.getResource();
     jedis.auth("foobared");
     jedis.incr("foo");
-    pool.returnResource(jedis);
+    jedis.close();
     pool.destroy();
     assertTrue(pool.isClosed());
   }
@@ -81,12 +81,12 @@ public void checkPoolRepairedWhenJedisIsBroken() {
     Jedis jedis = pool.getResource();
     jedis.auth("foobared");
     jedis.quit();
-    pool.returnBrokenResource(jedis);
+    jedis.close();
 
     jedis = pool.getResource();
     jedis.auth("foobared");
     jedis.incr("foo");
-    pool.returnResource(jedis);
+    jedis.close();
     pool.destroy();
     assertTrue(pool.isClosed());
   }
@@ -113,7 +113,7 @@ public void securePool() {
     JedisPool pool = new JedisPool(config, hnp.getHost(), hnp.getPort(), 2000, "foobared");
     Jedis jedis = pool.getResource();
     jedis.set("foo", "bar");
-    pool.returnResource(jedis);
+    jedis.close();
     pool.destroy();
     assertTrue(pool.isClosed());
   }
@@ -125,15 +125,15 @@ public void nonDefaultDatabase() {
     Jedis jedis0 = pool0.getResource();
     jedis0.set("foo", "bar");
     assertEquals("bar", jedis0.get("foo"));
-    pool0.returnResource(jedis0);
+    jedis0.close();
     pool0.destroy();
     assertTrue(pool0.isClosed());
 
     JedisPool pool1 = new JedisPool(new JedisPoolConfig(), hnp.getHost(), hnp.getPort(), 2000,
         "foobared", 1);
     Jedis jedis1 = pool1.getResource();
     assertNull(jedis1.get("foo"));
-    pool1.returnResource(jedis1);
+    jedis1.close();
     pool1.destroy();
     assertTrue(pool1.isClosed());
   }
@@ -184,13 +184,13 @@ public void selectDatabaseOnActivation() {
     jedis0.select(1);
     assertEquals(1, jedis0.getDB());
 
-    pool.returnResource(jedis0);
+    jedis0.close();
 
     Jedis jedis1 = pool.getResource();
     assertTrue("Jedis instance was not reused", jedis1 == jedis0);
     assertEquals(0, jedis1.getDB());
 
-    pool.returnResource(jedis1);
+    jedis1.close();
     pool.destroy();
     assertTrue(pool.isClosed());
   }
@@ -204,7 +204,7 @@ public void customClientName() {
 
     assertEquals("my_shiny_client_name", jedis.clientGetname());
 
-    pool0.returnResource(jedis);
+    jedis.close();
     pool0.destroy();
     assertTrue(pool0.isClosed());
   }
@@ -254,7 +254,7 @@ public void passivateObject(PooledObject<Jedis> p) throws Exception {
     Jedis crashingJedis = pool.getResource();
 
     try {
-      pool.returnResource(crashingJedis);
+      crashingJedis.close();
     } catch (Exception ignored) {
     }
 
@@ -311,16 +311,6 @@ public void checkResourceIsCloseable() {
     }
   }
 
-  @Test
-  public void returnNullObjectShouldNotFail() {
-    JedisPool pool = new JedisPool(new JedisPoolConfig(), hnp.getHost(), hnp.getPort(), 2000,
-        "foobared", 0, "my_shiny_client_name");
-
-    pool.returnBrokenResource(null);
-    pool.returnResource(null);
-    pool.returnResourceObject(null);
-  }
-
   @Test
   public void getNumActiveIsNegativeWhenPoolIsClosed() {
     JedisPool pool = new JedisPool(new JedisPoolConfig(), hnp.getHost(), hnp.getPort(), 2000,
@@ -346,10 +336,10 @@ public void getNumActiveReturnsTheCorrectNumber() {
 
     assertEquals(2, pool.getNumActive());
 
-    pool.returnResource(jedis);
+    jedis.close();
     assertEquals(1, pool.getNumActive());
 
-    pool.returnResource(jedis2);
+    jedis2.close();
 
     assertEquals(0, pool.getNumActive());
 

diff --git a/src/test/java/redis/clients/jedis/tests/benchmark/PoolBenchmark.java b/src/test/java/redis/clients/jedis/tests/benchmark/PoolBenchmark.java
@@ -44,7 +44,7 @@ public void run() {
               final String key = "foo" + i;
               j.set(key, key);
               j.get(key);
-              pool.returnResource(j);
+              j.close();
             } catch (Exception e) {
               e.printStackTrace();
             }

diff --git a/src/test/java/redis/clients/jedis/tests/commands/AllKindOfValuesCommandsTest.java b/src/test/java/redis/clients/jedis/tests/commands/AllKindOfValuesCommandsTest.java
@@ -209,16 +209,6 @@ public void rename() {
     assertArrayEquals(bbar, bvalue);
   }
 
-  @Test
-  public void renameOldAndNewAreTheSame() {
-    jedis.set("foo", "bar");
-    jedis.rename("foo", "foo");
-
-    // Binary
-    jedis.set(bfoo, bbar);
-    jedis.rename(bfoo, bfoo);
-  }
-
   @Test
   public void renamenx() {
     jedis.set("foo", "bar");