From fc07828b2f79cdbab7daf50e2d73ea06cc864b10 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 b7602a18f..1dce0800d 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 @@ -4167,7 +4167,7 @@ public class JedisClusterConnection implements RedisClusterConnection { */ @Override public void returnResourceForSpecificNode(RedisClusterNode node, Object client) { - getResourcePoolForSpecificNode(node).returnResource((Jedis) client); + ((Jedis) client).close(); } } @@ -4224,7 +4224,7 @@ public class JedisClusterConnection implements RedisClusterConnection { 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 211877cc9..cff136ec7 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 @@ -284,29 +284,27 @@ public class JedisConnection extends AbstractRedisConnection { } 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) @@ -314,13 +312,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 57ee613e1..53f5567d5 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 @@ -150,6 +150,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); } @@ -310,7 +311,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); } @@ -330,6 +333,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(); }