[automatic-failover] Implement failover retry for in-flight commands #4175
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ggivo
changed the title
This change adds support for retrying in-flight commands when a Redis connection fails and the client automatically fails over to another cluster. The feature is configurable through the FailoverOptions builder. Key changes: - Added retryFailedInflightCommands option to FailoverOptions - Implemented retry logic in CircuitBreakerCommandExecutor - Added integration tests to verify both retry and no-retry behavior - Created utility methods for test setup and configuration This enhancement improves resilience for long-running commands like blocking operations, allowing them to transparently continue on the failover cluster without client-side errors.
…cutor.java Co-authored-by: Copilot <[email protected]> # Conflicts: # src/main/java/redis/clients/jedis/mcf/CircuitBreakerCommandExecutor.java
ggivo
force-pushed
the
automatic-failover-retry-on-failover
branch
from
21103fe to
adf7f57
Compare
…lover
blpop timeout should be less than async command timeout to prevent completing with java.util.concurrent.TimeoutException instead of actuall command failure
- rename retryFailedInflightCommands->retryOnFailover - remove check for CB in OPEN state
|
Current implementation performs actual failover, on first incoming command after CB threshold is reached. |
There was a problem hiding this comment.
Pull Request Overview
This pull request implements automatic failover for in-flight Redis commands by adding a new retry option and updating related components to support configurable retry behavior during failover. Key changes include:
- Adding the new "retryOnFailover" option with its builder in FailoverOptions.
- Implementing retry logic in CircuitBreakerCommandExecutor.
- Updating UnifiedJedis to use the new options and adding integration tests for both retry-enabled and retry-disabled scenarios.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/redis/clients/jedis/failover/FailoverIntegrationTest.java | Updated integration tests to verify automatic and manual failover behaviors and added tests for retry scenarios. |
| src/main/java/redis/clients/jedis/mcf/FailoverOptions.java | Introduced new configuration option for failover retry behavior. |
| src/main/java/redis/clients/jedis/mcf/ConnectionFailoverException.java | Added a specialized exception for failover command failures. |
| src/main/java/redis/clients/jedis/mcf/CircuitBreakerCommandExecutor.java | Integrated the retryOnFailover option in handling command execution and failover logic. |
| src/main/java/redis/clients/jedis/UnifiedJedis.java | Updated constructors to incorporate the new failover options. |
| src/main/java/redis/clients/jedis/MultiClusterClientConfig.java | Updated fallback exception configuration to include ConnectionFailoverException. |
src/main/java/redis/clients/jedis/mcf/CircuitBreakerCommandExecutor.java
Outdated
Show resolved
Hide resolved
|
@atakavci updated, the review could you take a second look? |
| * <p> | ||
| */ | ||
| @Experimental | ||
| public UnifiedJedis(MultiClusterPooledConnectionProvider provider, FailoverOptions failoverOptions) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
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 change adds support for retrying in-flight commands when a Redis connection fails and the client automatically fails over to another cluster. The feature is configurable through the FailoverOptions builder.
Key changes:
This enhancement improves resilience for long-running commands like blocking operations, allowing them to transparently continue on the failover cluster without client-side errors.