format & clean-up

Author: ggivo

pom.xml

@@ -334,6 +334,7 @@
 					<configFile>${project.basedir}/hbase-formatter.xml</configFile>
 					<directories>
 						<directory>${project.basedir}/src/main/java/redis/clients/jedis/annots</directory>
+						<directory>${project.basedir}/src/test/java/redis/clients/jedis/failover</directory>
 					</directories>
 				</configuration>
 				<executions>

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

@@ -60,40 +60,6 @@ public class FailoverIntegrationTest {
   private static String JEDIS2_ID = "";
   private MultiClusterPooledConnectionProvider provider;
   private UnifiedJedis failoverClient;
-  
-  /**
-   * Creates a MultiClusterPooledConnectionProvider with standard configuration
-   * @return A configured provider
-   */
-  private MultiClusterPooledConnectionProvider createProvider() {
-    JedisClientConfig clientConfig = DefaultJedisClientConfig.builder()
-        .socketTimeoutMillis(RecommendedSettings.DEFAULT_TIMEOUT_MS)
-        .connectionTimeoutMillis(RecommendedSettings.DEFAULT_TIMEOUT_MS).build();
-
-    MultiClusterClientConfig failoverConfig = new MultiClusterClientConfig.Builder(
-        getClusterConfigs(clientConfig, endpoint1, endpoint2)).retryMaxAttempts(1)
-        .retryWaitDuration(1).circuitBreakerSlidingWindowType(COUNT_BASED)
-        .circuitBreakerSlidingWindowSize(1).circuitBreakerFailureRateThreshold(100)
-        .circuitBreakerSlidingWindowMinCalls(1).build();
-
-    return new MultiClusterPooledConnectionProvider(failoverConfig);
-  }
-  
-  /**
-   * Creates a UnifiedJedis client with customizable failover options
-   * @param provider The connection provider to use
-   * @param optionsCustomizer A function that customizes the failover options (can be null for defaults)
-   * @return A configured failover client
-   */
-  private UnifiedJedis createClient(MultiClusterPooledConnectionProvider provider, 
-      Function<FailoverOptions.Builder, FailoverOptions.Builder> optionsCustomizer) {
-    FailoverOptions.Builder builder = FailoverOptions.builder();
-    if (optionsCustomizer != null) {
-        builder = optionsCustomizer.apply(builder);
-    }
-    
-    return new UnifiedJedis(provider, builder.build());
-  }
 
   @BeforeAll
   public static void setupAdminClients() throws IOException {
@@ -119,33 +85,6 @@ public static void cleanupAdminClients() throws IOException {
     executor.shutdown();
   }
 
-  private static String getNodeId(UnifiedJedis client) {
-
-    return getNodeId(client.info("server"));
-  }
-
-  private static String getNodeId(String info) {
-
-    Matcher m = pattern.matcher(info);
-    if (m.find()) {
-      return m.group(1);
-    }
-    return null;
-  }
-
-  /**
-   * Generates a string of a specific byte size.
-   * @param byteSize The desired size in bytes
-   * @return A string of the specified byte size
-   */
-  private static String generateTestValue(int byteSize) {
-    StringBuilder value = new StringBuilder(byteSize);
-    for (int i = 0; i < byteSize; i++) {
-      value.append('x');
-    }
-    return value.toString();
-  }
-
   @BeforeEach
   public void setup() throws IOException {
     tp.getProxies().forEach(proxy -> {
@@ -184,12 +123,14 @@ public void cleanup() throws IOException {
 
   /**
    * Tests the automatic failover behavior when a Redis server becomes unavailable. This test
-   * verifies: 1. Initial connection to the first Redis server works correctly 2. When the first
-   * server is disabled, the first command throws a JedisConnectionException 3. The circuit breaker
-   * for the first endpoint transitions to OPEN state 4. Subsequent commands are automatically
-   * routed to the second available endpoint 5. When the second server is also disabled, all
-   * commands fail with JedisConnectionException 6. The circuit breaker for the second endpoint also
-   * transitions to OPEN state
+   * verifies:
+   * <ol>
+   * <li>Initial connection to the first Redis server works correctly</li>
+   * <li>Disable access, the first command throws</li>
+   * <li>Command failure is propagated to the caller</li>
+   * <li>CB transitions to OPEN, failover is initiated and following commands are sent to the next endpoint</li>
+   * <li>Second server is also disabled, all commands fail with JedisConnectionException and error is propagated to the caller</li>
+   * </ol>
    */
   @Test
   public void testAutomaticFailoverWhenServerBecomesUnavailable() throws Exception {
@@ -199,27 +140,26 @@ public void testAutomaticFailoverWhenServerBecomesUnavailable() throws Exception
     redisProxy1.disable();
 
     // Endpoint 1 not available
-    // 1. First call should  should throw JedisConnectionException and trigger failover
-    // 1.1. Endpoint1  CB transitions to  open
-    // 2. Subsequent calls should be routed to Endpoint2
+    // 1. First call should throw JedisConnectionException and trigger failover
+    // 2. Endpoint 1 CB transitions to OPEN
+    // 3. Subsequent calls should be routed to Endpoint 2
     assertThrows(JedisConnectionException.class, () -> failoverClient.info("server"));
 
-    // Check that the circuit breaker for Endpoint 1 is open
     assertThat(provider.getCluster(1).getCircuitBreaker().getState(),
-        equalTo(CircuitBreaker.State.OPEN));
+      equalTo(CircuitBreaker.State.OPEN));
 
     // Check that the failoverClient is now using Endpoint 2
     assertThat(getNodeId(failoverClient.info("server")), equalTo(JEDIS2_ID));
 
-    // Disable also second proxy
+    // Disable second access to Endpoint 2
     redisProxy2.disable();
 
-    // Endpoint1 and Endpoint2 are not available,
+    // Endpoint1 and Endpoint2 are NOT available,
     assertThrows(JedisConnectionException.class, () -> failoverClient.info("server"));
     assertThat(provider.getCluster(2).getCircuitBreaker().getState(),
-        equalTo(CircuitBreaker.State.OPEN));
+      equalTo(CircuitBreaker.State.OPEN));
 
-    // and since no other nodes are available, it should throw an exception for subsequent calls
+    // and since no other nodes are available, it should propagate the errors to the caller subsequent calls
     assertThrows(JedisConnectionException.class, () -> failoverClient.info("server"));
   }
 
@@ -246,46 +186,44 @@ public void testManualFailoverInflightCommandsCompleteGracefully()
       throws ExecutionException, InterruptedException {
 
     assertThat(getNodeId(failoverClient.info("server")), equalTo(JEDIS1_ID));
-    Future<List<String>> blpop = executor.submit(() -> {
-      try {
-        // This command will block until a value is pushed to the list or timeout occurs
-        // We will trigger failover while this command is blocking
-        return failoverClient.blpop(1000, "test-list");
-      } catch (Exception e) {
-        throw new RuntimeException(e);
-      }
-    });
+
+    // We will trigger failover while this command is in-flight
+    Future<List<String>> blpop = executor.submit(() -> failoverClient.blpop(1000, "test-list"));
 
     provider.setActiveMultiClusterIndex(2);
 
-    // new command should be executed against the new endpoint
+    // After the manual failover, commands should be executed against Endpoint 2
     assertThat(getNodeId(failoverClient.info("server")), equalTo(JEDIS2_ID));
-    //Since failover was manually triggered and there were no errors
-    // previous endpoint CB should be still in CLOSED
+
+    // Failover was manually triggered, and there were no errors
+    // previous endpoint CB should still be in CLOSED state
     assertThat(provider.getCluster(1).getCircuitBreaker().getState(),
-        equalTo(CircuitBreaker.State.CLOSED));
+      equalTo(CircuitBreaker.State.CLOSED));
 
     jedis1.rpush("test-list", "somevalue");
 
     assertThat(blpop.get(), equalTo(Arrays.asList("test-list", "somevalue")));
   }
 
   /**
-   * Verify that in-flight commands during manual failover fail gracefully with an error will
-   * propagate the error to the caller and will toggle CB to OPEN state.
+   * Verify that in-flight commands that complete with error during manual failover will
+   * propagate the error to the caller and toggle CB to OPEN state.
    */
   @Test
   public void testManualFailoverInflightCommandsWithErrorsPropagateError() throws Exception {
     assertThat(getNodeId(failoverClient.info("server")), equalTo(JEDIS1_ID));
 
+    // We will trigger failover while this command is in-flight
     Future<List<String>> blpop = executor.submit(() -> failoverClient.blpop(10000, "test-list-1"));
 
     // trigger failover manually
     provider.setActiveMultiClusterIndex(2);
     Future<String> infoCmd = executor.submit(() -> failoverClient.info("server"));
-    // new command should be executed against the new endpoint
+
+    // After the manual failover, commands should be executed against Endpoint 2
     assertThat(getNodeId(infoCmd.get()), equalTo(JEDIS2_ID));
-    // Disable redisProxy1 to simulate an error
+
+    // Disable redisProxy1 to drop active connections and trigger an error
     redisProxy1.disable();
 
     // previously submitted command should fail with JedisConnectionException
@@ -294,9 +232,9 @@ public void testManualFailoverInflightCommandsWithErrorsPropagateError() throws
 
     // Check that the circuit breaker for Endpoint 1 is open after the error
     assertThat(provider.getCluster(1).getCircuitBreaker().getState(),
-        equalTo(CircuitBreaker.State.OPEN));
+      equalTo(CircuitBreaker.State.OPEN));
 
-    // Ensure that active cluster is still Endpoint 2
+    // Ensure that the active cluster is still Endpoint 2
     assertThat(getNodeId(failoverClient.info("server")), equalTo(JEDIS2_ID));
   }
 
@@ -305,68 +243,130 @@ public void testManualFailoverInflightCommandsWithErrorsPropagateError() throws
    */
   @Test
   public void testInflightCommandsAreRetriedAfterFailover() throws Exception {
-    // Create a custom provider and client with retry enabled for this specific test
+
     MultiClusterPooledConnectionProvider customProvider = createProvider();
-    
-    try (UnifiedJedis customClient = createClient(customProvider, 
-        builder -> builder.retryFailedInflightCommands(true))) {
-      
+
+    // Create a custom client with retryFailedInflightCommands enabled for this specific test
+    try (UnifiedJedis customClient = createClient(customProvider,
+      builder -> builder.retryFailedInflightCommands(true))) {
+
       assertThat(getNodeId(customClient.info("server")), equalTo(JEDIS1_ID));
-      Future<List<String>> blpop = executor.submit(() -> {
-        try {
-          // This command will block until a value is pushed to the list or timeout occurs
-          // We will trigger failover while this command is blocking
-          return customClient.blpop(10000, "test-list-1");
-        } catch (Exception e) {
-          throw new RuntimeException(e);
-        }
-      });
 
-      // info command will return more than 100 bytes so we simulate connection dropped
-      redisProxy1.toxics().limitData("simulate-socket-failure", ToxicDirection.UPSTREAM, 100);
-      assertThrows(JedisConnectionException.class, () -> customClient.set("test-key", generateTestValue(150)));
+      // We will trigger failover while this command is in-flight
+      Future<List<String>> blpop = executor.submit(() -> customClient.blpop(10000, "test-list-1"));
 
-      // Disable redisProxy1 to enforce current blpop command failure
-      redisProxy1.disable();
+
+      // Simulate error by sending more than 100 bytes. This causes the connection close, and triggers
+      // failover
+      redisProxy1.toxics().limitData("simulate-socket-failure", ToxicDirection.UPSTREAM, 100);
+      assertThrows(JedisConnectionException.class,
+        () -> customClient.set("test-key", generateTestValue(150)));
 
       // Check that the circuit breaker for Endpoint 1 is open
       assertThat(customProvider.getCluster(1).getCircuitBreaker().getState(),
-          equalTo(CircuitBreaker.State.OPEN));
+        equalTo(CircuitBreaker.State.OPEN));
+
+      // Disable redisProxy1 to enforce the current blpop command failure
+      redisProxy1.disable();
 
       customClient.rpush("test-list-1", "somevalue");
       assertThat(blpop.get(), equalTo(Arrays.asList("test-list-1", "somevalue")));
     }
   }
-  
+
   /**
    * Tests that in-flight commands are not retried after automatic failover when retry is disabled.
    */
   @Test
   public void testInflightCommandsAreNotRetriedAfterFailover() throws Exception {
     // Create a custom provider and client with retry disabled for this specific test
     MultiClusterPooledConnectionProvider customProvider = createProvider();
-    
-    try (UnifiedJedis customClient = createClient(customProvider, 
-        builder -> builder.retryFailedInflightCommands(false))) {
-      
+
+    try (UnifiedJedis customClient = createClient(customProvider,
+      builder -> builder.retryFailedInflightCommands(false))) {
+
       assertThat(getNodeId(customClient.info("server")), equalTo(JEDIS1_ID));
-      Future<List<String>> blpop = executor.submit(() -> customClient.blpop(10000, "test-list-2"));
+      Future<List<String>> blpop = executor.submit(() -> customClient.blpop(1000, "test-list-2"));
 
-      // info command will return more than 100 bytes so we simulate connection dropped
+      // Simulate error by sending more than 100 bytes. This causes connection close, and triggers
+      // failover
       redisProxy1.toxics().limitData("simulate-socket-failure", ToxicDirection.UPSTREAM, 100);
-      assertThrows(JedisConnectionException.class, () -> customClient.set("test-key", generateTestValue(150)));
-
-      // Disable redisProxy1 to enforce current blpop command failure
-      redisProxy1.disable();
+      assertThrows(JedisConnectionException.class,
+        () -> customClient.set("test-key", generateTestValue(150)));
 
       // Check that the circuit breaker for Endpoint 1 is open
       assertThat(customProvider.getCluster(1).getCircuitBreaker().getState(),
-          equalTo(CircuitBreaker.State.OPEN));
+        equalTo(CircuitBreaker.State.OPEN));
+
+      // Disable redisProxy1 to enforce the current blpop command failure
+      redisProxy1.disable();
 
-      // The blpop command should fail since retry is disabled
-      customClient.rpush("test-list-2", "somevalue");
-      ExecutionException exception = assertThrows(ExecutionException.class, () -> blpop.get(1, TimeUnit.SECONDS));
+      // The in-flight command should fail since the retry is disabled
+      ExecutionException exception = assertThrows(ExecutionException.class,
+        () -> blpop.get(1, TimeUnit.SECONDS));
       assertThat(exception.getCause(), instanceOf(JedisConnectionException.class));
     }
   }
+
+  private static String getNodeId(UnifiedJedis client) {
+
+    return getNodeId(client.info("server"));
+  }
+
+  private static String getNodeId(String info) {
+
+    Matcher m = pattern.matcher(info);
+    if (m.find()) {
+      return m.group(1);
+    }
+    return null;
+  }
+
+  /**
+   * Generates a string of a specific byte size.
+   * @param byteSize The desired size in bytes
+   * @return A string of the specified byte size
+   */
+  private static String generateTestValue(int byteSize) {
+    StringBuilder value = new StringBuilder(byteSize);
+    for (int i = 0; i < byteSize; i++) {
+      value.append('x');
+    }
+    return value.toString();
+  }
+
+  /**
+   * Creates a MultiClusterPooledConnectionProvider with standard configuration
+   * @return A configured provider
+   */
+  private MultiClusterPooledConnectionProvider createProvider() {
+    JedisClientConfig clientConfig = DefaultJedisClientConfig.builder()
+        .socketTimeoutMillis(RecommendedSettings.DEFAULT_TIMEOUT_MS)
+        .connectionTimeoutMillis(RecommendedSettings.DEFAULT_TIMEOUT_MS).build();
+
+    MultiClusterClientConfig failoverConfig = new MultiClusterClientConfig.Builder(
+        getClusterConfigs(clientConfig, endpoint1, endpoint2)).retryMaxAttempts(1)
+            .retryWaitDuration(1).circuitBreakerSlidingWindowType(COUNT_BASED)
+            .circuitBreakerSlidingWindowSize(1).circuitBreakerFailureRateThreshold(100)
+            .circuitBreakerSlidingWindowMinCalls(1).build();
+
+    return new MultiClusterPooledConnectionProvider(failoverConfig);
+  }
+
+  /**
+   * Creates a UnifiedJedis client with customizable failover options
+   * @param provider The connection provider to use
+   * @param optionsCustomizer A function that customizes the failover options (can be null for
+   *          defaults)
+   * @return A configured failover client
+   */
+  private UnifiedJedis createClient(MultiClusterPooledConnectionProvider provider,
+      Function<FailoverOptions.Builder, FailoverOptions.Builder> optionsCustomizer) {
+    FailoverOptions.Builder builder = FailoverOptions.builder();
+    if (optionsCustomizer != null) {
+      builder = optionsCustomizer.apply(builder);
+    }
+
+    return new UnifiedJedis(provider, builder.build());
+  }
 }