Repository navigation
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 004961aa91
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (slots.isEmpty()) { | ||
| return getConnection(); // a keyless command can run on any node |
There was a problem hiding this comment.
Preserve replica-only routing for keyless commands
When JedisCluster or RedisClusterClient.executeCommandToReplica receives a keyless command such as INFO replication, ReplicaOnlyConnectionResolver reaches this branch, but getConnection() selects exclusively from cache.getShuffledPrimaryNodesPool(). The command therefore runs on a primary despite the replica-only API contract and can return primary-specific state; select an actual replica pool for this empty-slot case instead.
Useful? React with 👍 / 👎.
| if (slots.isEmpty()) { | ||
| return getConnection(); // a keyless command can run on any node |
There was a problem hiding this comment.
Record the keyless-cluster behavior change
This changes keyless cluster commands from throwing NoSuchElementException to executing successfully, which is an externally observable behavior change, but the commit does not add an entry to the running docs/release-notes/8.1.0.md. Add the required release-note entry so users can discover the fix.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
004961a to
0dd02cf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dd02cfbfe
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Map<String, ConnectionPool> replicas = new HashMap<>(getNodes()); | ||
| replicas.keySet().removeAll(getPrimaryNodes().keySet()); |
There was a problem hiding this comment.
Derive replicas from recorded topology roles
During resharding, an ASK redirection to a newly importing primary that is absent from the current slot topology causes ClusterCommandExecutor to call getConnection(HostAndPort), which adds that target to the all-nodes map without adding it to the primary map. This subtraction then misclassifies the importing primary as a replica, so a later keyless executeCommandToReplica can randomly run on that primary. The fresh evidence in the revised code is the new assumption that every non-primary entry in getNodes() is a replica; select from explicitly recorded replica roles instead.
Useful? React with 👍 / 👎.
| if (replicas.isEmpty()) { | ||
| return getConnection(); |
There was a problem hiding this comment.
Refresh topology before concluding replicas are absent
When the provider initially discovers a primary-only topology and a replica is attached later, this empty branch immediately falls back to a primary without renewing the slot cache. Unlike getReplicaConnectionFromSlot, a successful primary fallback produces no error or redirection that would refresh the topology, so without periodic topology refresh subsequent keyless executeCommandToReplica calls can continue running on the primary indefinitely even though a replica is available; renew the topology once before falling back.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| @Test | ||
| public void keylessCommandGetsAReplicaConnectionFromAnyReplica() { |
There was a problem hiding this comment.
Add the required cluster integration regression
This bug fix adds only Mockito tests that stub the connection-selection methods, so it never validates topology discovery, replica classification, or that a keyless command actually reaches the intended Redis node. The repository contribution rules require both unit and integration coverage; add a *IT regression against the cluster environment that exercises keyless primary and replica execution through a real client.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
|
hi @renechoi , thank you for submitting this PR. i will check this in detail in another round and provide my feedback. |
| return replica; | ||
| } | ||
|
|
||
| renewSlotCache(); |
There was a problem hiding this comment.
the issue with this new method is,, in case of no replica in topology, it keeps falling back to slot cache renewal which is pretty expensive operation.
|
|
||
| private Connection getAnyReplicaConnection() { | ||
| Map<String, ConnectionPool> replicas = new HashMap<>(getNodes()); | ||
| replicas.keySet().removeAll(getPrimaryNodes().keySet()); |
There was a problem hiding this comment.
this doesn't respect to the JedisClientConfig.isReadOnlyForRedisClusterReplicas() flag, which excludes the replicas from slot cache when it is set to false
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 24c9c88. Configure here.
|
Both were right, thanks. Fixed in 24c9c88. Replicas now come from the slot cache, which only records them when A keyed |
37eb159 to
d1a1d64
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
getConnection(CommandArguments) and getReplicaConnection took the first element of the hash slot set after checking only for more than one slot, so a keyless command such as PING or INFO threw NoSuchElementException before it could reach the random node fallback the ternary below was written for. That fallback was dead code anyway: JedisClusterCRC16.getSlot only returns 0..16383. Return a connection from any node when the command carries no keys, the same way ClusterPipeline.getNodeKey already handles an empty slot set. On the replica path pick a replica instead, since ReplicaOnlyConnectionResolver routes there to keep the command off the primaries, and fall back to a primary only when the cluster reports no replica, which is what getReplicaConnectionFromSlot already does for a slot with unknown replicas.
The no-argument getReplicaConnection() was extracted only so that the keyless replica path stays on a replica; nothing outside the class calls it. Make it private and drop the release-note sentence that advertised it. The two tests that stubbed or called it now drive the same behaviour through getReplicaConnection(CommandArguments) with a keyless command, so they assert the routing rather than the seam.
The keyless replica path derived its candidates from getNodes() minus getPrimaryNodes() and renewed the slot cache before falling back. That made a cluster without replicas renew the whole topology on every keyless call, an expensive operation for a result the renewal cannot change, and it ignored isReadOnlyForRedisClusterReplicas: replicas are only recorded in the slot cache when that option is on, so a default client routed to nodes the cache never recorded as replicas. Record the replica pools alongside the replica slots and pick from those, so a client that does not track replicas finds none and falls back to a primary immediately. getSlotReplicaPools also returned replicaSlots[slot] unguarded, and that array is null unless the option is on, so the keyed replica path threw NullPointerException for every command on a default client. Return null there instead, which is the "no replica known" value the callers already handle.
The keyed replica path renewed the slot cache whenever the slot had no replica pools, and getSlotReplicaPools returns null for a client that leaves readOnlyForRedisClusterReplicas off, so every keyed executeCommandToReplica on a default client renewed the whole topology before falling back to the slot's primary. That renewal cannot record a replica for such a client, so it was pure cost, the same cost the keyless path stopped paying in the previous commit. Let the cache tell the two cases apart: getSlotReplicaPools returns null only when the client records no replica at all, and an empty list for a recorded slot whose replicas are not known yet, which a renewal can still fill. The keyed path then goes straight to the primary in the first case and keeps renewing in the second.
executeCommandToReplica is the only caller that reaches the keyless branch, so this fix is observable through the public API. The Mockito tests stub connection selection, so they cannot show that a keyless command ends up on a replica of a live topology. The test runs against the cluster-stable environment, three primaries with one replica each. All three cases fail on master: the two keyless ones with NoSuchElementException from getReplicaConnection, and the keyed one with NullPointerException from getReplicaConnectionFromSlot.
3cc9ba4 to
f2a81f9
Compare
Fixes #4720
Summary
Both
CommandArgumentsoverloads callslots.iterator().next()after checking only for more than one slot, so a keyless command (PING, INFO) throwsNoSuchElementException.Key Decisions
The primary path returns
getConnection(), what the deadslot >= 0ternary was for (getSlotnever returns -1). The replica path picks from the replicas the slot cache recorded, which it does only withreadOnlyForRedisClusterReplicason.getSlotReplicaPoolsnow separates the two no-replica cases: null when the client records none, empty when the slot has none yet. Neither replica path renews the cache for a result the renewal cannot change, and the keyed one no longer throwsNullPointerException.Testing
Unit tests cover keyless and keyed routing, both fallbacks and disabled tracking; each new one fails without its fix. 3306 tests, release note updated.
Note
Medium Risk
Changes core Redis Cluster connection selection for keyless and replica reads; fixes exceptions but alters which node handles those commands.
Overview
Fixes cluster routing when a command has no key (
PING,INFO, etc.).ClusterConnectionProviderpreviously calledslots.iterator().next()on an empty hash-slot set and threwNoSuchElementExceptionbefore opening a connection.Primary path: empty slots now use
getConnection()(any reachable primary). Replica path: keyless commands use a random recorded replica via newgetAnyReplicaConnection(), or fall back to a primary when the topology has no replicas.Replica tracking:
JedisClusterInfoCache.getSlotReplicaPools(int)now distinguishes clients that do not record replicas (nullwhenreadOnlyForRedisClusterReplicasis off) from slots with no replica known yet (empty list). Keyed replica routing skipsrenewSlotCache()when renewal cannot add replicas, avoidingNullPointerExceptionand repeated topology refresh on keyless replica calls. AreplicaNodesCacheand publicgetReplicaNodes()support picking any replica for keyless replica commands.Release notes for 8.1.0 document the behavior change; unit and integration tests cover cache semantics and routing fallbacks.
Reviewed by Cursor Bugbot for commit f2a81f9. Bugbot is set up for automated code reviews on this repo. Configure here.