[maintenance events] Smart Client Handoff (umbrella, feature/sch-1) - #4668
Draft
ggivo wants to merge 11 commits into
Draft
[maintenance events] Smart Client Handoff (umbrella, feature/sch-1)#4668ggivo wants to merge 11 commits into
ggivo wants to merge 11 commits into
Conversation
* Introduce push handler
- Preparation step for processing custom push notifications
- Push notification can appear out-of band in-between executed commands
- Current Connection implementation does not support out of band Push notifications
- Meaning it will crash if "CLIENT TRACKING ON is enabled" on regular Jedis Connection and "invalidation" push event is triggered
This commit provides a way to register push handler for the connection which process incoming push messages, before actual command is executed. To preserve backward compatibility unprocessed push messages are forward to application logic as before.
- By default Connection will start with NOOP push handler which marks any incoming push event as processed and skips it
- On subcsribe/psubscribe a dedicated push handler is registered which propagates to the app only supported push vents such as (message, subscribe, unsubscribe ...)
- CacheConection is refactored to use a push handler handling "invalidate" push events only, and skipping any other
# Conflicts:
# src/main/java/redis/clients/jedis/Connection.java
# src/test/java/redis/clients/jedis/commands/jedis/PublishSubscribeCommandsTest.java
* Introduce PushHandlerChain for composable push event handling
This commit adds a new PushHandlerChain class that implements the Chain of
Responsibility pattern for Redis RESP3 push message handling. Key features:
- Allows composing multiple PushHandlers in a processing chain
- Push events propagate through the complete chain in sequence
- Events marked as not processed are propagated to the client application
- Provides both constructor-based and fluent builder API for chain creation
- Includes predefined handlers for common use cases (CONSUME_ALL, PROPAGATE_ALL)
- Supports immutable chain transformations via methods like then(),
The chain approach provides a flexible way to handle different types of push
messages (invalidations, pub/sub, etc.) with specialized handlers while
maintaining a clean separation of concerns.
Example usage:
PushHandlerChain chain = PushHandlerChain.of(loggingHandler)
.then(invalidationHandler)
.then(PushHandlerChain.PROPAGATE_PUB_SUB_PUSH_HANDLER);
* Handle relax timeout for maintenance events
- code clean up
- added relaxed timeout configuration
- fix unit tests
* Support custom Push listeners for Jedis client
* Add proactiveRebindEnabled configuration option
* PushHandler is now provided through JedisClientConfig instead through constructors.
* Fix NPE in CacheConnection
Register PushInvalidateConsumer after cache is initialised
* [cleanup] Use weak reference in AdaptiveTimeoutHandler to avoid memory leak
* [cleanup] Fix javadoc errors
* [cleanup] Fix TransactionCommandsTest mocked test
* Moving/Rebind initial support
* Mocked relaxed timeout test
* Mocked rebind test
* Fix : wrong order connection.rebind pool.clear
ConenctionFactory should be rebound before triggering the disposal of Idle connection, so that any newly creaetd are using the proper hostname
* [clean up] Address review comments from a-TODO-rov
* add more rebind tests
* clean up
* clean up remove unused test method
* fix relaxed timeout on blocking command
Issue : If Maintenace notifications are received during blocking command, relaxTimeout is enforced instead of infinit timeout.
Fix: Introduce dedicated relax timeout setting for blocking commands. It will fall back to infinit timeout if not set
* format
* enforce code formating for new classes
* reformat to fix java docs
* force formating of TimeoutOptions.java
* Address review comments
* Address review comments
* Address review comments
- Mark all pushes by default as processed
- Remove CONSUME_ALL_HANDLER
* format ConnectionTestHelper
* fix merge errors after rebase
- Use existing ReflectionTestUtil instead of ReflectionTestUtils.java
- address connection pool now uses builder - socketFactory not accessible
- test should now use Endpoints
- ListenerNotificationConsumer from Jedis
moved to Connection
- fix PushMessageNotificationTest
- fix pom.xml missing includes tag
- ConnectionTestHelper is obsolete after rebase
* remove support for generic listeners for Push events
* per connection maintenance event handler
- fix A MIGRATING event on connection A triggers AdaptiveTimeoutHandler for all connections
* use Connection memberOf reference to its owning ConnectionPool to notify it for move event
* PushConsumerContext rename and clean up
* drop readProtocolWithCheckingBroken(pushConsumer)
- propagate connection configured pushConsumers
* fix tests
- ClientSideCacheFunctionalityTest.testConcurrentAccessWithStats:420 expected: <100000> but was: <60006>
- ClientSideCacheFunctionalityTest.testEvictionPolicyMultithreaded:562 expected: <0> but was: <29>
- ClientSideCacheFunctionalityTest.testMaxSize:463 expected: <110000> but was: <17>
- RedisClientSideCacheTest>UnifiedJedisClientSideCacheTestBase.invalidationOnCacheHitTest:212 » NullPointer
* tcp mock clean up & improvements
* fix : Single-argument Protocol.read returns wrong type for pushes
* fix : Builder constructor skips MaintenanceEventConsumer registration for relaxed timeouts
* address review comments
* fix: PushMessageNotificationTest
* format and fix moving target parsing
* Address review comments from @atakavci
* Add out-of-band push notification handling
Handle unsupported push notifications gracefully instead of failing the connection.
Fixes errors occurring when push messages are received on connections that are not configured to process them. For example, enabling CLIENT TRACKING on a regular Connection may result in errors when invalidate push messages are delivered.
This change introduces initial support for out-of-band push notifications by:
- detecting push messages on the connection
- silently skipping unsupported push types
* clean up tests
* fix: Wrong @experimental annotation
* add unit test for PushConsumerChainImpl
* address review comments
* add test PushInvalidateConsumer triggers cache invalidations
* format
* [csc] protocolReadPushes now uses pushConsumers instead of custom invalidate processing
* fix: NumberUtils.safeToInt does not consider negative
* format
* update maint event format
MIGRATING <seq_number> <time> <shard_id-s>:
MIGRATED <seq_number> <shard_id-s>
FAILING_OVER <seq_number> <time> <shard_id-s>
FAILED_OVER <seq_number> <shard_id-s>
MOVING <seq_number> <time> <endpoint>
* Unify maintenance notifications configuration and add MAINT_NOTIFICATIONS ON handshake command
- add maint notification handshake (CLIENT MAINT_NOTIFICATIONS ON) - Move TimeoutOptions from JedisClientConfig to MaintenanceNotificationsConfig
- Remove proactiveRebindEnabled flag from JedisClientConfig
* extract known push message types as consts
* remove commented code
* push consumer chain is now required
* document push handling
* enforce push message as required when initialising PushMessageContext
* add benchmark for Protocol.read(RedisInputStream,PushConsumerChain)
* perf: eliminate string decoding in PushMessage type checking
Replace string-based type checking with byte array comparisons using
Arrays.equals(). PushMessage.getType() now returns byte[] directly
from content.get(0) without decoding or caching.
Adds *_BYTES constants to PushMessageTypes for efficient comparison.
* address review comments
- npe guard
- check invalidation message format
- fix tests
* format
* format
* format
* fix UnifiedJedisProactiveRebindTest
- apache commons pool creates new connection upon invalidating existing one
* protocol is now required for CommandObjects
* remove a noisy test message on disconnect
* remove a noisy test message on disconnect
* fix auto-merge error error - TRACKING ON missing for CacheConnection
* rename DISABLED_TIMEOUT -> UNSET_TIMEOUT
* fix: parse MOVING endpoint at index 3 to match ["MOVING", seq, time_s, host:port] format
* Make MOVING pool rebind bounded, lock-free and seq-guarded
The rebind to a MOVING target was permanent and reprocessed every duplicate
event. Now it expires back to the original endpoint after time_s, and a
monotonic seq guard ignores duplicate/out-of-order MOVINGs.
Implemented as a lock-free overlay in DefaultJedisSocketFactory (one
AtomicReference holding {seq, target, deadline}); new connections pick the
target while in the grace window, else the original. Deduplication is keyed
solely on the seq number: the first event for a given seq wins, any seq <=
the last applied is ignored as STALE, and any strictly-newer seq applies as
APPLIED_NEW_TARGET. ConnectionPool clears idle connections on each applied
event.
* fix: guard MOVING seq/time_s casts; overflow-safe deadline check
* chore: add DefaultJedisSocketFactoryRebindTest to formatter includes
* Move MOVING rebind overlay ownership to ConnectionFactory
ConnectionFactory now owns the {seq, target, deadline} rebind overlay and
the seq-guarded apply; DefaultJedisSocketFactory becomes a plain resolver
that reads the target through an injected supplier (override during the
grace window, else the configured host). Target selection is a pure read of
the CAS-updated state, so it can never observe a stale target, and revert is
implicit once the window expires.
Replace DefaultJedisSocketFactoryRebindTest with ConnectionFactoryRebindTest.
* Revert relaxed timeouts after the maintenance window
Relaxed timeouts were activated on MIGRATING/FAILING_OVER/MOVING but never
reverted for MOVING (it has no closing event), leaving connections stuck on
the relaxed read timeout. A connection now holds a time-bounded relaxed-timeout
overlay that reverts lazily once its window passes: MIGRATED/FAILED_OVER revert
early, MIGRATING/FAILING_OVER fall back to a configurable max duration if the
closing event is lost, and MOVING relaxes for time_s.
Relaxation is applied on pool borrow, so every connection handed out during a
rebind window is relaxed - not only the one that received MOVING.
Add TimeoutOptions.relaxedTimeoutMaxDuration (default 60s).
* Parse maintenance events into a typed hierarchy
* Centralize maintenance handling in a pool-owned controller
Maintenance is a pool-only feature handled by a single MaintenanceEventController
that owns the MOVING rebind overlay, the relax-window policy, and the handoff
hooks fired when a MOVING is applied.
- ConnectionPool creates and owns the controller and hands it to the factory.
- ConnectionFactory wires the socket-factory target resolver and relaxes on
borrow.
- Connection forwards events to the controller and keeps the per-socket
relaxed-timeout overlay.
- Handoff hooks (public add/removeHandoffHook): registered synchronous hooks,
fired once per applied MOVING with a MaintenanceHandoff payload (seq, target,
ttl); CopyOnWriteArrayList; hook exceptions propagate. ConnectionPool wires
the pool's clear() through this hook.
* set moving-target-type during handshake
* add example with enabled maint-events
* Rebalance pool on MOVING via post-DNS address mapping
Replace the pre-DNS global target override + pool.clear() with a post-DNS,
per-affected-peer redirect and selective idle eviction:
- New SocketAddressMapper interface (post-DNS)
- RebindAwareEvictionPolicy wraps the user's EvictionPolicy, destroying idle
connections whose peer is affected; handoff hook triggers evict() (not clear()).
- Connection gains getRemoteSocketAddress() to provide the affected key.
- Tests use 127.0.0.1 (deterministic post-DNS match) and assert connected-client
counts.
* format
* fix SentineledConnectionProviderReconnectionTest
* drop EndpointType.NONE till complete support is implmented
* Merge same-seq MOVING peers into the rebind's affected set
same node could resolve to multiple ip's
* Default MaintenanceNotificationsConfig.DEFAULT mode to AUTO
* Apply and revert relaxed socket timeouts on each read
Relaxed SO_TIMEOUT was pushed from maintenance event handlers and the
pool's borrow hook, requiring every affecting site to keep the socket
in sync. The protocol read now pulls the desired value from the
connection's per-receiver window and the pool controller's rebind
state, applying it through a cached last-applied value so
setSoTimeout fires only on actual transitions.
The server-supplied MOVING ttl is capped at the configured
relaxedTimeoutMaxDuration, and expired relaxation state is cleared
lazily on first observe.
* Fix stale relaxed-timeout Javadoc
* Require RESP3 for maintenance notifications
Maintenance push frames need RESP3. Mode.ENABLED now throws on a
RESP2 connection; Mode.AUTO logs and falls back without maintenance.
TcpMockServer gains per-connection HELLO negotiation so the new
MaintenanceHandshake tests can exercise both paths.
* clean up
* maintenance and relaxed-timeout mock tests for CacheConnection
* Fix TcpMockServer dropping HELLO from custom CommandHandler
* Require maintenance controller for MAINT_NOTIFICATIONS; unregister consumer on server reject
* Add sch/ to formatter includes; format Abstract* tests
* Rename UnifiedJedisProactiveRebindTest -> sch/RebindMockTest
* Inject maintenance controller into socket factory at construction time
The controller used to be wired post-construction via
ConnectionFactory.attachMaintenanceController + DefaultJedisSocketFactory
.setSocketAddressMapper. That created a window where a ConnectionFactory
existed without a fully-wired socket factory, and the instanceof
DefaultJedisSocketFactory gate inside attachMaintenanceController silently
dropped the mapper for any custom factory.
Move the wiring into the builder. ConnectionFactory.Builder now carries the
controller; withDefaults() constructs a DefaultJedisSocketFactory with the
mapper baked in via a new package-private ctor (h, c, mapper) and feeds the
same controller into the default Connection.Builder. The field becomes
final; setSocketAddressMapper and attachMaintenanceController are deleted.
ConnectionPool.wireMaintenance shrinks to installMaintenanceHooks: it only
adds the rebind-aware eviction policy + handoff hook, reading the (already
attached) controller back from the factory. Controller construction moves
to a new buildFactoryWithMaintenance helper used by the four host+config
convenience constructors -- a temporary location; C2 will lift this up to
StandaloneClientBuilder where the maintenance decision actually belongs.
* Move MaintenanceNotificationsConfig to StandaloneClientBuilder; split TimeoutOptions
The maintenance config no longer rides on JedisClientConfig -- only the
RedisClient builder accepts it, which makes the feature's scope explicit
and prevents silent no-ops on JedisPool/JedisCluster/Jedis.
MaintenanceEventController becomes public final so RedisClient-style
builders outside redis.clients.jedis can wire it without indirection;
implemented MaintenanceEvent.Handler and SocketAddressMapper interfaces
stay package-private, so dispatch and remap entry points remain internal.
StandaloneClientBuilder creates the controller directly and injects it
via a new 5-arg clientConfig-aware constructor on ConnectionPool /
PooledConnectionProvider that also attaches the AuthX listener, preserving
the contract that AuthX wiring lives in clientConfig-aware constructors.
ENABLED + custom ConnectionProvider throws IllegalArgumentException at
build time; AUTO + custom logs at debug and disables.
TimeoutOptions is dropped entirely: relaxedSocketTimeoutMillis and
relaxedBlockingSocketTimeoutMillis move to JedisClientConfig as peers of
socketTimeoutMillis/blockingSocketTimeoutMillis; relaxedWindowMaxDuration
(the per-window backstop) stays on MaintenanceNotificationsConfig with a
clearer name. Connection reads relaxed timeouts directly from clientConfig.
* Wire pool maintenance hooks via explicit controller
Pool-side hook installation (rebind-aware eviction +handoff-driven evict)
now happens only when the caller explicitly hands a
MaintenanceEventController to the clientConfig-aware 5-arg constructor:
ConnectionPool(host, cfg, cache, poolConfig, controller)
PooledConnectionProvider(host, cfg, cache, poolConfig, controller)
* Decouple Connection timeout decision from MaintenanceEventController
Connection's SO_TIMEOUT no longer queries the controller's rebind state
directly. A pluggable SoTimeoutSupplier (package-private, int getSoTimeout
(boolean blocking)) is consulted before each read; it returns the timeout
to apply or UNSET_TIMEOUT_MS to defer to the connection's own calculation.
The pool wires a controller-backed supplier in ConnectionFactory that
relaxes the timeout while a MOVING rebind window is active and defers
otherwise. Relaxed values are captured from the immutable client config at
wiring time. Connection keeps no reference to the controller on the timeout
path; the controller is still used for event dispatch and the handshake.
* Dispatch maintenance events to listeners; drop controller from Connection
Connection no longer references MaintenanceEventController. Maintenance push
frames are dispatched to registered MaintenanceEventListener(s)
, invoked synchronously on the read thread.
* Make MaintenanceEventController package-private
Thread the public MaintenanceNotificationsConfig from the builder through the
provider to ConnectionPool, which creates the controller in-package and wires
it into the factory + eviction.
* Trim unused maintenance accessors; pool owns the controller
Shrinks the maintenance surface and moves the controller reference
from ConnectionFactory to ConnectionPool, which owns it.
- Drop dead MaintenanceEventController.getMode/getEndpointType
(duplicated MaintenanceNotificationsConfig accessors).
- Drop test-only isTimeoutRelaxed and
DefaultJedisSocketFactory.getSocketAddressMapper.
- ConnectionPool holds the controller; ConnectionTestHelper reads it
from the pool instead of the factory.
borrowRelaxesConnection_duringRebindWindow asserts via isRebindActive;
obsolete factory-wiring tests removed.
* [maintenance events] Wire maintenance config/controller into multidb components (#4569)
* wire maintenance config/controller to multidb compoenents
* fix multiDb mocking issues in test
* fix flaky rebind test
* Apply MOVING rebind mapper to multidb pool connections
* Preserve maintenance config across multidb pool rebuild
* attachAuthenticationListener() now invoked in supper
* address @atakavci review
- maintNotificationsConfig set to DEFAULT in StandaloneClientBuilder
- add MaintenanceNotificationsConfig shorthand config methods
- code clean up
* address @atakavci review comments
- maintenance event parsing optimised
- extract push notification parsing from domain MaintenanceEvent in dedicated MaintenancePushCodec and optimise parsing to one pass message type processing and parsing
* extract MaintenanceEventConsumer as top level class and add test
* Move relaxed-timeout config to the maintenance feature
Relaxed socket/blocking timeouts were public knobs on JedisClientConfig
but only take effect during SCH maintenance windows. Move them to
MaintenanceNotificationsConfig (sourced by Connection/ConnectionFactory)
and make the connection-level relax API package-private, so the public
surface only exposes settings wired end-to-end.
Addresses @atakavci: don't add interface config that isn't supported
end-to-end via core components — it only creates user-side confusion.
* Set per-client maintenance-notifications defaults: standalone AUTO, MCF DISABLED
RedisClient defaults to AUTO; MultiDbClient databases default to DISABLED.
Each builder tracks the unset state via a dedicated sentinel instance, so an
explicit value is distinguishable from the default and the default can change
in a future release without overriding clients that set one.
Pending: "auto" endpoint-type discovery for MOVING (from IP format + TLS
config), to become the resolved default under AUTO mode.
* Signal malformed maintenance pushes via exception
- add todo for missing `none` support for moving event
* [maintenance events] Alternative approach for handling timeout changes via TimeoutSupplier (#4572)
* fix duble authx registration
* draft timeoutsupplier
* hot path perf optimization
* micro optimization by AI =) ridiculously micro
* - clean up connection from rebindstate
- preserver the old semantics with timout getters on conneciton
- use last reference for relaxingtimeouts instead of list
- drop simpletimeoutsupplier
- improve controller to hold rebinding connections.
- introduce returnHook in ConnectionPool
* clean up
* - feedback from @ggivo
* - clean duplicate fields
- fix compile issue
* - introduce TimeoutSupplierChain
- replace AdvancedTimeoutSupplier
* fix duration-timestamp issue with relaxation
* - drop weakhashmap
- remove applysotimeout at getStatusCodeReplyInner
* - fix possible refencing corruption with unified timeoutsupplierchain instance from controller
* - add expiration to conneciton in case future renewals needed
- improve connection with init visitors and disect the logic out to relevant components for optional features.
* - rollback to using isBlocking at connection level
-clean up with configuration classes
* - feedback from @copilot
* - polish names
* - move applyCurrentTimeout into readProtocol
* - refactor type names
* - introduce upluggable source
* - move customtimeoutsupplier setup into maintenanceawarevisitor
* - fix test-compiler issues
* - inject maintenance initiation responsibililty into connection.builder
* - fix controller unit tests
* - fix connection build for maintenancevisitor
- fix AbstractRelaxedTimeoutBehaviorTest
* - probe and add controller as listener in maintenancevisitor
* fix java doc refs
* polish
* - address @cursor feedback
* clean up connection builder
* - fix tests
* feeback from @curosr
* - format
* - format
* Align TimeoutSource chain naming with its chain-of-responsibility role
TimeoutSourceNode leaked the linked-list implementation into the type
name and forced every source to be chainable through the
TimeoutSource/UnplugableSource split.
- TimeoutSourceNode -> ChainedTimeoutSource; overrideWith/unplug ->
addOverride/removeOverride, typed to the chain class
- TimeoutSource slimmed to the read contract; the null-means-no-opinion
protocol is now documented on get()
- UnplugableSource, its unused generic and the unchecked cast removed
- anonymous controller node named RebindTimeoutSource
- get() snapshots the volatile override to avoid an NPE against a
concurrent removeOverride
---------
Co-authored-by: ggivo <ivo.gaydazhiev@redis.com>
* - format
* Restore connect-on-demand in setTimeoutInfinite and revert to SocketException when applying socket.setSoTimeout
* drop public listener from maintenanceconfig
* remove maintennace init in conn builder
* remove comments
* - fix failing MaintenanceHandshake tests
* Scope shared maintenance handshake tests to connection level
The shared handshake tests built a full RedisClient per test, coupling
connection-level behavior to client wiring. The base test now wires the
handshake onto a Connection.Builder directly, mirroring ConnectionFactory
production wiring. Client-level wiring coverage moves to a dedicated
RedisClient test.
* CacheConnectionMockTest use test cache
* mark connection expire immediately upon receiving MOVING
---------
Co-authored-by: atakavci <a_takavci@yahoo.com>
* fix accessors * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * - fix tests --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…tighten the deadline (#4611) -to not reduce the timeout by changes on timeout config
…n characteristics (#4613) * Auto-resolve moving-endpoint-type from connection characteristics CAE-1560. The handshake previously always requested the configured fixed endpoint type (default external-ip), which is wrong for private networks and TLS. The default is now resolved per connection, matching Lettuce: private peer IP selects internal-*, public external-*; TLS selects *-fqdn, plaintext *-ip. TLS is taken from the declared client config (ssl flag or sslOptions set), mirroring DefaultJedisSocketFactory; custom socket factories deviating from the declared configuration are not supported. endpointType(...) still forces a fixed type. * Rename EndpointTypeSource to EndpointTypeResolver Resolver better conveys the per-connection resolution strategy. Addresses review feedback on #4613.
…ing a rebind window (#4623) * - plug timeoutSources before handshake, unplug after handshake if disabled. * format * fix merge issue * javadoc fix * format * - feedback from copilot * apply timeouts right after relaxation * change log level for MaintenanceAwareVisitor * fix flaky one in TBA tests
Test Results 206 files ±0 206 suites ±0 7m 46s ⏱️ -3s Results for commit 58e23ca. ± Comparison against base commit ccfc080. This pull request skips 237 tests.♻️ This comment has been updated with latest results. |
…on registry (CAE-1559) (#4625) * Support 'none' moving-endpoint-type: wire decoding and handshake Groundwork shared with the deadline-based implementation (PR #4620): EndpointType.NONE with the 'none' handshake token, RESP3 null MOVING target decoded as a null-target MovingEvent, and test-server support for RESP3 null frames and server-side client drops. * Mark MOVING-affected connections via registry walk and scheduled pass The controller tracks every pool-managed connection in a weak-reference registry and runs one marking pass per applied MOVING transition (new seq or merged source) - inline for a real target, at half the grace period for 'none' - flipping an advisory volatile flag and running the handoff hooks so the pool evicts marked idles. Marked in-use connections retire on return; validation reports them invalid under testOnBorrow/testWhileIdle. Connections register before socket init, so a connect racing a MOVING commit is either visible to its marking pass or remapped via the committed rebind. Connections created after the pass are never visited, so a reconnect that re-lands on a not-yet-repointed endpoint (and its same-seq re-notification) is immune by construction. * Include ConnectionRegistry in formatter validation New file was missing from the formatter includes, failing formatter:validate in CI; add it and apply the formatter. * Create the maintenance scheduler lazily on first null-target rebind The endpoint type can be auto-resolved per connection, so the config alone cannot decide whether a scheduler is needed. Creating it on the first null-target rebind. * Use the raw MOVING ttl as the relax window MOVING's time_s is the server's completion bound (move done, old connections dropped by then), so it is a trustworthy horizon and the relaxedWindowMaxDuration cap does not apply. The backstop remains for MIGRATING/FAILING_OVER, whose time_s only means "starts within". * Fix error after merge * Remove unused MaintenanceEventController.isAffected Connection-level affected checks now go through getSocketAddress; the per-connection variant had no remaining callers. * Rename connection reconnect mark to 'retire' and route returns in one place 'markedForReconnect' suggested the connection re-establishes its own socket; it never does — the pool disposes of it and creates a replacement. 'retire()' / 'isRetired()' name the actual contract: generic, advisory, one-way removal from pool service. Connection.close() no longer special-cases the flag; the pool's return hook is the single routing point for retired connections. Addresses review feedback on #4625. * Bind the handoff hook to a single owner A multi-hook list suggested the controller outlives its pool and can be shared; it cannot — one owner creates it, registers its reaction, and closes it. Replace the list with a single-slot setHandoffHook and state the ownership contract on the class. Addresses review feedback on #4625. * Explain the registration-before-init contract in comments The register-before-connect ordering carries the marking-pass coverage guarantee; state the why on the factory helper and align comments with the code's 'applied' vocabulary. Addresses review feedback on #4625. * Move maintenance registration into the visitor visitBeforeHandshake runs before connect(), so the visitor gives the same register-before-connect guarantee as the factory did — and all maintenance wiring now lives in one component. ConnectionFactory loses its maintenance code entirely. Addresses review feedback on #4625.
…(CAE-3395) (#4636) * Support 'none' moving-endpoint-type: wire decoding and handshake Groundwork shared with the deadline-based implementation (PR #4620): EndpointType.NONE with the 'none' handshake token, RESP3 null MOVING target decoded as a null-target MovingEvent, and test-server support for RESP3 null frames and server-side client drops. * Mark MOVING-affected connections via registry walk and scheduled pass The controller tracks every pool-managed connection in a weak-reference registry and runs one marking pass per applied MOVING transition (new seq or merged source) - inline for a real target, at half the grace period for 'none' - flipping an advisory volatile flag and running the handoff hooks so the pool evicts marked idles. Marked in-use connections retire on return; validation reports them invalid under testOnBorrow/testWhileIdle. Connections register before socket init, so a connect racing a MOVING commit is either visible to its marking pass or remapped via the committed rebind. Connections created after the pass are never visited, so a reconnect that re-lands on a not-yet-repointed endpoint (and its same-seq re-notification) is immune by construction. * Include ConnectionRegistry in formatter validation New file was missing from the formatter includes, failing formatter:validate in CI; add it and apply the formatter. * Create the maintenance scheduler lazily on first null-target rebind The endpoint type can be auto-resolved per connection, so the config alone cannot decide whether a scheduler is needed. Creating it on the first null-target rebind. * Use the raw MOVING ttl as the relax window MOVING's time_s is the server's completion bound (move done, old connections dropped by then), so it is a trustworthy horizon and the relaxedWindowMaxDuration cap does not apply. The backstop remains for MIGRATING/FAILING_OVER, whose time_s only means "starts within". * Support overlapping MOVING events at pool level Distinct MOVING events (different seq or endpoint) can overlap on one pool. The single rebind slot let a newer event orphan an earlier unexpired one: its remap stopped, its pending 'none' pass no-oped, and the relax window could be silently truncated. Events are now keyed by (seq, original endpoint) in an immutable snapshot map and only expire — never supersede. Remap resolves the matched event's endpoint at connect time; relaxation holds until the last event expires; a marking pass is a no-op only if its event was pruned. * Cover overlapping MOVING events in rebind and relaxation suites Real-world scenario: DNS round-robin (simulated with a host-port mapper) spreads the pool over two backends; each announces its own MOVING to a different target, the second within the first's window. Asserts pool-wide relaxation holds until the last event expires and that connections created during each window land on that window's target only while it is open. * Fix error after merge * Remove unused MaintenanceEventController.isAffected Connection-level affected checks now go through getSocketAddress; the per-connection variant had no remaining callers. * Rename connection reconnect mark to 'retire' and route returns in one place 'markedForReconnect' suggested the connection re-establishes its own socket; it never does — the pool disposes of it and creates a replacement. 'retire()' / 'isRetired()' name the actual contract: generic, advisory, one-way removal from pool service. Connection.close() no longer special-cases the flag; the pool's return hook is the single routing point for retired connections. Addresses review feedback on #4625. * Bind the handoff hook to a single owner A multi-hook list suggested the controller outlives its pool and can be shared; it cannot — one owner creates it, registers its reaction, and closes it. Replace the list with a single-slot setHandoffHook and state the ownership contract on the class. Addresses review feedback on #4625. * Explain the registration-before-init contract in comments The register-before-connect ordering carries the marking-pass coverage guarantee; state the why on the factory helper and align comments with the code's 'applied' vocabulary. Addresses review feedback on #4625. * Move maintenance registration into the visitor visitBeforeHandshake runs before connect(), so the visitor gives the same register-before-connect guarantee as the factory did — and all maintenance wiring now lives in one component. ConnectionFactory loses its maintenance code entirely. Addresses review feedback on #4625. * Simplify event identity to seq plus target for MOVING Only MOVING operations are deduplicated pool-wide, so the id classes and their type component were dead generality: seq is sufficient for every other notification. identity() now returns an opaque value — boxed seq, or (seq, target) for MOVING so concurrent MOVINGs to different endpoints stay distinct. Addresses review feedback on #4636. * [maintenance events]Proposal for early retirement of connections (#4670) * tag with expireAt early, schedule a delayed evict * fix delayed schedule and retirement * Re-walk the registry in the scheduled pass; always run the hook The pass stamps connections registered after the apply-time walk; the hook runs however late, when stamped idles are dead sockets. * Fix tests for deadline-based retirement Tests drive a deterministic clock and await the off-thread hook. --------- Co-authored-by: ggivo <ivo.gaydazhiev@redis.com> * Format MovingOperations and add it to the formatter includes CI's check-format validates every file a PR adds; the file was missing from the formatter includes, so local runs never formatted it. * test : Add test against evict() livelock on evictor-triggered rebind The idle-validation ping consumes the MOVING on the evictor thread; retirement is stamped inline and the eviction hook runs on the maintenance scheduler. --------- Co-authored-by: atakavci <a_takavci@yahoo.com>
ggivo
added a commit
that referenced
this pull request
Aug 5, 2026
A long-lived template kept strong references to every connection it was prepared on, retaining connections the pool had already destroyed. Record them in ConnectionRegistry instead: a weak-reference registry (same class as the maintenance-events branch, PR #4668) that prunes collected connections, so close() visits only live ones. close() is now idempotent, and registerConnection re-checks discarded afterwards so a close() racing first use cannot leave the fieldset undiscarded.
ggivo
added a commit
that referenced
this pull request
Aug 7, 2026
* Initial * Add pipeline support * Add hooks * Move state in the template * Track prepared connections weakly in HashImport A long-lived template kept strong references to every connection it was prepared on, retaining connections the pool had already destroyed. Record them in ConnectionRegistry instead: a weak-reference registry (same class as the maintenance-events branch, PR #4668) that prunes collected connections, so close() visits only live ones. close() is now idempotent, and registerConnection re-checks discarded afterwards so a close() racing first use cannot leave the fieldset undiscarded. * Run cluster ASKING as a command pre-process hook On an ASK redirect the executor sent ASKING out-of-band before the command; a command-intrinsic hook such as HIMPORT's lazy PREPARE then consumed the one-shot ASKING state, redirecting the command again until attempts were exhausted. CommandObject is now immutable and carries an ordered list of pre-process hooks; withPreProcessHook() returns a modified copy. ClusterCommandExecutor appends the ASKING hook to a copy of the redirected command, so the wire order is PREPARE, ASKING, SET and ASKING applies to the command itself. Covered by CommandObjectTest and testAskResponseWithHimportSet (himportSet on a migrating slot), verified against a live cluster. * Send pending HIMPORT discards before command execution Discard reconciliation ran only in ConnectionFactory.activateObject, so legacy JedisPool/JedisSentinelPool and direct unpooled connections never issued queued HIMPORT DISCARDs, accumulating server-side fieldsets. A discard failure was also swallowed there after marking the connection broken, handing a dead connection to the borrower. Move reconciliation to the Connection.executeCommand entry points: one choke point covers every path, gated by a single volatile read. Pending discards go out as one packed write with replies drained in one pass; error replies are ignored while connection failures propagate to the retry machinery. activateObject is a no-op again. * Keep internal pipeline commands out of syncAndReturnAll results The HIMPORT PREPARE injected before a first-use himportSet was buffered like a user command, so syncAndReturnAll() returned an extra OK and shifted the positions callers rely on. Pipeline responses are now QueuedResponse entries flagged internal or user at creation; internal commands are buffered via appendInternalCommand and their replies, while still read in wire order, are filtered from syncAndReturnAll(). sync() is unchanged. * Reset HIMPORT state only on reconnect, on the owner thread setBroken() cleared the connection's HIMPORT state, but it may be called by threads that do not own the connection (forceDisconnect, maintenance paths), racing the owner's unsynchronized prepared set. The clear was also unnecessary: broken connections are dropped on every pooled and executor path, never reused. Remove the reset from setBroken(); connect() alone resets the state (renamed HimportConnectionState.reset()), which only the owner thread runs. Drop the legacy integration tag from HashImportReconcileIT - the IT suffix alone routes it to failsafe. * Reject empty field names eagerly; cover HIMPORT lifecycle at wire level Empty field names slipped past validation despite the documented contract, deferring the failure to HIMPORT PREPARE on the server. Reject "" and zero-length byte[] in the shared build step. Replace HashImportReconcileIT (legacy-client, introspection-based) with HimportLifecycleMockTest: a TcpMockServer command log pins the wire contract - PREPARE once per connection ahead of the first SET, queued DISCARDs of closed templates drained right before the next command, batched. Reconciliation runs at the shared Connection.executeCommand choke point, so provisioning-path variants added no logic coverage. * Clear HIMPORT state on RESET RESET drops connection-scoped fieldsets server-side, but the client kept its prepared note, so later himportSet calls skipped PREPARE and failed until reconnect. Jedis.reset() now resets the connection's HIMPORT state; clearing in finally is harmless on failure (at worst one extra re-PREPARE). Raw sendCommand(RESET) is intentionally out of scope. * Add @SInCE to CommandObject.withPreProcessHook New public API needs release provenance in the generated docs; also complete the truncated javadoc sentence. * Build HIMPORT PREPARE only when a connection needs it Every himportSet call eagerly built the PREPARE command object - fieldset name plus all field tokens - just to capture it in the pre-process hook, then discarded it whenever the connection was already prepared, the common case in bulk-import loops. Assemble the PREPARE arguments inside prepareBeforeUse instead, on the not-prepared branch only; the steady-state path allocates nothing PREPARE-related. Wire order covered by HimportLifecycleMockTest. * Document himportSet API and add Hash Import user guide Expand the himportSet javadoc on all four command interfaces with the verified contract: existing hashes are replaced, wrong value count and closed templates fail eagerly, transactions and cluster pipelines are unsupported, PREPARE is injected per pooled connection. Add docs/hash-import.md under User Guide covering usage, pipelining, cluster behavior and limitations, with a compiled example (HashImportUsage) mirroring the doc snippets, verified against a live Redis 8.10. --------- Co-authored-by: ggivo <ivo.gaydazhiev@redis.com>
# Conflicts: # pom.xml # src/main/java/redis/clients/jedis/Connection.java # src/main/java/redis/clients/jedis/ConnectionRegistry.java
Contributor
|
|
Contributor
|
|
Contributor
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Umbrella PR for the Smart Client Handoff (SCH) maintenance-events feature. Kept as a draft until the feature is complete; meanwhile it serves two purposes:
feature/sch-1, with PR CI validating each sync.Feature summary
With
CLIENT MAINT_NOTIFICATIONSenabled, the client reacts to server maintenance push notifications (MOVING/MIGRATING/FAILING_OVER): relaxed command timeouts during maintenance windows, proactive reconnection of affected pooled connections to the new endpoint (or to the re-pointed configured endpoint for thenonemoving-endpoint-type), with per-connection handshake-time resolution of the endpoint type.Work lands here through focused PRs (#4623, #4625, #4636, ...); do not merge this PR until the feature set is complete.