From 968dab6bcffc94bacf652061fb4884b15857651d Mon Sep 17 00:00:00 2001 From: Louis Morgan Date: Mon, 12 Dec 2016 16:46:57 +0000 Subject: [PATCH 1/2] Call renewSlots() before final retry on JedisConnectionException --- .../redis/clients/jedis/JedisClusterCommand.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/redis/clients/jedis/JedisClusterCommand.java b/src/main/java/redis/clients/jedis/JedisClusterCommand.java index 6134e30e8a..9f63b1df0c 100644 --- a/src/main/java/redis/clients/jedis/JedisClusterCommand.java +++ b/src/main/java/redis/clients/jedis/JedisClusterCommand.java @@ -128,15 +128,15 @@ private T runWithRetries(byte[] key, int attempts, boolean tryRandomNode, boolea releaseConnection(connection); connection = null; - if (attempts <= 1) { - //We need this because if node is not reachable anymore - we need to finally initiate slots renewing, - //or we can stuck with cluster state without one node in opposite case. - //But now if maxAttempts = 1 or 2 we will do it too often. For each time-outed request. + int attemptsRemaining = attempts - 1; + if (attemptsRemaining == 1 && maxAttempts > 2) { + // May be that the node is not reachable anymore, try calling cluster slots before our final retry + // Only do this if maxAttempts > 2 to avoid calling cluster slots on any connection error //TODO make tracking of successful/unsuccessful operations for node - do renewing only //if there were no successful responses from this node last few seconds this.connectionHandler.renewSlotCache(); - - //no more redirections left, throw original exception, not JedisClusterMaxRedirectionsException, because it's not MOVED situation + } else if (attemptsRemaining <= 0) { + // No more redirections left, throw original exception, not JedisClusterMaxRedirectionsException, because it's not MOVED situation throw jce; } From e2e76207d06fa57d67417732b135dfae73526efa Mon Sep 17 00:00:00 2001 From: Louis Morgan Date: Mon, 12 Dec 2016 16:54:09 +0000 Subject: [PATCH 2/2] Add previously failing unit test --- pom.xml | 7 +++ .../jedis/JedisClusterCommandTest.java | 53 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 src/test/java/redis/clients/jedis/JedisClusterCommandTest.java diff --git a/pom.xml b/pom.xml index c7be106096..60e00db51c 100644 --- a/pom.xml +++ b/pom.xml @@ -59,6 +59,13 @@ jar test + + org.mockito + mockito-core + 2.3.0 + jar + test + org.apache.commons commons-pool2 diff --git a/src/test/java/redis/clients/jedis/JedisClusterCommandTest.java b/src/test/java/redis/clients/jedis/JedisClusterCommandTest.java new file mode 100644 index 0000000000..02e148c5f9 --- /dev/null +++ b/src/test/java/redis/clients/jedis/JedisClusterCommandTest.java @@ -0,0 +1,53 @@ +package redis.clients.jedis; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InOrder; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; +import redis.clients.jedis.exceptions.JedisConnectionException; + +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.when; + +/** + * @author Louis Morgan + */ +@RunWith(MockitoJUnitRunner.class) +public class JedisClusterCommandTest { + + @Mock + private Jedis connection; + + @Mock + private JedisClusterConnectionHandler connectionHandler; + + @Test + public void onConnectionExceptionClusterRenewsSlotCacheAndThenRetries() { + when(connectionHandler.getConnectionFromSlot(anyInt())).thenReturn(connection); + when(connection.get("foo")).thenThrow(new JedisConnectionException("Command failed")); + + try { + new JedisClusterCommand(connectionHandler, 3) { + @Override + public String execute(Jedis connection) { + return connection.get("foo"); + } + }.run("foo"); + } catch (JedisConnectionException expected) { + // We expect this to be thrown + } + + // maxAttempts is 3, so we expect 2 attempts to get the key, followed by a call to + // renewSlotCache(), followed by a further attempt to get the key + InOrder inOrder = Mockito.inOrder(connection, connectionHandler); + inOrder.verify(connection).get("foo"); + inOrder.verify(connection).close(); + inOrder.verify(connection).get("foo"); + inOrder.verify(connection).close(); + inOrder.verify(connectionHandler).renewSlotCache(); + inOrder.verify(connection).get("foo"); + inOrder.verify(connection).close(); + } +}