Retry with backoff on cluster connection failures

pom.xml

@@ -92,6 +92,12 @@
 			<version>2.3.2</version>
 			<scope>test</scope>
 		</dependency>
+		<dependency>
+			<groupId>org.mockito</groupId>
+			<artifactId>mockito-core</artifactId>
+			<version>3.7.7</version>
+			<scope>test</scope>
+		</dependency>
 	</dependencies>
 
 	<distributionManagement>

src/main/java/redis/clients/jedis/BinaryJedis.java

@@ -293,6 +293,11 @@ public BinaryJedis(final JedisSocketFactory jedisSocketFactory, final JedisClien
     initializeFromClientConfig(clientConfig);
   }
 
+  @Override
+  public String toString() {
+    return "BinaryJedis{" + client + '}';
+  }
+
   public boolean isConnected() {
     return client.isConnected();
   }

src/main/java/redis/clients/jedis/Connection.java

@@ -81,6 +81,11 @@ public Connection(final JedisSocketFactory jedisSocketFactory) {
     this.soTimeout = jedisSocketFactory.getSoTimeout();
   }
 
+  @Override
+  public String toString() {
+    return "Connection{" + socketFactory + "}";
+  }
+
   public Socket getSocket() {
     return socket;
   }

src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java

@@ -220,4 +220,9 @@ public HostAndPortMapper getHostAndPortMapper() {
   public void setHostAndPortMapper(HostAndPortMapper hostAndPortMapper) {
     this.hostAndPortMapper = hostAndPortMapper;
   }
+
+  @Override
+  public String toString() {
+    return "DefaultJedisSocketFactory{" + hostAndPort.toString() + "}";
+  }
 }

src/main/java/redis/clients/jedis/JedisClusterCommand.java

@@ -1,8 +1,10 @@
 package redis.clients.jedis;
 
+import java.time.Duration;
+import java.time.Instant;
+import java.util.concurrent.TimeUnit;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import redis.clients.jedis.exceptions.JedisAskDataException;
 import redis.clients.jedis.exceptions.JedisClusterMaxAttemptsException;
 import redis.clients.jedis.exceptions.JedisClusterOperationException;
@@ -18,10 +20,20 @@ public abstract class JedisClusterCommand<T> {
 
   private final JedisClusterConnectionHandler connectionHandler;
   private final int maxAttempts;
+  private final Duration maxTotalRetriesDuration;
 
   public JedisClusterCommand(JedisClusterConnectionHandler connectionHandler, int maxAttempts) {
+    this(connectionHandler, maxAttempts, Duration.ofMillis((long) BinaryJedisCluster.DEFAULT_TIMEOUT * maxAttempts));
+  }
+
+  /**
+   * @param maxTotalRetriesDuration No more attempts after we have been trying for this long.
+   */
+  public JedisClusterCommand(JedisClusterConnectionHandler connectionHandler, int maxAttempts,
+      Duration maxTotalRetriesDuration) {
     this.connectionHandler = connectionHandler;
     this.maxAttempts = maxAttempts;
+    this.maxTotalRetriesDuration = maxTotalRetriesDuration;
   }
 
   public abstract T execute(Jedis connection);
@@ -85,7 +97,10 @@ public T runWithAnyNode() {
   }
 
   private T runWithRetries(final int slot) {
+    Instant deadline = Instant.now().plus(maxTotalRetriesDuration);
+
     JedisRedirectionException redirect = null;
+    int consecutiveConnectionFailures = 0;
     Exception lastException = null;
     for (int attemptsLeft = this.maxAttempts; attemptsLeft > 0; attemptsLeft--) {
       Jedis connection = null;
@@ -106,15 +121,18 @@ private T runWithRetries(final int slot) {
         throw jnrcne;
       } catch (JedisConnectionException jce) {
         lastException = jce;
+        ++consecutiveConnectionFailures;
         LOG.debug("Failed connecting to Redis: {}", connection, jce);
         // "- 1" because we just did one, but the attemptsLeft counter hasn't been decremented yet
-        handleConnectionProblem(attemptsLeft - 1);
-      } catch (JedisRedirectionException jre) {
-        // avoid updating lastException if it is a connection exception
-        if (lastException == null || lastException instanceof JedisRedirectionException) {
-          lastException = jre;
+        boolean reset = handleConnectionProblem(attemptsLeft - 1, consecutiveConnectionFailures, deadline);
+        if (reset) {
+          consecutiveConnectionFailures = 0;
+          redirect = null;
         }
+      } catch (JedisRedirectionException jre) {
+        lastException = jre;
         LOG.debug("Redirected by server to {}", jre.getTargetNode());
+        consecutiveConnectionFailures = 0;
         redirect = jre;
         // if MOVED redirection occurred,
         if (jre instanceof JedisMovedDataException) {
@@ -124,22 +142,69 @@ private T runWithRetries(final int slot) {
       } finally {
         releaseConnection(connection);
       }
+      if (Instant.now().isAfter(deadline)) {
+        throw new JedisClusterOperationException("Retry deadline exceeded");
+      }
     }
 
-    JedisClusterMaxAttemptsException maxAttemptsException
-        = new JedisClusterMaxAttemptsException("No more cluster attempts left.");
-    maxAttemptsException.addSuppressed(lastException);
-    throw maxAttemptsException;
+    throw new JedisClusterMaxAttemptsException("No more cluster attempts left.", lastException);
   }
 
-  private void handleConnectionProblem(int attemptsLeft) {
-    if (attemptsLeft <= 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.
-      // 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();
+  /**
+   * Related values should be reset if <code>TRUE</code> is returned.
+   *
+   * @param attemptsLeft
+   * @param consecutiveConnectionFailures
+   * @param doneDeadline
+   * @return true - if some actions are taken
+   * <br /> false - if no actions are taken
+   */
+  private boolean handleConnectionProblem(int attemptsLeft, int consecutiveConnectionFailures, Instant doneDeadline) {
+    if (this.maxAttempts < 3) {
+      // Since we only renew the slots cache after two consecutive connection
+      // failures (see consecutiveConnectionFailures above), we need to special
+      // case the situation where we max out after two or fewer attempts.
+      // Otherwise, on two or fewer max attempts, the slots cache would never be
+      // renewed.
+      if (attemptsLeft == 0) {
+        this.connectionHandler.renewSlotCache();
+        return true;
+      }
+      return false;
+    }
+
+    if (consecutiveConnectionFailures < 2) {
+      return false;
+    }
+
+    sleep(getBackoffSleepMillis(attemptsLeft, doneDeadline));
+    //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.
+    //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();
+    return true;
+  }
+
+  private static long getBackoffSleepMillis(int attemptsLeft, Instant deadline) {
+    if (attemptsLeft <= 0) {
+      return 0;
+    }
+
+    long millisLeft = Duration.between(Instant.now(), deadline).toMillis();
+    if (millisLeft < 0) {
+      // TODO: change to JedisClusterOperationException or a new sub-class of it
+      throw new JedisClusterMaxAttemptsException("Deadline exceeded");
+    }
+
+    return millisLeft / (attemptsLeft * (attemptsLeft + 1));
+  }
+
+  protected void sleep(long sleepMillis) {
+    try {
+      TimeUnit.MILLISECONDS.sleep(sleepMillis);
+    } catch (InterruptedException e) {
+      throw new JedisClusterOperationException(e);
     }
   }