Support client-side caching (RESP3 tracking) for Redis Cluster - #3885
Support client-side caching (RESP3 tracking) for Redis Cluster#3885jruaux wants to merge 10 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 321ff1a326
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| public V get(K key) { | ||
| return connection.sync().get(key); |
There was a problem hiding this comment.
When callers configure the cluster connection with ReadFrom set to a replica mode, this sync().get(key) is routed as a READ command and PooledClusterConnectionProvider#getConnectionAsync can select replica connections. enable(...) only turns CLIENT TRACKING on for upstream host/port connections, so those replica reads are never registered for tracking and later writes can leave entries in the local cache stale. Route cache reads through the tracked upstream connection for the key's slot, or enable tracking on the same read connections selected by ReadFrom.
Useful? React with 👍 / 👎.
| // use the host/port connection: slot-routed commands are served by connections keyed | ||
| // by host and port, not by the nodeId-keyed connections | ||
| RedisURI uri = node.getUri(); | ||
| connection.getConnection(uri.getHost(), uri.getPort()).sync().clientTracking(tracking); |
There was a problem hiding this comment.
With a cluster client configured for RESP2, this issues plain CLIENT TRACKING ON on each node and then returns a cache frontend, but the cluster implementation does not create a Pub/Sub redirection path and the Javadoc for this overload says redirection is unsupported. In RESP2, invalidations are not delivered back on the command connection, so such frontends can retain stale entries; fail fast unless the node connections negotiated RESP3 before enabling tracking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1567405: each node connection's negotiated protocol version is checked and setup fails with IllegalStateException unless RESP3.
| List<Object> content = message.getContent(codec::decodeKey); | ||
| List<K> keys = (List<K>) content.get(1); | ||
| keys.forEach(listener); |
There was a problem hiding this comment.
When Redis sends a client-tracking invalidation for FLUSHALL or FLUSHDB, the payload is a null message rather than a key list. This code unconditionally casts content.get(1) to a list and dereferences it, so a flush while entries are cached throws from the push listener and leaves the local cache populated with values Redis has deleted; treat null invalidations as clear-all before iterating keys.
Useful? React with 👍 / 👎.
| // use the host/port connection: slot-routed commands are served by connections keyed | ||
| // by host and port, not by the nodeId-keyed connections | ||
| RedisURI uri = node.getUri(); | ||
| connection.getConnection(uri.getHost(), uri.getPort()).sync().clientTracking(tracking); |
There was a problem hiding this comment.
If callers pass TrackingArgs.Builder.enabled().redirect(...), Redis sends invalidations to that client ID rather than back over the per-node connections this cache listens to. The method still returns a frontend, so after frontend.get(key) a write from another client can update Redis without evicting clientCache; since the Javadoc says redirection is unsupported for cluster, reject redirected tracking args or consume from the redirected connection instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1567405: added TrackingArgs.isRedirect() and the cluster enable(...) rejects redirected tracking args.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- reject TrackingArgs REDIRECT for cluster tracking (add TrackingArgs.isRedirect()) - fail fast when a node connection did not negotiate RESP3 - enable tracking on replica read-intent connections so ReadFrom replica reads are tracked - treat null invalidation payloads (FLUSHALL/FLUSHDB) as clear-all via new CacheAccessor.clear() and RedisCache.addClearListener() Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1567405f75
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if (node.is(RedisClusterNode.NodeFlag.REPLICA)) { | ||
| enableTracking(connection, node, ConnectionIntent.READ, tracking); |
There was a problem hiding this comment.
This branch unconditionally opens a READ connection and sends CLIENT TRACKING to every node flagged as a replica. In the cluster provider, slot reads only use READ replica connections when ReadFrom is set away from the default upstream policy and after applying the ReadFrom/candidate filtering, so an unreachable or otherwise non-selected replica can make enable(...) fail even though all cache reads would go to upstream nodes. Consider enabling replica tracking only for replicas the current read policy can select, or doing it lazily when a replica read connection is actually used.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f81f808: replica tracking is now driven by the connection's ReadFrom policy; with no ReadFrom configured, no replica connections are opened.
Avoid opening read-intent connections to replicas that the connection's ReadFrom setting can never select for reads; with no ReadFrom configured, reads are routed to upstream nodes and no replica connections are opened. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f81f808625
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| @Override | ||
| public void addInvalidationListener(java.util.function.Consumer<? super K> listener) { |
There was a problem hiding this comment.
This new method signature inlines java.util.function.Consumer even though there is no same-named type clash in this file. The repository Java rules require imports instead of package-qualified names except for genuine clashes, and this convention is not enforced by formatting, so please add the import and use the simple name here. .agents/rules/java-rules.mdL8-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 40bcf1d: Consumer is now imported.
| connection.addListener((node, message) -> { | ||
| if (message.getType().equals("invalidate")) { | ||
|
|
||
| List<Object> content = message.getContent(codec::decodeKey); |
There was a problem hiding this comment.
For cluster caches using a RedisCodec whose decodeKey cannot decode arbitrary ASCII control strings, this decodes the whole push frame, including the leading invalidate type, as a key. PushOutput.getContent(Function) applies the decoder to every ByteBuffer, so the listener can throw before evicting any cached key; use the already available message type and decode only the key list payload.
Useful? React with 👍 / 👎.
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| List<RedisNodeDescription> nodes = new ArrayList<>(connection.getPartitions().getPartitions()); |
There was a problem hiding this comment.
When callers use a custom ReadFrom whose selection depends on the supplied Nodes set, this builds one global selection from all cluster nodes, while PooledClusterConnectionProvider#getReadConnection calls ReadFrom.select separately with only the master/replicas for the requested slot. The fresh issue in this revision is that later cache GETs can open a per-slot replica connection that was not in the global selection and therefore never received CLIENT TRACKING, so entries read from that replica are populated but not invalidated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 40bcf1d: ReadFrom.select(...) now runs per slot group (upstream + its replicas), mirroring PooledClusterConnectionProvider's per-slot selection.
| } | ||
|
|
||
| @Override | ||
| public void addInvalidationListener(java.util.function.Consumer<? super K> listener) { |
There was a problem hiding this comment.
This new method signature inlines java.util.function.Consumer even though there is no same-named type clash in this file. The repository Java rules require imports instead of package-qualified names except for genuine clashes, and this convention is not enforced by formatting, so please add the import and use the simple name here.
Useful? React with 👍 / 👎.
| public V get(K key) { | ||
| return connection.sync().get(key); |
There was a problem hiding this comment.
When callers pass TrackingArgs.Builder.enabled().optin(), Redis only tracks reads that are immediately preceded by CLIENT CACHING YES on the same node connection. This cluster frontend routes the GET directly and never sends that per-node opt-in command, so it can populate clientCache with values that Redis never registers for invalidation; reject OPTIN for this factory or issue CLIENT CACHING YES on the selected node before each cache miss read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 40bcf1d: added TrackingArgs.isOptin() and the cluster enable(...) rejects OPTIN tracking args.
- reject OPTIN tracking (add TrackingArgs.isOptin()); the frontend never issues CLIENT CACHING yes - select replica read candidates per slot group (upstream + its replicas) to mirror the cluster connection provider's per-slot ReadFrom selection - decode only the invalidation key payload instead of the whole push frame - import Consumer instead of using a fully qualified name - add client-side caching user guide page covering the cluster entry point Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40bcf1d4fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (node.is(RedisClusterNode.NodeFlag.REPLICA) && upstream.getNodeId().equals(node.getSlaveOf())) { | ||
| nodes.add(node); |
There was a problem hiding this comment.
When ReadFrom can select replicas, this mirror of the slot reader includes every replica of the upstream, but the actual cluster read path only treats a replica as a candidate when partition.getReplOffset() != 0 in PooledClusterConnectionProvider#isReadCandidate. In a topology with a replica present but not yet replicating (offset 0), enable(...) opens and sends CLIENT TRACKING to a node that cached reads will not use; if that unused replica is unavailable or still handshaking, cache setup fails even though the normal reader could use the upstream or another valid replica. Apply the same replication-offset filter before adding the replica.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 59da6e0: replica read candidates are filtered by getReplOffset() != 0, mirroring PooledClusterConnectionProvider#isReadCandidate.
| With RESP3, invalidation messages are delivered as push messages on the | ||
| connection itself. With RESP2, use `TrackingArgs` redirection | ||
| (`TrackingArgs.Builder.enabled().redirect(clientId)`) to deliver invalidations | ||
| to a Pub/Sub connection. |
There was a problem hiding this comment.
In the CacheFrontend flow documented above, this RESP2 guidance is incomplete: passing TrackingArgs.redirect(...) to ClientSideCaching.enable(...) still installs DefaultRedisCache's invalidation listener on the data connection, while Redis sends RESP2 invalidations only to the redirected Pub/Sub client. Users following this will populate the local cache but never evict it unless they also subscribe and call CacheAccessor.evict from that Pub/Sub listener, so the doc should show that wiring or state that the helper example is RESP3-only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 59da6e0: the docs now state the example is RESP3-only and describe the redis:invalidate Pub/Sub eviction wiring required for RESP2 redirection.
| if (node.is(RedisClusterNode.NodeFlag.UPSTREAM)) { | ||
| enableTracking(connection, node.getUri(), ConnectionIntent.WRITE, tracking); |
There was a problem hiding this comment.
When the cluster view contains an empty upstream/master, such as a node just added before slots are assigned, slot-routed cache GET/SET calls cannot use that node because it owns no key slots. This loop still opens it and requires CLIENT TRACKING/RESP3, so an unreachable or still-joining empty master can make cache setup fail even though all assigned slots remain serviceable through other masters; filter out hasNoSlots() upstreams (and their read groups) until they actually own slots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 59da6e0: upstream nodes that own no slots are skipped when enabling tracking.
| // null payload indicates a full invalidation, e.g. after FLUSHALL/FLUSHDB | ||
| clearListeners.forEach(Runnable::run); | ||
| } else { | ||
| keys.forEach(key -> listener.accept(codec.decodeKey((ByteBuffer) key))); |
There was a problem hiding this comment.
With another cluster push listener or a second cache frontend on the same connection, and a key codec that advances the supplied ByteBuffer (for example ByteArrayCodec), this decodes the nested key buffers returned by message.getContent() in place. PushOutput#getContent() only protects top-level buffers, so the first listener can leave the shared nested buffers at their limit and later listeners decode empty keys, leaving their caches stale; decode from a duplicate/read-only buffer or use the recursive decode helper for the key payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 59da6e0: invalidation keys are decoded from duplicated buffers so shared nested buffers are not consumed for other listeners.
| for (RedisNodeDescription node : readCandidates(connection)) { | ||
| // reads from upstream nodes are served by the write-intent connections tracked above | ||
| if (node.getRole().isReplica()) { | ||
| enableTracking(connection, node.getUri(), ConnectionIntent.READ, tracking); |
There was a problem hiding this comment.
This computes the tracked read candidates only once during enable(...), but the same cluster connection exposes setReadFrom(...) and ClusterRedisCache.get keeps delegating future misses to the connection's current read policy. If the cache is enabled while reads use upstreams and the application later switches to ReadFrom.REPLICA or another replica-selecting policy, those newly selected read connections never received CLIENT TRACKING, so values populated from them can remain stale; either prevent changing the read policy for the frontend or enable tracking when the policy changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 59da6e0: documented in the Javadoc and user guide that setReadFrom(...) changes after enabling are not tracked and require re-enabling tracking.
- skip upstream nodes that own no slots when enabling tracking - filter replica read candidates by replication offset, mirroring PooledClusterConnectionProvider#isReadCandidate - decode invalidation keys from duplicated buffers so shared buffers are not consumed for other listeners - treat null entries in the invalidation key list as clear-all (defensive) - document that setReadFrom changes after enabling are not tracked (Javadoc and docs) - document RESP2 redirect Pub/Sub eviction wiring in the user guide - add flushall regression test for cluster cache clear Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DefaultRedisCache now handles null invalidation payloads (FLUSHALL/FLUSHDB) through the clear listener, decodes only the key payload from duplicated buffers, and imports Consumer. Adds a standalone flushall regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79bef0e5bb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (RedisNodeDescription node : readCandidates(connection)) { | ||
| // reads from upstream nodes are served by the write-intent connections tracked above | ||
| if (node.getRole().isReplica()) { | ||
| enableTracking(connection, node.getUri(), ConnectionIntent.READ, tracking); |
There was a problem hiding this comment.
When ReadFrom selects more than one node for a slot, this eager tracking step opens every selected replica and fails the entire cache setup if any one of those connections cannot be established. The normal cluster read path is more tolerant: PooledClusterConnectionProvider#getReadConnection keeps the successfully connected candidates and only fails when all read candidates fail, so configurations like ReadFrom.UPSTREAM_PREFERRED, REPLICA_PREFERRED, or ANY can still serve reads while one replica is temporarily down. In that environment, enabling client-side caching now rejects an otherwise usable cluster connection; track only the candidates that the provider can actually use, or defer tracking to successful read connections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8cafd4e: unreachable replicas no longer fail cache setup; tracking is skipped with a warning, matching the read path's fault tolerance.
| LettuceAssert.assertState(protocolVersion == ProtocolVersion.RESP3, | ||
| "Client-side caching for Redis Cluster requires RESP3"); | ||
|
|
||
| nodeConnection.sync().clientTracking(tracking); |
There was a problem hiding this comment.
If a tracked cluster node connection reconnects after a network drop or node restart, this completed CLIENT TRACKING command is not replayed by the reconnect handshake; RedisHandshake#applyPostHandshake only restores connection state such as SELECT and READONLY. The cache frontend and push listener remain alive, so later cache misses on the replacement node connection can populate clientCache without Redis tracking those reads, and subsequent writes on that node will not evict the local entry. Persist/reapply the tracking args for tracked node connections on reconnect, or clear/disable the frontend when reconnects occur.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8cafd4e: a RedisConnectionStateListener re-applies CLIENT TRACKING on each tracked node connection when it reconnects.
- tolerate unreachable replicas when enabling tracking; log a warning instead of failing setup, matching the read path's fault tolerance - re-apply CLIENT TRACKING when a tracked node connection reconnects, since the reconnect handshake does not restore tracking state - preserve partition order when building per-slot read candidate groups, mirroring PooledClusterConnectionProvider#getReadCandidates Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cafd4ee2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| public void onRedisConnected(RedisChannelHandler<?, ?> connectionHandler, SocketAddress socketAddress) { | ||
| nodeConnection.async().clientTracking(tracking); |
There was a problem hiding this comment.
If a cache miss is issued while a tracked node connection is disconnected, Lettuce buffers that GET; on reconnect, CommandHandler#channelActive activates the endpoint and flushes the disconnected buffer before ConnectionEventTrigger notifies this RedisConnectionStateListener. The fresh evidence is that the replay is now scheduled from onRedisConnected, so the async CLIENT TRACKING can be appended after the queued GET, allowing that miss to cache a value before Redis is tracking it and leaving the entry stale after subsequent writes.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cafd4ee2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| public void onRedisConnected(RedisChannelHandler<?, ?> connectionHandler, SocketAddress socketAddress) { | ||
| nodeConnection.async().clientTracking(tracking); |
There was a problem hiding this comment.
When the caller mutates the TrackingArgs instance after enable(...) returns, this reconnect listener reuses that same mutable object rather than the options that were validated and enabled initially. After a reconnect, this can replay CLIENT TRACKING with later changes such as OFF, REDIRECT, or OPTIN, allowing subsequent cache misses on that node to be stored without usable invalidations; snapshot or defensively copy the tracking parameters before storing them for reconnect replay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f16d3d8: added TrackingArgs.copy() and the reconnect listener replays a snapshot taken at enable time.
| subscribe to the `__redis__:invalidate` Pub/Sub channel on the redirect target | ||
| connection and evict entries from the client-side cache | ||
| (`CacheAccessor.evict(...)`) in the Pub/Sub listener yourself. |
There was a problem hiding this comment.
For RESP2 redirection, full invalidations from FLUSHALL/FLUSHDB have no key payload, so a Pub/Sub listener that only calls CacheAccessor.evict(...) as described here cannot remove any specific entry and will leave the local cache stale after a flush. The manual RESP2 wiring should also tell users to call CacheAccessor.clear() when the invalidation message is null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f16d3d8: the RESP2 wiring docs now instruct calling CacheAccessor.clear() when the invalidation message is null.
- snapshot TrackingArgs (new copy()) so reconnect replay does not observe later mutations - validate tracking args: reject null and disabled tracking (add TrackingArgs.isEnabled()) - add @throws tags to the cluster enable() Javadoc - document CacheAccessor.clear() for RESP2 null flush invalidations Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…cking setup - clear the client-side cache when a tracked node connection reconnects, since Redis drops tracking state with the old connection and cached entries would no longer receive invalidations - log a warning when the tracking re-enable after reconnect fails instead of failing silently - roll back tracking and reconnect listeners on earlier nodes when enabling tracking fails midway, so a failed enable() leaves no half-configured connections Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bec4d1701f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high 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 bec4d17. Configure here.
…ring reconnect replay - reject prefix-limited BCAST tracking (add TrackingArgs.hasPrefixes()); the frontend caches all keys, so keys outside the prefixes would never be invalidated - clear the cache again after the reconnect tracking re-enable completes, evicting entries populated by commands replayed before tracking was restored Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3828fa464
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (e != null) { | ||
| LOG.warn("Cannot re-enable key tracking on {} after reconnect, reads from this node are not tracked", | ||
| uri, e); |
There was a problem hiding this comment.
If re-enabling CLIENT TRACKING after a reconnect fails (for example because ACLs changed or Redis returns a command error while the connection can still serve GET), this branch only logs the error. The frontend remains active, so future cache misses on that node are still stored by ClusterRedisCache.get even though Redis is no longer tracking those reads, and later writes will not evict them; disable/close the frontend or prevent caching on this node until tracking is restored successfully.
Useful? React with 👍 / 👎.
1 participant
This PR addresses #1380 (Support client-side caching in Redis Cluster mode): it adds server-assisted client-side caching support for Redis Cluster connections.
ClientSideCachinggains cluster-awareenable(...)andcreate(...)factory methods that turn onCLIENT TRACKINGon each upstream node connection (invalidations originate from the node serving a key's slot), backed by a newClusterRedisCacheimplementation and aClusterClientsideCachingIntegrationTestsintegration test suite. Requires RESP3; redirection mode is not supported for cluster.Closes #1380
Make sure that:
mvn formatter:formattarget. Don’t submit any formatting related changes.🤖 Generated with Claude Code
Note
Medium Risk
Touches cache invalidation and cluster connection tracking, where bugs can serve stale data. Additive feature with integration tests, but reconnect/topology edge cases are subtle.
Overview
Adds server-assisted client-side caching for Redis Cluster via new
ClientSideCaching.enable/createoverloads that turn onCLIENT TRACKINGon each relevant node connection (upstream write-intent, plus replica read-intent whenReadFromcan select them).Introduces
ClusterRedisCacheto consume invalidate push messages across cluster nodes. Also adds full-cache clear support forFLUSHALL/FLUSHDB(CacheAccessor.clear(),RedisCache.addClearListener) and reconnect handling that re-applies tracking and clears stale entries. Cluster mode requires RESP3 and rejectsREDIRECT,OPTIN, and prefix-limitedBCAST.Includes user-guide docs and cluster/standalone integration tests covering per-node invalidation and flush clears.
Reviewed by Cursor Bugbot for commit d3828fa. Bugbot is set up for automated code reviews on this repo. Configure here.