Skip to content

[automatic-failover] Implement failover retry for in-flight commands #4175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import redis.clients.jedis.annots.Experimental;
import redis.clients.jedis.exceptions.JedisConnectionException;
import redis.clients.jedis.exceptions.JedisValidationException;

import redis.clients.jedis.mcf.ConnectionFailoverException;

/**
* @author Allen Terleto (aterleto)
Expand Down Expand Up @@ -43,7 +43,7 @@ public final class MultiClusterClientConfig {
private static final float CIRCUIT_BREAKER_SLOW_CALL_RATE_THRESHOLD_DEFAULT = 100.0f; // measured as percentage
private static final List<Class> CIRCUIT_BREAKER_INCLUDED_EXCEPTIONS_DEFAULT = Arrays.asList(JedisConnectionException.class);

private static final List<Class<? extends Throwable>> FALLBACK_EXCEPTIONS_DEFAULT = Arrays.asList(CallNotPermittedException.class);
private static final List<Class<? extends Throwable>> FALLBACK_EXCEPTIONS_DEFAULT = Arrays.asList(CallNotPermittedException.class, ConnectionFailoverException.class);

private final ClusterConfig[] clusterConfigs;

Expand Down
15 changes: 14 additions & 1 deletion src/main/java/redis/clients/jedis/UnifiedJedis.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import redis.clients.jedis.json.Path2;
import redis.clients.jedis.json.JsonObjectMapper;
import redis.clients.jedis.mcf.CircuitBreakerCommandExecutor;
import redis.clients.jedis.mcf.FailoverOptions;
import redis.clients.jedis.mcf.MultiClusterPipeline;
import redis.clients.jedis.mcf.MultiClusterTransaction;
import redis.clients.jedis.params.*;
Expand Down Expand Up @@ -237,7 +238,19 @@ public UnifiedJedis(ConnectionProvider provider, int maxAttempts, Duration maxTo
*/
@Experimental
public UnifiedJedis(MultiClusterPooledConnectionProvider provider) {
this(new CircuitBreakerCommandExecutor(provider), provider);
this(new CircuitBreakerCommandExecutor(provider, FailoverOptions.builder().build()), provider);
}

/**
* Constructor which supports multiple cluster/database endpoints each with their own isolated connection pool.
* <p>
* With this Constructor users can seamlessly failover to Disaster Recovery (DR), Backup, and Active-Active cluster(s)
* by using simple configuration which is passed through from Resilience4j - https://resilience4j.readme.io/docs
* <p>
*/
@Experimental
public UnifiedJedis(MultiClusterPooledConnectionProvider provider, FailoverOptions failoverOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is going to be the only place that we introduce FailoverOptions into UnifiedJedis, how about putting it into MultiClusterClientConfig and may be holding a reference to it in provider itself.
My reasoning here is that retryOnFailover is not going to be the only thing that we need from config and i guess we will need it in provider as well.
Not perfectly sure about it though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggivo on a second thought, its better to do it later when we actually need it. lets keep it as it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my initial thinking, but it is rather setting of the executor, and decided to put it in the Executor, with the idea to add additional configuration settings in future if needed without the need to introduce new constructors..

this(new CircuitBreakerCommandExecutor(provider, failoverOptions), provider);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@
public class CircuitBreakerCommandExecutor extends CircuitBreakerFailoverBase
implements CommandExecutor {

public CircuitBreakerCommandExecutor(MultiClusterPooledConnectionProvider provider) {
private final FailoverOptions options;

public CircuitBreakerCommandExecutor(MultiClusterPooledConnectionProvider provider,
FailoverOptions options) {
super(provider);
this.options = options != null ? options : FailoverOptions.builder().build();
}

@Override
Expand All @@ -49,9 +53,35 @@ public <T> T executeCommand(CommandObject<T> commandObject) {
private <T> T handleExecuteCommand(CommandObject<T> commandObject, Cluster cluster) {
try (Connection connection = cluster.getConnection()) {
return connection.executeCommand(commandObject);
} catch (Exception e) {

if (retryOnFailover() && !isActiveCluster(cluster)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying to explore if we get a case where a failover already executed and a delayed exception landed on old cluster,, does this trigger a second unnecessary failover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atakavci
I think this should not be different compared to how we handle currently multiple parallel threads trying to execute a command just after the CB threshold is reached, and it transitions to the OPEN state.
Actual synchronization should happen in CircuitBreakerFailoverBase#clusterFailover
https://github.com/ggivo/jedis/blob/2ae7186d2e17788327cab0f0d32caf28c6880a64/src/main/java/redis/clients/jedis/mcf/CircuitBreakerFailoverBase.java#L40-L40

&& isCircuitBreakerTrackedException(e, cluster.getCircuitBreaker())) {
throw new ConnectionFailoverException(
"Command failed during failover: " + cluster.getCircuitBreaker().getName(), e);
}

throw e;
}
}

private boolean isCircuitBreakerOpen(CircuitBreaker circuitBreaker) {
return circuitBreaker.getState() == CircuitBreaker.State.OPEN;
}

private boolean isCircuitBreakerTrackedException(Exception e, CircuitBreaker cb) {
return cb.getCircuitBreakerConfig().getRecordExceptionPredicate().test(e);
}

private boolean retryOnFailover() {
return options.isRetryOnFailover();
}

private boolean isActiveCluster(Cluster cluster) {
Cluster activeCluster = provider.getCluster();
return activeCluster != null && activeCluster.equals(cluster);
}

/**
* Functional interface wrapped in retry and circuit breaker logic to handle open circuit breaker
* failure scenarios
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package redis.clients.jedis.mcf;

import redis.clients.jedis.exceptions.JedisException;

public class ConnectionFailoverException extends JedisException {
public ConnectionFailoverException(String s, Exception e) {
super(s, e);
}
}
59 changes: 59 additions & 0 deletions src/main/java/redis/clients/jedis/mcf/FailoverOptions.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package redis.clients.jedis.mcf;

import redis.clients.jedis.annots.Experimental;

/**
* Configuration options for CircuitBreakerCommandExecutor
*/
@Experimental
public class FailoverOptions {
private final boolean retryOnFailover;

private FailoverOptions(Builder builder) {
this.retryOnFailover = builder.retryOnFailover;
}

/**
* Gets whether to retry failed commands during failover
* @return true if retry is enabled, false otherwise
*/
public boolean isRetryOnFailover() {
return retryOnFailover;
}

/**
* Creates a new builder with default options
* @return a new builder
*/
public static Builder builder() {
return new Builder();
}

/**
* Builder for FailoverOptions
*/
public static class Builder {
private boolean retryOnFailover = false;

private Builder() {
}

/**
* Sets whether to retry failed commands during failover
* @param retry true to retry, false otherwise
* @return this builder for method chaining
*/
public Builder retryOnFailover(boolean retry) {
this.retryOnFailover = retry;
return this;
}

/**
* Builds a new FailoverOptions instance with the configured options
* @return a new FailoverOptions instance
*/
public FailoverOptions build() {
return new FailoverOptions(this);
}
}
}
Loading
Loading