Skip to content

Commit ee1021d

Browse files
ggivoggivo
committed
Add tests: EchoStrategy does recover on connection error
1 parent c426e81 commit ee1021d

File tree

4 files changed

+182
-3
lines changed

4 files changed

+182
-3
lines changed

‎pom.xml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,8 @@
340340
<include>src/main/java/redis/clients/jedis/annots/*.java</include>
341341
<include>src/main/java/redis/clients/jedis/mcf/*.java</include>
342342
<include>src/test/java/redis/clients/jedis/failover/*.java</include>
343+
<include>src/test/java/redis/clients/jedis/mcf/EchoStrategyIntegrationTest.java</include>
344+
<include>src/test/java/redis/clients/jedis/mcf/HealthCheckTest.java</include>
343345
<include>src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java</include>
344346
<include>src/main/java/redis/clients/jedis/providers/MultiClusterPooledConnectionProvider.java</include>
345347
<include>src/main/java/redis/clients/jedis/MultiClusterClientConfig.java</include>

‎src/main/java/redis/clients/jedis/mcf/EchoStrategy.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,11 @@ public int minConsecutiveSuccessCount() {
4141

4242
@Override
4343
public HealthStatus doHealthCheck(Endpoint endpoint) {
44-
return "HealthCheck".equals(jedis.echo("HealthCheck")) ? HealthStatus.HEALTHY : HealthStatus.UNHEALTHY;
44+
try {
45+
return "HealthCheck".equals(jedis.echo("HealthCheck")) ? HealthStatus.HEALTHY : HealthStatus.UNHEALTHY;
46+
} catch (Exception e) {
47+
return HealthStatus.UNHEALTHY;
48+
}
4549
}
4650

4751
@Override

‎src/test/java/redis/clients/jedis/mcf/EchoStrategyIntegrationTest.java‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
package redis.clients.jedis.mcf;
2+
3+
import eu.rekawek.toxiproxy.Proxy;
4+
import eu.rekawek.toxiproxy.ToxiproxyClient;
5+
import eu.rekawek.toxiproxy.model.ToxicDirection;
6+
import org.junit.jupiter.api.AfterAll;
7+
import org.junit.jupiter.api.BeforeAll;
8+
import org.junit.jupiter.api.BeforeEach;
9+
import org.junit.jupiter.api.Test;
10+
import org.junit.jupiter.api.Tag;
11+
import redis.clients.jedis.DefaultJedisClientConfig;
12+
import redis.clients.jedis.EndpointConfig;
13+
import redis.clients.jedis.HostAndPort;
14+
import redis.clients.jedis.HostAndPorts;
15+
import redis.clients.jedis.JedisClientConfig;
16+
17+
import java.io.IOException;
18+
19+
import static org.junit.jupiter.api.Assertions.*;
20+
21+
@Tag("failover")
22+
public class EchoStrategyIntegrationTest {
23+
24+
private static final EndpointConfig endpoint = HostAndPorts.getRedisEndpoint("redis-failover-1");
25+
private static final HostAndPort proxyHostAndPort = endpoint.getHostAndPort();
26+
private static final ToxiproxyClient tp = new ToxiproxyClient("localhost", 8474);
27+
private static Proxy redisProxy;
28+
29+
@BeforeAll
30+
public static void setupProxy() throws IOException {
31+
if (tp.getProxyOrNull("redis-health-test") != null) {
32+
tp.getProxy("redis-health-test").delete();
33+
}
34+
redisProxy = tp.createProxy("redis-health-test", "0.0.0.0:29379", "redis-failover-1:9379");
35+
}
36+
37+
@AfterAll
38+
public static void cleanupProxy() throws IOException {
39+
if (redisProxy != null) {
40+
redisProxy.delete();
41+
}
42+
}
43+
44+
@BeforeEach
45+
public void resetProxy() throws IOException {
46+
redisProxy.enable();
47+
redisProxy.toxics().getAll().forEach(toxic -> {
48+
try {
49+
toxic.remove();
50+
} catch (IOException e) {
51+
throw new RuntimeException(e);
52+
}
53+
});
54+
}
55+
56+
@Test
57+
public void testEchoStrategyRecoversAfterDisconnect() throws Exception {
58+
JedisClientConfig config = DefaultJedisClientConfig.builder().socketTimeoutMillis(1000)
59+
.connectionTimeoutMillis(1000).build();
60+
61+
try (EchoStrategy strategy = new EchoStrategy(proxyHostAndPort, config, 1000, 500, 1)) {
62+
63+
// Initial health check should work
64+
HealthStatus initialStatus = strategy.doHealthCheck(proxyHostAndPort);
65+
assertEquals(HealthStatus.HEALTHY, initialStatus);
66+
67+
// Disable the proxy to simulate network failure
68+
redisProxy.disable();
69+
70+
// Health check should now fail - this will expose the bug
71+
HealthStatus statusAfterDisable = strategy.doHealthCheck(proxyHostAndPort);
72+
assertEquals(HealthStatus.UNHEALTHY, statusAfterDisable);
73+
74+
// Re-enable proxy
75+
redisProxy.enable();
76+
// Health check should recover
77+
HealthStatus statusAfterEnable = strategy.doHealthCheck(proxyHostAndPort);
78+
assertEquals(HealthStatus.HEALTHY, statusAfterEnable);
79+
}
80+
81+
}
82+
83+
@Test
84+
public void testEchoStrategyWithConnectionTimeout() throws Exception {
85+
JedisClientConfig config = DefaultJedisClientConfig.builder().socketTimeoutMillis(100)
86+
.connectionTimeoutMillis(100).build();
87+
88+
try (EchoStrategy strategy = new EchoStrategy(proxyHostAndPort, config, 1000, 300, 1)) {
89+
90+
// Initial health check should work
91+
assertEquals(HealthStatus.HEALTHY, strategy.doHealthCheck(proxyHostAndPort));
92+
93+
// Add latency toxic to simulate slow network
94+
redisProxy.toxics().latency("slow-connection", ToxicDirection.DOWNSTREAM, 1000);
95+
96+
// Health check should timeout and return unhealthy
97+
HealthStatus slowStatus = strategy.doHealthCheck(proxyHostAndPort);
98+
assertEquals(HealthStatus.UNHEALTHY, slowStatus, "Health check should fail with high latency");
99+
100+
// Remove toxic
101+
redisProxy.toxics().get("slow-connection").remove();
102+
103+
// Health check should recover
104+
HealthStatus recoveredStatus = strategy.doHealthCheck(proxyHostAndPort);
105+
assertEquals(HealthStatus.HEALTHY, recoveredStatus, "Health check should recover from high latency");
106+
}
107+
}
108+
109+
@Test
110+
public void testConnectionDropDuringHealthCheck() throws Exception {
111+
JedisClientConfig config = DefaultJedisClientConfig.builder().socketTimeoutMillis(2000).build();
112+
113+
try (EchoStrategy strategy = new EchoStrategy(proxyHostAndPort, config, 1000, 1500, 1)) {
114+
115+
// Initial health check
116+
assertEquals(HealthStatus.HEALTHY, strategy.doHealthCheck(proxyHostAndPort));
117+
118+
// Simulate connection drop by limiting data transfer
119+
redisProxy.toxics().limitData("connection-drop", ToxicDirection.UPSTREAM, 10);
120+
121+
// This should fail due to connection issues
122+
HealthStatus droppedStatus = strategy.doHealthCheck(proxyHostAndPort);
123+
assertEquals(HealthStatus.UNHEALTHY, droppedStatus);
124+
125+
// Remove toxic
126+
redisProxy.toxics().get("connection-drop").remove();
127+
128+
// Health check should recover
129+
HealthStatus afterRecovery = strategy.doHealthCheck(proxyHostAndPort);
130+
assertEquals(HealthStatus.HEALTHY, afterRecovery);
131+
}
132+
}
133+
}

‎src/test/java/redis/clients/jedis/mcf/HealthCheckTest.java‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public class HealthCheckTest {
2929
@Mock
3030
private HealthCheckStrategy mockStrategy;
3131

32-
private HealthCheckStrategy alwaysHealthyStrategy = new HealthCheckStrategy() {
32+
private final HealthCheckStrategy alwaysHealthyStrategy = new HealthCheckStrategy() {
3333
@Override
3434
public int getInterval() {
3535
return 100;
@@ -156,7 +156,7 @@ void testHealthCheckStop() {
156156
HealthCheck healthCheck = new HealthCheck(testEndpoint, mockStrategy, mockCallback);
157157
healthCheck.start();
158158

159-
assertDoesNotThrow(() -> healthCheck.stop());
159+
assertDoesNotThrow(healthCheck::stop);
160160
}
161161

162162
// ========== HealthStatusManager Tests ==========
@@ -370,6 +370,46 @@ void testClusterConfigHealthCheckEnabledExplicitly() {
370370
}
371371

372372
// ========== Integration Tests ==========
373+
@Test
374+
@Timeout(5)
375+
void testHealthCheckRecoversAfterException() throws InterruptedException {
376+
// Create a mock strategy that alternates between healthy and throwing an exception
377+
HealthCheckStrategy alternatingStrategy = new HealthCheckStrategy() {
378+
volatile boolean isHealthy = true;
379+
380+
@Override
381+
public int getInterval() {
382+
return 1;
383+
}
384+
385+
@Override
386+
public int getTimeout() {
387+
return 5;
388+
}
389+
390+
@Override
391+
public HealthStatus doHealthCheck(Endpoint endpoint) {
392+
if (isHealthy) {
393+
isHealthy = false;
394+
throw new RuntimeException("Simulated exception");
395+
} else {
396+
isHealthy = true;
397+
return HealthStatus.HEALTHY;
398+
}
399+
}
400+
};
401+
402+
CountDownLatch statusChangeLatch = new CountDownLatch(2); // Wait for 2 status changes
403+
HealthStatusListener listener = event -> statusChangeLatch.countDown();
404+
405+
HealthStatusManager manager = new HealthStatusManager();
406+
manager.registerListener(listener);
407+
manager.add(testEndpoint, alternatingStrategy);
408+
409+
assertTrue(statusChangeLatch.await(1, TimeUnit.SECONDS));
410+
411+
manager.remove(testEndpoint);
412+
}
373413

374414
@Test
375415
@Timeout(5)

0 commit comments

Comments
 (0)