[maintenance events] Support overlapping MOVING events at pool level (CAE-3395) - #4636
Merged
Merged
Conversation
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.
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.
New file was missing from the formatter includes, failing formatter:validate in CI; add it and apply the formatter.
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.
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".
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.
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.
ggivo
marked this pull request as ready for review
July 23, 2026 06:01
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…CAE-1559-none-registry-walk
Connection-level affected checks now go through getSocketAddress; the per-connection variant had no remaining callers.
… 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.
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.
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.
…vo/CAE-3395-overlapping-moving # Conflicts: # src/main/java/redis/clients/jedis/MaintenanceEventController.java # src/test/java/redis/clients/jedis/MaintenanceEventControllerTest.java # src/test/java/redis/clients/jedis/MaintenanceMarkingTest.java
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.
…vo/CAE-3395-overlapping-moving
atakavci
reviewed
Jul 31, 2026
atakavci
reviewed
Jul 31, 2026
atakavci
reviewed
Jul 31, 2026
atakavci
reviewed
Jul 31, 2026
…vo/CAE-3395-overlapping-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.
* 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>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 57697e5. Configure here.
CI's check-format validates every file a PR adds; the file was missing from the formatter includes, so local runs never formatted it.
The idle-validation ping consumes the MOVING on the evictor thread; retirement is stamped inline and the eviction hook runs on the maintenance scheduler.
ggivo
changed the base branch from
topic/ggivo/CAE-1559-none-registry-walk
to
feature/sch-1
August 3, 2026 15:58
…CAE-3395-overlapping-moving # Conflicts: # pom.xml # src/main/java/redis/clients/jedis/Connection.java # src/main/java/redis/clients/jedis/MaintenanceEventController.java # src/test/java/redis/clients/jedis/ConnectionTestHelper.java # src/test/java/redis/clients/jedis/MaintenanceEventControllerTest.java # src/test/java/redis/clients/jedis/MaintenanceMarkingTest.java # src/test/java/redis/clients/jedis/sch/AbstractRebindBehaviorTest.java
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
2 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.
Summary
Builds on #4625. Replaces the controller's single active MOVING slot (
volatile RebindState+ CAS) with a per-operation store so distinct MOVING events that overlap in time on one pool no longer fight over one slot.A pool talks to multiple nodes (or one node across a re-plan), so distinct MOVING events — different
seqor different target — can be unexpired simultaneously. The single-slot model wholesale-replaced on a higherseq, dropped a lowerseqas a stale replay, and rejected same-seq/different-target — in each case orphaning the earlier event's remap, marking pass, and relaxation window.Change
MovingOperations— aConcurrentHashMap<MovingEventId, MovingOperation>keyed by(seq, original endpoint), deduplicating the per-connection deliveries of one server-side operation into a single pool-wide entry. Admission (dedup + peer merge) is atomic per key viacompute; expired entries are removed by readers on sight, so the post-maintenance hot path returns toisEmpty().getSocketAddressresolves the active operation for the connecting peer across all operations, at connect time (a mid-window DNS repoint is honored).Behavior deltas
seq/different-target is a distinct concurrent event, no longer a rejected conflict.seqevents are admitted (identity, not ordering); an earlier still-active event keeps remapping/relaxing after a newer one arrives and even after the newer one expires first.nonemarking pass runs after a newer event arrives instead of no-oping.Tests
MaintenanceEventControllerTest,MaintenanceMarkingTest,MaintenanceEventIdentityTest— overlapping scenarios on distinct peers (per-peer remap, relaxation outlives the shorter event, pendingnonepass survives a newer event, identity dedup).AbstractRebindBehaviorTest,AbstractRelaxedTimeoutBehaviorTest— chained/overlapping MOVING via mock servers, for bothRedisClientandMultiDbClient.Draft — opened for review; not ready to merge.
Note
Medium Risk
Touches core connection-pool maintenance, remap, and retirement during live server handoffs; behavior changes are broad but heavily covered by unit and SCH integration tests.
Overview
Replaces the maintenance controller’s single active MOVING slot (
RebindState+ CAS) withMovingOperations, a concurrent store keyed by eventidentity()—(seq, target)for MOVING — so multiple unexpired MOVING announcements can coexist instead of superseding or rejecting each other.Per-operation behavior: each admitted operation gets its own affected-peer set, remap window, and marking schedule;
getSocketAddresspicks the active operation for a peer and resolves the target at connect time. Pool-wide relaxed timeouts stay on while any operation is unexpired. Connections useretireAt(deadline)instead of a one-way boolean so'none'MOVINGs can retire at half the grace while targeted moves retire immediately; duplicate deliveries can still stamp retirement without re-running a full pass.Semantics changes: same
seq/different target and lowerseqare distinct concurrent events; newer events no longer cancel earlier pending'none'passes; handoff/eviction runs on the maintenance scheduler (fixes async eviction / evictor livelock scenarios covered in tests).Reviewed by Cursor Bugbot for commit 5500c4a. Bugbot is set up for automated code reviews on this repo. Configure here.