Skip to content

Conversation

@uglide
Copy link
Contributor

@uglide uglide commented Aug 28, 2025

Motivation

This change creates a foundation for providing a new client class(es) for AA failover, allowing developers to easily instantiate JedisPooled, JedisSentineled, JedisCluster, etc.

Review

Summary

This commit introduces a formal Builder pattern for UnifiedJedis-based clients (JedisPooled, JedisCluster, JedisSentineled) and adds a shared builder base (AbstractClientBuilder) plus specialized builders for standalone, cluster, and sentinel deployments. It also exposes a couple of configuration hooks, aligns RediSearch default dialect handling, and adds reflection-based tests to ensure builder coverage over the existing constructor surfaces. Build tooling is adjusted to preserve parameter names for those tests.


High-level Impact

  • Additive public API: new static builder() entry points and builder classes under redis.clients.jedis.builders.
  • Internal visibility tweak: UnifiedJedis constructor made package-visible to allow subclass super(...) calls.
  • Slightly broader access: CommandObjects.setKeyArgumentPreProcessor() is now public.
  • Tests and tooling: reflection-based tests to validate builder coverage; Maven profile to compile with parameter names.
  • Behavioral defaults remain intact (e.g., RediSearch DEFAULT_DIALECT still 2, now referenced via constant).

Detailed Changes

1) Builders

  • New classes:
    • redis.clients.jedis.builders.AbstractClientBuilder<T,C>
    • redis.clients.jedis.builders.StandaloneClientBuilder
    • redis.clients.jedis.builders.ClusterClientBuilder
    • redis.clients.jedis.builders.SentinelClientBuilder

Key capabilities (AbstractClientBuilder):

  • Common options: poolConfig, cache/cacheConfig, connectionProvider, commandExecutor, commandObjects, redisProtocol, key preprocessor, JSON mapper, search dialect.
  • Build flow: validate -> optional cache from cacheConfig -> default provider/executor if unset -> apply common config to CommandObjects -> instantiate concrete client.
  • Cache implies RESP3: calling cache(...) or cacheConfig(...) forces redisProtocol=RESP3 (UnifiedJedis guards against cache+RESP2).
  • applyCommonConfiguration wires:
    • CommandObjects.setKeyArgumentPreProcessor(...)
    • CommandObjects.setJsonObjectMapper(...)
    • CommandObjects.setDefaultSearchDialect(...) when non-default

Specialized builders:

  • StandaloneClientBuilder
    • hostAndPort(String,int|HostAndPort), clientConfig(JedisClientConfig)
    • fromURI(String|URI) builds DefaultJedisClientConfig based on URI and sets host/port
    • Default provider: PooledConnectionProvider(hostAndPort, config, cache, poolConfig)
    • Validation intends to ensure either URI or host/port, not both
  • ClusterClientBuilder
    • nodes(Set), clientConfig, maxAttempts(int), maxTotalRetriesDuration(Duration), topologyRefreshPeriod(Duration)
    • Default provider: ClusterConnectionProvider(nodes, config, cache, poolConfig, topologyRefreshPeriod)
    • Validates nodes non-empty and non-negative durations/attempts
  • SentinelClientBuilder
    • masterName(String), sentinels(Set), clientConfig, sentinelClientConfig
    • Default provider: SentineledConnectionProvider(masterName, clientConfig, cache, poolConfig, sentinels, sentinelClientConfig)
    • Validates required fields

2) Jedis* classes integrate builders

  • JedisPooled, JedisCluster, JedisSentineled:
    • Add private constructors accepting (CommandExecutor, ConnectionProvider, CommandObjects, RedisProtocol, Cache) and delegating to super(...)
    • Add public static inner Builder extends respective specialized builder
    • Add public static Builder builder() factory method

3) UnifiedJedis visibility

  • UnifiedJedis(CommandExecutor, ConnectionProvider, CommandObjects, RedisProtocol, Cache) constructor changed from private to package-visible to enable subclass super(...) calls from Jedis*.

4) CommandObjects tuning

  • Replace hard-coded default search dialect (2) with SearchProtocol.DEFAULT_DIALECT.
  • setKeyArgumentPreProcessor made public (was @experimental with package access). This enables configuration via builders.

5) RediSearch protocol constant usage

  • SearchDefaultDialectTest and CommandObjects now reference SearchProtocol.DEFAULT_DIALECT rather than literals. SearchProtocol.DEFAULT_DIALECT remains 2.

6) Build tooling to support reflection tests

  • Makefile: mvn-test-local and mvn-test now pass -Dwith-param-names=true.
  • pom.xml: new profile tests-with-params activated via -Dwith-param-names=true, enabling:
    • maven-compiler-plugin true (preserve method/parameter names)
    • maven-surefire-plugin argLine includes --add-opens java.base/java.lang=ALL-UNNAMED ${JVM_OPTS}

7) Tests

  • New reflection-based coverage tests:
    • builders/JedisPooledConstructorReflectionTest
    • builders/JedisClusterConstructorReflectionTest
    • builders/JedisSentineledConstructorReflectionTest
    • builders/UnifiedJedisConstructorReflectionTest
  • Purpose: verify that for every constructor of the target classes, the builder surface offers an equivalent configuration path (maps each parameter type/name to a builder option). Tests assert full coverage and log any gaps.

API/Compatibility Assessment

  • Additive: New builder APIs are additive; existing constructors remain.
  • Internal constructor visibility change is not exposed as public API but is a package-level relaxation; safe and contained.
  • Making setKeyArgumentPreProcessor public increases surface area; consider de-@experimental or documenting stability expectations.
  • No default behavioral changes beyond wiring options when using the builder.

ggivo and others added 30 commits June 9, 2025 15:41
  * Add toxyproxy
  * Add test endpoints
  * Add example tests
…se (#4176)

* enforce formating for mcf

* enforce formating for mcf

* Trigger integration test jobs  feature branches
…t breaker before retries (#4178)

* fix: Change evaluation order of circuit breaker and retry mechanisms

This commit modifies the order in which circuit breaker and retry mechanisms
are evaluated during failover scenarios. Previously, the circuit breaker was
applied after all retries were exhausted, which could delay failover in
unstable network conditions. Now, the circuit breaker counts each connection
error separately, even during retry attempts.

* format

* format

* use java-8 compatible formater version

* fix AutomaticFailoverTest

* formating
…4175)

* Implement failover retry for in-flight commands

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.

* Update src/main/java/redis/clients/jedis/mcf/CircuitBreakerCommandExecutor.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
# Conflicts:
#	src/main/java/redis/clients/jedis/mcf/CircuitBreakerCommandExecutor.java

* format

format & clean-up

* fix FailoverIntegrationTest.testInflightCommandsAreNotRetriedAfterFailover

    blpop timeout should be less than async command timeout to prevent completing with java.util.concurrent.TimeoutException instead of actuall command failure

* Address comments from review
   - rename retryFailedInflightCommands->retryOnFailover
   - remove check for CB in OPEN state

* remove unused method

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…es (#4183)

* fix formatting for anticipated failover changes

* additional formatting to cover given folders
#4189)

* - weighted cluster seleciton
- Healtstatus manager with initial listener and registration logic
- pluggable health checker strategy  introduced,  these are draft NoOpStrategy, EchoStrategy, LagAwareStrategy,
- fix failing tests impacted from weighted clusters

* - add builder for ClusterConfig
- add echo ot CommandObjects and UnifiedJEdis
- improve StrategySupplier by accepting jedisclientconfig
- adapt EchoStrategy to StrategySupplier. Now it handles the creation of connection by accepting endpoint and JedisClientConfig
- make healthchecks disabled by default
- drop noOpStrategy
-  add unit&integration tests for health check

* - fix naming

* clean up and mark override methods

* fix link in javadoc

* fix formatting

* - fix double registered listeners in healtstatusmgr
- clear redundant catch
- replace failover options and drop failoveroptions class
- remove forced_unhealthy from healthstatus
- fix failback check
- add disabled flag to cluster
- update/fix related tests

* Update src/main/java/redis/clients/jedis/mcf/EchoStrategy.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* - weighted cluster seleciton
- Healtstatus manager with initial listener and registration logic
- pluggable health checker strategy  introduced,  these are draft NoOpStrategy, EchoStrategy, LagAwareStrategy,
- fix failing tests impacted from weighted clusters

* - add builder for ClusterConfig
- add echo ot CommandObjects and UnifiedJEdis
- improve StrategySupplier by accepting jedisclientconfig
- adapt EchoStrategy to StrategySupplier. Now it handles the creation of connection by accepting endpoint and JedisClientConfig
- make healthchecks disabled by default
- drop noOpStrategy
-  add unit&integration tests for health check

* - fix naming

* clean up and mark override methods

* fix link in javadoc

* fix formatting

* - fix double registered listeners in healtstatusmgr
- clear redundant catch
- replace failover options and drop failoveroptions class
- remove forced_unhealthy from healthstatus
- fix failback check
- add disabled flag to cluster
- update/fix related tests

* Update src/main/java/redis/clients/jedis/mcf/EchoStrategy.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* - add remove endpoints

* - replace cluster disabled with failbackCandidate
- replace failback enabled with failbacksupported in client
- fix formatting
- set defaults

* - remove failback candidate
- fix failing tests

* - fix remove logic
- fix failing tests

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* - weighted cluster seleciton
- Healtstatus manager with initial listener and registration logic
- pluggable health checker strategy  introduced,  these are draft NoOpStrategy, EchoStrategy, LagAwareStrategy,
- fix failing tests impacted from weighted clusters

* - add builder for ClusterConfig
- add echo ot CommandObjects and UnifiedJEdis
- improve StrategySupplier by accepting jedisclientconfig
- adapt EchoStrategy to StrategySupplier. Now it handles the creation of connection by accepting endpoint and JedisClientConfig
- make healthchecks disabled by default
- drop noOpStrategy
-  add unit&integration tests for health check

* - fix naming

* clean up and mark override methods

* fix link in javadoc

* fix formatting

* - fix double registered listeners in healtstatusmgr
- clear redundant catch
- replace failover options and drop failoveroptions class
- remove forced_unhealthy from healthstatus
- fix failback check
- add disabled flag to cluster
- update/fix related tests

* Update src/main/java/redis/clients/jedis/mcf/EchoStrategy.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* - add remove endpoints

* - replace cluster disabled with failbackCandidate
- replace failback enabled with failbacksupported in client
- fix formatting
- set defaults

* - remove failback candidate
- fix failing tests

* - fix remove logic
- fix failing tests

* - periodic failback checks
- introduce graceperiod
- fix issue when CB is forced_open and gracePeriod is completed

* - format

* - fix timing issue

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ient init (#4207)

* - weighted cluster seleciton
- Healtstatus manager with initial listener and registration logic
- pluggable health checker strategy  introduced,  these are draft NoOpStrategy, EchoStrategy, LagAwareStrategy,
- fix failing tests impacted from weighted clusters

* - add builder for ClusterConfig
- add echo ot CommandObjects and UnifiedJEdis
- improve StrategySupplier by accepting jedisclientconfig
- adapt EchoStrategy to StrategySupplier. Now it handles the creation of connection by accepting endpoint and JedisClientConfig
- make healthchecks disabled by default
- drop noOpStrategy
-  add unit&integration tests for health check

* - fix naming

* clean up and mark override methods

* fix link in javadoc

* fix formatting

* - fix double registered listeners in healtstatusmgr
- clear redundant catch
- replace failover options and drop failoveroptions class
- remove forced_unhealthy from healthstatus
- fix failback check
- add disabled flag to cluster
- update/fix related tests

* Update src/main/java/redis/clients/jedis/mcf/EchoStrategy.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* - add remove endpoints

* - replace cluster disabled with failbackCandidate
- replace failback enabled with failbacksupported in client
- fix formatting
- set defaults

* - remove failback candidate
- fix failing tests

* - fix remove logic
- fix failing tests

* - periodic failback checks
- introduce graceperiod
- fix issue when CB is forced_open and gracePeriod is completed

* - introduce StatusTracker with purpose of waiting initial healthcheck results during consturction of provider
- add HealthStatus.UNKNOWN as default for Cluster
- handle status changes in order of events during initialization
- add tests for status tracker and orderingof events
- fix impacted unit&integ tests

* - introduce forceActiveCluster by duration
- fix formatting

* - fix failing tests by waiting on clusters to get healthy

* - fix failing scenario test
- downgrade logback version for slf4j compatibility
- increase timeouts for faultInjector

* - adressing reviews and feedback

* - fix formatting

* - fix formatting

* - get rid of the queue and event ordering for healthstatus change in MultiClusterPooledConnectionProvider
- add test for init and post init events
- fix failing tests

* - replace use of reflection with helper methods
- fix failing tests due to method name change

* - replace  'sleep' with 'await', feedback from @a-TODO-rov

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…e approach with builders (#4226)

* - weighted cluster seleciton
- Healtstatus manager with initial listener and registration logic
- pluggable health checker strategy  introduced,  these are draft NoOpStrategy, EchoStrategy, LagAwareStrategy,
- fix failing tests impacted from weighted clusters

* - add builder for ClusterConfig
- add echo ot CommandObjects and UnifiedJEdis
- improve StrategySupplier by accepting jedisclientconfig
- adapt EchoStrategy to StrategySupplier. Now it handles the creation of connection by accepting endpoint and JedisClientConfig
- make healthchecks disabled by default
- drop noOpStrategy
-  add unit&integration tests for health check

* - fix naming

* clean up and mark override methods

* fix link in javadoc

* fix formatting

* - fix double registered listeners in healtstatusmgr
- clear redundant catch
- replace failover options and drop failoveroptions class
- remove forced_unhealthy from healthstatus
- fix failback check
- add disabled flag to cluster
- update/fix related tests

* Update src/main/java/redis/clients/jedis/mcf/EchoStrategy.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* - add remove endpoints

* - replace cluster disabled with failbackCandidate
- replace failback enabled with failbacksupported in client
- fix formatting
- set defaults

* - remove failback candidate
- fix failing tests

* - fix remove logic
- fix failing tests

* - periodic failback checks
- introduce graceperiod
- fix issue when CB is forced_open and gracePeriod is completed

* - introduce StatusTracker with purpose of waiting initial healthcheck results during consturction of provider
- add HealthStatus.UNKNOWN as default for Cluster
- handle status changes in order of events during initialization
- add tests for status tracker and orderingof events
- fix impacted unit&integ tests

* - introduce forceActiveCluster by duration
- fix formatting

* - fix failing tests by waiting on clusters to get healthy

* - fix failing scenario test
- downgrade logback version for slf4j compatibility
- increase timeouts for faultInjector

* - adressing reviews and feedback

* - fix formatting

* - fix formatting

* - get rid of the queue and event ordering for healthstatus change in MultiClusterPooledConnectionProvider
- add test for init and post init events
- fix failing tests

* - replace use of reflection with helper methods
- fix failing tests due to method name change

* - introduce clusterSwitchEvent and drop clusterFailover post processor
- fix broken echostrategy due to connection issue
- make healtthCheckStrategy closable and close on
- adding fastfailover mode to config and provider
- add local failover tests for total failover duration

* - introduce fastfailover using  objectMaker injection into connectionFactory

* - polish

* - cleanup

* - improve healtcheck thread visibility

* - introduce TrackingConnectionPool with FailFastConnectionFactory
- added builders to connection and connectionFactory
- introduce initializtionTracker to track list of connections during their construction.

* - return broken source as usual
- do not throw exception is failover already happening

* - unblock waiting threads

* - failover by closing the pool

* - formatting

* - check waiters and active/idle connections to force disconnect

* - add builder to trackingconnectionpool

* - fix failing tests due to mocked ctor for trackingConnectionPool

* - replace initTracker with split initializtion of conn

* - refactor on builders and ctors

* - undo format

* - clena up

* - add exceptions to logs

* - add max wait duration and minConsecutiveSuccessCount to healthCheckStrategy

* - replace  'sleep' with 'await', feedback from @a-TODO-rov

* - fix status tracker tests

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…on error (#4230)

* Add tests: EchoStrategy does recover on connection error

* log error's on exception
…4236)

* - replace SimpleEntry with HealthCheckResult

* - format
…4248)

* Add option for rate limiter to MultiThreadedFakeApp

* retrigger checks
* - lag-aware api

* - introduce healtCheckMetaConfig
- add config objects for both EchoStrategy and LagAwareStrategy
- make StrategySupplier interface generic
- fix impacted tests

* - fix issues with docs and log

* Fix EchoStrategyIntegrationTest after merge

* Implement resolve bdbId based on configured HostAndPort

* format & merge RedisRestAPI unit tests

* introduce builder for LagAwareStrategy

* configurable extended-check

   - provide option to enable/disable lag check
   - default to lag-aware check with AVAILABILITY_LAG_TOLERANCE_DEFAULT of 100ms

* - fix java doc issue

* - formatting

* -fix doc issue

* - formatting

* revert introduce healtCheckMetaConfig

    - introduces type unsafe warnings, revert till they are resovled

* Add list constructor

* Add option for rate limiter to MultiThreadedFakeApp

* Use /v1/local/bdbs/<int:uid>/endpoint/availability

   - use /v1/local/bdbs/<int:uid>/endpoint/availability instead of  availability address vs /v1/bdbs/<int:uid>/availability

* Add builder factory method for base HealthCheckStrategy.Config

* format

* format

* revert to /v1/bdbs/<int:uid>/availability

  reading again the spec /local is inteded to be used when configuring LoadBalancers

* revert to /v1/bdbs/<int:uid>/availability

  reading again the spec /local is inteded to be used when configuring LoadBalancers

* format

* fix unit test after revert of local

* format

* Address review comments
  - remove misleading default value comment
  - renamed Config.endpoint to Config.restEndpoint
  - renamed dataPathAvailability -> databaseAvailability

---------

Co-authored-by: ggivo <ivo.gaydazhiev@redis.com>
… `HealthCheck` itself (#4244)

* - set sleep duration 1 ms for forceDisconnect

* - refactor cluster to replace HealthStatus with HealthCheck itself

* - move Endpoint to parent package

* - format

* - fix typo

* - adjust tests with new provider behaviour with HealthCheck

* Address review comments
   - rename getTimeout() -> getMaxWaitFor()
   - remove unnecessary noHealthChecks

* resolve merge conflicts

* format

* format

---------

Co-authored-by: Ivo Gaydazhiev <ivo.gaydazhiev@redis.com>
…ovider is closed. (#4252)

* fix: health checks continue to run even after provider is closed.

Closes #4251

* format

* format & fix tests after merge

format
* Improve AA failover test

* Fix endpoint name

* Count failedCommandsAfterFailover

* do not enforce formating on ActiveActiveFailoverTest

---------

Co-authored-by: Ivo Gaydazhiev <ivo.gaydazhiev@redis.com>
…al state changes #4255 (#4256)

* fix: only notify health status listeners on actual state changes #4255

  - Replace overlapping health checks with sequential execution
  - use wasUpdated flag to prevent false notifications from timestamp conflicts.
    Eliminates race conditions causing spurious failovers.

Closes: #4255

* format

* Update src/main/java/redis/clients/jedis/mcf/HealthCheckImpl.java

Co-authored-by: atakavci <a_takavci@yahoo.com>

* revert to fixed rate health checks

* fix ActiveActiveLocalFailoverTest

  - reduced endpoint pause to 10s to speed up test
  - ensure test endpoint is eavailable before startign the test (awaits up to endpoint pause time)

---------

Co-authored-by: atakavci <a_takavci@yahoo.com>
…4233 (#4258)

Address: Connection.forceDisconnect should also set the connection as broken #4233
* Initial draft

* Cover health checks

* Add javadoc's for MultiClusterClientConfig

* Fix docs for health checks

* Fix javadoc's

* Update wordlist.txt

---------

Co-authored-by: Ivo Gaydazhiev <ivo.gaydazhiev@redis.com>
   - ensure hbase-formatter.xml match current one in master
…ailover

# Conflicts:
#	.github/wordlist.txt
#	pom.xml
#	src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java
   - setClusterFailoverPostProcessor renamed
   - format AutomaticFailoverTest
    - Endpoint.java
    - ActiveActiveLocalFailoverTest
    - FailbackMechanismIntegrationTest
    - FailbackMechanismUnitTest
    - PeriodicFailbackTest
    - StatusTrackerTest

format

    - ActiveActiveLocalFailoverTest.java
Copy link
Collaborator

@ggivo ggivo left a comment

Choose a reason for hiding this comment

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

Approach with the builders looks good.
I have put some comments, and have concerns around tests validating the constructor coverage.

@ggivo ggivo changed the base branch from feature/automatic-failover to master September 30, 2025 07:00
  -  resolve cannot find symbol
             Error:    symbol:   class MultiClusterPooledConnectionProvider
* settings like key preprocessor, JSON mapper, and search dialect.
* @param commandObjects the CommandObjects instance to configure
*/
public void applyCommandObjectsConfiguration(CommandObjects commandObjects) {
Copy link
Contributor

@atakavci atakavci Oct 1, 2025

Choose a reason for hiding this comment

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

missing important part is that this instance of commandObjects is not managed in other abstract or concrete builders.
In addition to that commandobjects is a kind of factory itself and should be left to be instantiated/configured by typed builders(like the other fields in builder) instead of being set as default by abstract one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Fixed.

@ggivo ggivo merged commit f8de2fe into master Oct 2, 2025
15 checks passed
@ggivo ggivo added this to the 7.0.0 milestone Oct 10, 2025
@ggivo ggivo deleted the im/add-builders branch October 10, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants