[automatic failover] Improve failover resilience by evaluating circuit breaker before retries (#4178)

* fix: Change evaluation order of circuit breaker and retry mechanisms

This commit modifies the order in which circuit breaker and retry mechanisms
are evaluated during failover scenarios. Previously, the circuit breaker was
applied after all retries were exhausted, which could delay failover in
unstable network conditions. Now, the circuit breaker counts each connection
error separately, even during retry attempts.

* format

* format

* use java-8 compatible formater version

* fix AutomaticFailoverTest

* formating

Author: ggivo

formatter-pom.xml

@@ -20,7 +20,7 @@
 			<plugin>
 				<groupId>net.revelc.code.formatter</groupId>
 				<artifactId>formatter-maven-plugin</artifactId>
-				<version>2.23.0</version>
+				<version>2.16.0</version>
 				<configuration>
 					<configFile>${project.basedir}/hbase-formatter.xml</configFile>
 				</configuration>

pom.xml

@@ -335,6 +335,7 @@
 					<directories>
 						<directory>${project.basedir}/src/main/java/redis/clients/jedis/annots</directory>
 						<directory>${project.basedir}/src/main/java/redis/clients/jedis/mcf</directory>
+						<directory>${project.basedir}/src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java</directory>
 						<directory>${project.basedir}/src/test/java/redis/clients/jedis/failover</directory>
 					</directories>
 				</configuration>

src/main/java/redis/clients/jedis/mcf/CircuitBreakerCommandExecutor.java

@@ -35,8 +35,8 @@ public <T> T executeCommand(CommandObject<T> commandObject) {
     DecorateSupplier<T> supplier = Decorators
         .ofSupplier(() -> this.handleExecuteCommand(commandObject, cluster));
 
-    supplier.withRetry(cluster.getRetry());
     supplier.withCircuitBreaker(cluster.getCircuitBreaker());
+    supplier.withRetry(cluster.getRetry());
     supplier.withFallback(provider.getFallbackExceptionList(),
       e -> this.handleClusterFailover(commandObject, cluster.getCircuitBreaker()));
 

src/test/java/redis/clients/jedis/failover/FailoverIntegrationTest.java

@@ -272,4 +272,62 @@ public void testManualFailoverInflightCommandsWithErrorsPropagateError() throws
     assertThat(getNodeId(failoverClient.info("server")), equalTo(JEDIS2_ID));
   }
 
-}
+  /**
+   * Tests that the CircuitBreaker counts each command error separately, and not just after all
+   * retries are exhausted. This ensures that the circuit breaker opens based on the actual number
+   * of send commands with failures, and not based on the number of logical operations.
+   */
+  @Test
+  public void testCircuitBreakerCountsEachConnectionErrorSeparately() throws IOException {
+    MultiClusterClientConfig failoverConfig = new MultiClusterClientConfig.Builder(
+        getClusterConfigs(
+          DefaultJedisClientConfig.builder()
+              .socketTimeoutMillis(RecommendedSettings.DEFAULT_TIMEOUT_MS)
+              .connectionTimeoutMillis(RecommendedSettings.DEFAULT_TIMEOUT_MS).build(),
+          endpoint1, endpoint2)).retryMaxAttempts(2).retryWaitDuration(1)
+              .circuitBreakerSlidingWindowType(COUNT_BASED).circuitBreakerSlidingWindowSize(3)
+              .circuitBreakerFailureRateThreshold(50) // 50% failure
+                                                      // rate threshold
+              .circuitBreakerSlidingWindowMinCalls(3).build();
+
+    MultiClusterPooledConnectionProvider provider = new MultiClusterPooledConnectionProvider(
+        failoverConfig);
+    try (UnifiedJedis client = new UnifiedJedis(provider)) {
+      // Verify initial connection to first endpoint
+      assertThat(getNodeId(client.info("server")), equalTo(JEDIS1_ID));
+
+      // Disable first endpoint
+      redisProxy1.disable();
+
+      // First command should fail and OPEN the circuit breaker immediately
+      //
+      // If CB is applied after retries:
+      // - It would take 2 commands to OPEN CB (error is propagated for both commands)
+      // - Failover to the next Endpoint happens on the 3rd command
+      //
+      // If CB is applied before retries:
+      // - It should open after just 1 command with retries
+      // - CB is OPEN after the 2nd retry of the first command
+      // - Failover to the next Endpoint happens on the 2nd command
+      //
+      // This test verifies the second case by checking that:
+      // 1. CB opens after the first command (with retries)
+      // 2. The second command is routed to the second endpoint
+      // Command 1
+      assertThrows(JedisConnectionException.class, () -> client.info("server"));
+
+      // Circuit breaker should be open after just one command with retries
+      assertThat(provider.getCluster(1).getCircuitBreaker().getState(),
+        equalTo(CircuitBreaker.State.OPEN));
+
+      // Next command should be routed to the second endpoint
+      // Command 2
+      assertThat(getNodeId(client.info("server")), equalTo(JEDIS2_ID));
+
+      // Command 3
+      assertThat(getNodeId(client.info("server")), equalTo(JEDIS2_ID));
+
+    }
+  }
+
+}

src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java

@@ -21,6 +21,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 @Tag("failover")
@@ -91,37 +92,38 @@ public void transactionWithSwitch() {
 
   @Test
   public void commandFailover() {
-    int slidingWindowMinCalls = 10;
-    int slidingWindowSize = 10;
+    int slidingWindowMinCalls = 6;
+    int slidingWindowSize = 6;
 
     MultiClusterClientConfig.Builder builder = new MultiClusterClientConfig.Builder(
         getClusterConfigs(clientConfig, hostPortWithFailure, workingEndpoint.getHostAndPort()))
+        .retryMaxAttempts(3) // Default is 3
         .circuitBreakerSlidingWindowMinCalls(slidingWindowMinCalls)
         .circuitBreakerSlidingWindowSize(slidingWindowSize);
 
     RedisFailoverReporter failoverReporter = new RedisFailoverReporter();
-    MultiClusterPooledConnectionProvider cacheProvider = new MultiClusterPooledConnectionProvider(builder.build());
-    cacheProvider.setClusterFailoverPostProcessor(failoverReporter);
+    MultiClusterPooledConnectionProvider connectionProvider = new MultiClusterPooledConnectionProvider(builder.build());
+    connectionProvider.setClusterFailoverPostProcessor(failoverReporter);
 
-    UnifiedJedis jedis = new UnifiedJedis(cacheProvider);
+    UnifiedJedis jedis = new UnifiedJedis(connectionProvider);
 
     String key = "hash-" + System.nanoTime();
     log.info("Starting calls to Redis");
     assertFalse(failoverReporter.failedOver);
-    for (int attempt = 0; attempt < 10; attempt++) {
-      try {
-        jedis.hset(key, "f1", "v1");
-      } catch (JedisConnectionException jce) {
-        //
-      }
-      assertFalse(failoverReporter.failedOver);
-    }
+    // First call fails - will be retried 3 times
+    // this will increase the CircuitBreaker failure count to 3
+    assertThrows(JedisConnectionException.class, () -> jedis.hset(key, "c1", "v1"));
 
+    // Second call fails - will be retried 3 times
+    // this will increase the CircuitBreaker failure count to 6
     // should failover now
-    jedis.hset(key, "f1", "v1");
+    assertThrows(JedisConnectionException.class, () ->jedis.hset(key, "c2", "v1"));
+
+    // CB is in OPEN state now, next call should cause failover
+    assertEquals(1L, jedis.hset(key, "c3", "v1"));
     assertTrue(failoverReporter.failedOver);
 
-    assertEquals(Collections.singletonMap("f1", "v1"), jedis.hgetAll(key));
+    assertEquals(Collections.singletonMap("c3", "v1"), jedis.hgetAll(key));
     jedis.flushAll();
 
     jedis.close();