[maintenance events] Alternative approach for handling timeout changes via TimeoutSupplier#4572
Open
atakavci wants to merge 15 commits into
Open
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ggivo
reviewed
Jun 26, 2026
ggivo
left a comment
ggivo
left a comment
Collaborator
There was a problem hiding this comment.
Direction looks good to me, will need some polish
I also forgot to add one additional requirement around maintenance events relaxation,
we need to ensure we are not reducing the original configured on the connection in case relaxed timeout end's up to be lower then configured
| */ | ||
| private static final class RebindState { | ||
|
|
||
| static LongSupplier CLOCK_NANOS = System::nanoTime; |
Collaborator
There was a problem hiding this comment.
I would keep it per instance, it is used for testing and it make sense different test to have different instance.
Contributor
Author
There was a problem hiding this comment.
check NanoClock , i believe it looks good enough while keeping performance high as possible.
- 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
…ers' into ali/sch
- fix compile issue
- replace AdvancedTimeoutSupplier
- remove applysotimeout at getStatusCodeReplyInner
… instance from controller
- improve connection with init visitors and disect the logic out to relevant components for optional features.
-clean up with configuration classes
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit eb044bc. Configure here.
| RebindState cur = rebind.get(); | ||
| if (cur != null && e.seq < cur.seq) { | ||
| RebindState cur = rebind; | ||
| if (e.seq < cur.seq) { |
There was a problem hiding this comment.
Expired rebind blocks new MOVING
High Severity
onMoving treats e.seq < cur.seq as an older event without checking whether cur is still a valid rebind window. After the deadline passes, rebind still holds the old state with a high sequence, so a later MOVING with a lower (but legitimate) sequence is dropped and handoff never updates.
Reviewed by Cursor Bugbot for commit eb044bc. Configure here.
Contributor
Author
There was a problem hiding this comment.
@ggivo
we discussed it is not possible to have multiple MOVING operations happening at the same time. This suggests that multiple different nodes can not go into rebinding state anyway on server side. Could you confirm? If yes lets skip this one from AI.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors Jedis’ maintenance-event timeout handling by replacing the per-operation SoTimeoutSupplier/scattered timeout fields with a supplier chain, and moving the maintenance handshake/setup out of Connection into an init-visitor registered by ConnectionFactory. This also changes MOVING rebind handling to mark pooled connections for discard via expiry/return hooks instead of requestRebind().
Changes:
- Introduces
TimeoutSupplierChain+ decorator implementations (DefaultTimeoutSupplier,ExpiringTimeoutSupplier) and switchesConnectionreads toapplySoTimeout(currentTimeout())before reply parsing. - Reworks maintenance wiring: adds
InitVisitor+MaintenanceAwareVisitor;MaintenanceEventControllernow exposes a shared timeout supplier and rebind predicates. - Makes
MaintenanceNotificationsConfigimmutable and renames relaxed timeout accessors; removesJedisClientConfig.UNSET_TIMEOUT_MS.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/redis/clients/jedis/TimeoutSupplierDecorator.java | Adds a decorator base for chaining/overriding timeout suppliers. |
| src/main/java/redis/clients/jedis/TimeoutSupplierChain.java | Introduces the chain interface and TimeoutInfo value object. |
| src/main/java/redis/clients/jedis/SoTimeoutSupplier.java | Removes the previous per-operation SO_TIMEOUT supplier interface. |
| src/main/java/redis/clients/jedis/NanoClock.java | Adds a shared nanoTime seam used for maintenance expiry/deadlines. |
| src/main/java/redis/clients/jedis/MaintenanceNotificationsConfig.java | Makes config immutable; renames relaxed timeout getters/setters; changes defaults. |
| src/main/java/redis/clients/jedis/MaintenanceEventController.java | Reworks rebind state tracking; adds shared timeout supplier + isRebinding(). |
| src/main/java/redis/clients/jedis/MaintenanceAwareVisitor.java | Moves maintenance handshake (CLIENT MAINT_NOTIFICATIONS) into an init visitor. |
| src/main/java/redis/clients/jedis/JedisClientConfig.java | Removes UNSET_TIMEOUT_MS sentinel and related helper. |
| src/main/java/redis/clients/jedis/InitVisitor.java | Adds init-visitor SPI for post-initialization wiring. |
| src/main/java/redis/clients/jedis/ExpiringTimeoutSupplier.java | Adds an expiring timeout override supplier for relaxed windows. |
| src/main/java/redis/clients/jedis/DefaultTimeoutSupplier.java | Adds a baseline/default timeout supplier that can be overridden. |
| src/main/java/redis/clients/jedis/ConnectionPool.java | Adds a return hook to mark rebinding connections as broken on return. |
| src/main/java/redis/clients/jedis/ConnectionFactory.java | Wires maintenance via MaintenanceAwareVisitor + controller timeout supplier. |
| src/main/java/redis/clients/jedis/Connection.java | Replaces internal timeout fields/logic with supplier chain + pre-read timeout application; adds expiry-based discard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
418
to
421
| public void setTimeoutInfinite() { | ||
| if (!isConnected()) { | ||
| connect(); | ||
| } | ||
| isBlocking = true; | ||
| applyTimeout(effectiveSoTimeout()); | ||
| applySoTimeout(currentTimeout()); | ||
| } |
Comment on lines
+5
to
+7
| class NanoClock { | ||
| public static LongSupplier INSTANCE = System::nanoTime; | ||
| } No newline at end of file |
Comment on lines
+86
to
+92
| public int getRelaxedTimeout() { | ||
| return relaxedTimeout; | ||
| } | ||
|
|
||
| public int getRelaxedBlockingTimeout() { | ||
| return relaxedBlockingTimeout; | ||
| } |
Comment on lines
+122
to
130
| public Builder relaxedTimeout(int timeout) { | ||
| this.relaxedTimeout = timeout; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Per-command socket timeout (ms) applied while a relaxation window is active. Pass | ||
| * {@link JedisClientConfig#UNSET_TIMEOUT_MS} to inherit the connection's configured socket | ||
| * timeout. Defaults to | ||
| * {@link MaintenanceNotificationsConfig#DEFAULT_RELAXED_SOCKET_TIMEOUT_MS}. | ||
| */ | ||
| public Builder relaxedSocketTimeoutMillis(int millis) { | ||
| this.relaxedSocketTimeoutMillis = millis; | ||
| public Builder relaxedBlockingTimeout(int blockingTimeout) { | ||
| this.relaxedBlockingTimeout = blockingTimeout; | ||
| return this; | ||
| } |
Comment on lines
1091
to
+1103
| /** | ||
| * Switches this connection to relaxed timeouts for at most {@code period}. While the window is | ||
| * open, commands use {@link #getRelaxedSoTimeout()} and {@link #getRelaxedBlockingSoTimeout()} | ||
| * instead of the configured values, giving in-flight commands extra headroom across a | ||
| * server-side maintenance event (MIGRATING / FAILING_OVER / MOVING-receiver). The original | ||
| * timeouts return into effect once the window closes or {@link #resetRelaxedTimeouts()} is | ||
| * called. Calling this with a later deadline extends the window; an earlier one is ignored. | ||
| * | ||
| * instead of the configured values, giving in-flight commands extra headroom across a server-side | ||
| * maintenance event (MIGRATING / FAILING_OVER / MOVING-receiver). The original timeouts return | ||
| * into effect once the window closes or {@link #resetRelaxedTimeouts()} is called. Calling this | ||
| * with a later deadline extends the window; an earlier one is ignored. | ||
| * @param period maximum duration of the relaxation window | ||
| */ | ||
| void relaxTimeouts(Duration period) { | ||
| long deadline = clockNanos.getAsLong() + period.toNanos(); | ||
| if (relaxedUntilNanos == 0 || deadline - relaxedUntilNanos > 0) { | ||
| relaxedUntilNanos = deadline; | ||
| @Experimental | ||
| void relaxTimeouts(long expiration) { | ||
| if (this.relaxedTimeout == null) { | ||
| throw new IllegalStateException("Relaxed timeouts not activated"); |
Comment on lines
12
to
16
| public interface JedisClientConfig { | ||
|
|
||
| /** Sentinel meaning "no relaxed override configured" — falls back to the base timeout. */ | ||
| int UNSET_TIMEOUT_MS = -1; | ||
|
|
||
| /** True iff {@code millis} represents a configured timeout (not {@link #UNSET_TIMEOUT_MS}). */ | ||
| static boolean isTimeoutSet(int millis) { | ||
| return millis != UNSET_TIMEOUT_MS; | ||
| } | ||
|
|
||
| default RedisProtocol getRedisProtocol() { | ||
| return null; | ||
| } |
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
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.
Note
High Risk
Touches core Connection read paths, pooled return semantics, and cluster maintenance timeout/rebind behavior; removes UNSET_TIMEOUT_MS and changes relaxed blocking defaults.
Overview
Replaces per-operation
SoTimeoutSupplierand scattered timeout fields onConnectionwith aTimeoutSupplierChain(DefaultTimeoutSupplier,ExpiringTimeoutSupplier, optional poolcustomTimeoutSupplier) that resolves normal vs relaxed blocking/non-blockingSO_TIMEOUTvalues. Socket timeouts are applied viaapplySoTimeout(currentTimeout())ahead of reads instead of the previouseffectiveSoTimeout()logic insidereadProtocolWithCheckingBroken.Maintenance setup moves out of
Connectioninto anInitVisitor(MaintenanceAwareVisitor) registered fromConnectionFactory, which enablesCLIENT MAINT_NOTIFICATIONS, relaxed timeouts, and push consumers after init.MaintenanceEventControllernow exposes a shared timeout supplier for MOVING rebind windows andisRebinding(Connection);ConnectionPooluses areturnHookto return affected connections as broken instead ofrequestRebind()on close.MOVING handling shifts from
requestRebind()toexpireAt, and relaxed windows use nanosecond deadlines viaNanoClock.JedisClientConfig.UNSET_TIMEOUT_MSis removed;MaintenanceNotificationsConfigis immutable with renamed relaxed timeout accessors and a default relaxed blocking timeout of0(was inherit-via-unset).Reviewed by Cursor Bugbot for commit eb044bc. Bugbot is set up for automated code reviews on this repo. Configure here.