-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[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
Changes from all commits
b615184
f3ab1fa
adf7f57
7ecbe03
1e25a09
2ae7186
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -49,9 +53,31 @@ 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @atakavci |
||
&& isCircuitBreakerTrackedException(e, cluster.getCircuitBreaker())) { | ||
throw new ConnectionFailoverException( | ||
"Command failed during failover: " + cluster.getCircuitBreaker().getName(), e); | ||
} | ||
|
||
throw e; | ||
} | ||
} | ||
|
||
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 | ||
|
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); | ||
} | ||
} |
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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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
intoUnifiedJedis
, how about putting it intoMultiClusterClientConfig
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..