From b1aef2717e8017ae3eb54887f5691b9d8986c9bf Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 31 Aug 2017 15:05:33 +0200 Subject: [PATCH] DATAREDIS-684 - Release Jedis cluster node connections with close(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now release Jedis cluster node connections with Jedis.close() to the pool instead of Pool.returnResource(…). The close() method itself checks whether the connection was broken and if so, the connection gets destroyed. Destroying broken connections prevents the pool from supplying broken connections on borrow when testOnBorrow is disabled. The only case where we return broken resources ourselves to the Pool is when we discover a broken connection ourselves: If we run into a NullPointerException or RedisConnectionFailureException, then we consider a connection is broken. Original Pull Request: #271 --- .../jedis/JedisClusterConnection.java | 4 +-- .../connection/jedis/JedisConnection.java | 30 +++++++++---------- .../connection/ClusterSlotHashUtilsTests.java | 19 ++++++------ .../JedisClusterConnectionUnitTests.java | 4 +++ 4 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java index 7a8d81fc3..9620cacb0 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java @@ -840,7 +840,7 @@ public class JedisClusterConnection implements DefaultedRedisClusterConnection { */ @Override public void returnResourceForSpecificNode(RedisClusterNode node, Object client) { - getResourcePoolForSpecificNode(node).returnResource((Jedis) client); + ((Jedis) client).close(); } } @@ -897,7 +897,7 @@ public class JedisClusterConnection implements DefaultedRedisClusterConnection { errors.put(entry.getKey(), ex); } finally { if (jedis != null) { - entry.getValue().returnResource(jedis); + jedis.close(); } } } diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java index e18a7f214..d1c7faaa0 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java @@ -343,29 +343,27 @@ public class JedisConnection extends AbstractRedisConnection { */ @Override public void close() throws DataAccessException { + super.close(); + // return the connection to the pool if (pool != null) { - if (!broken) { + + if (broken) { + pool.returnBrokenResource(jedis); + } else { + // reset the connection try { if (dbIndex > 0) { jedis.select(0); } - pool.returnResource(jedis); return; } catch (Exception ex) { - DataAccessException dae = convertJedisAccessException(ex); - if (broken) { - pool.returnBrokenResource(jedis); - } else { - pool.returnResource(jedis); - } - throw dae; + throw convertJedisAccessException(ex); + } finally { + jedis.close(); } - } else { - pool.returnBrokenResource(jedis); - return; } } // else close the connection normally (doing the try/catch dance) @@ -373,13 +371,13 @@ public class JedisConnection extends AbstractRedisConnection { if (isQueueing()) { try { client.quit(); - } catch (Exception ex) { - exc = ex; + } catch (Exception o_O) { + // ignore exception } try { client.disconnect(); - } catch (Exception ex) { - exc = ex; + } catch (Exception o_O) { + // ignore exception } return; } diff --git a/src/test/java/org/springframework/data/redis/connection/ClusterSlotHashUtilsTests.java b/src/test/java/org/springframework/data/redis/connection/ClusterSlotHashUtilsTests.java index b0322d005..52b7f4467 100644 --- a/src/test/java/org/springframework/data/redis/connection/ClusterSlotHashUtilsTests.java +++ b/src/test/java/org/springframework/data/redis/connection/ClusterSlotHashUtilsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2015 the original author or authors. + * Copyright 2015-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,6 +17,11 @@ package org.springframework.data.redis.connection; import static org.junit.Assert.*; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.Jedis; +import redis.clients.jedis.JedisCluster; +import redis.clients.jedis.JedisPool; + import java.io.IOException; import java.util.Collections; import java.util.Random; @@ -26,13 +31,9 @@ import org.junit.Test; import org.springframework.data.redis.test.util.RedisClusterRule; import org.springframework.util.StringUtils; -import redis.clients.jedis.HostAndPort; -import redis.clients.jedis.Jedis; -import redis.clients.jedis.JedisCluster; -import redis.clients.jedis.JedisPool; - /** * @author Christoph Strobl + * @author Mark Paluch */ public class ClusterSlotHashUtilsTests { @@ -58,7 +59,7 @@ public class ClusterSlotHashUtilsTests { serverSlot.intValue(), slot); } - pool.returnResource(jedis); + jedis.close(); } finally { if (cluster != null) { cluster.close(); @@ -100,7 +101,7 @@ public class ClusterSlotHashUtilsTests { serverSlot1.intValue(), slot1); } - pool.returnResource(jedis); + jedis.close(); } finally { if (cluster != null) { cluster.close(); @@ -110,7 +111,7 @@ public class ClusterSlotHashUtilsTests { /** * Generate random string using ascii chars {@code ' ' (space)} to {@code 'z'}. Explicitly skipping { and }. - * + * * @return */ private String randomString() { diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java index 657e7e8bf..ec1c4ff3e 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java @@ -154,6 +154,7 @@ public class JedisClusterConnectionUnitTests { verify(con2Mock, times(1)).clusterReplicate(CLUSTER_NODE_1.getId()); verify(con1Mock, times(1)).clusterNodes(); + verify(con1Mock, times(1)).close(); verifyZeroInteractions(con1Mock); } @@ -314,7 +315,9 @@ public class JedisClusterConnectionUnitTests { connection.time(CLUSTER_NODE_2); verify(con2Mock, times(1)).time(); + verify(con2Mock, times(1)).close(); verify(con1Mock, times(1)).clusterNodes(); + verify(con1Mock, times(1)).close(); verifyZeroInteractions(con1Mock, con3Mock); } @@ -334,6 +337,7 @@ public class JedisClusterConnectionUnitTests { connection.resetConfigStats(CLUSTER_NODE_2); verify(con2Mock, times(1)).configResetStat(); + verify(con2Mock, times(1)).close(); verify(con1Mock, never()).configResetStat(); verify(con3Mock, never()).configResetStat(); }