Repository navigation
Scan only primary nodes in scanIteration on cluster - #4798
Conversation
On a cluster with replicas, the command iteration base ran SCAN on every known node, so each key was returned by its primary and again by every replica. Iterate the primary nodes map instead, matching how KEYS and other all-shards commands are broadcast. MultiDbConnectionProvider now overrides getPrimaryNodesConnectionMap so it keeps using the active database pool instead of the interface default.
7fe093e to
df932b9
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. |
![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: 7fe093ef70
ℹ️ 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".
* Return pooled connections from iterations on MultiDbClient MultiDbConnectionProvider overrode getConnectionMap() but not getPrimaryNodesConnectionMap(), so the interface default borrowed a pooled connection that AggregateIterator treated as caller-owned and never returned; each ftAggregateIterator call leaked one connection until the pool was exhausted. Return the active database's pool from both methods. The ITs also cover scanIteration and ftAggregateIteration, which switch to the primaries map in #4798. Fixes #4799. Co-authored-by: Raphael Fakhri <153192858+RaphaelFakhri@users.noreply.github.com> * Share the MultiDbClient active-pool test helper Five tests copied the same lookup of the active database's connection pool. MultiDbConnectionProviderHelper.activeConnectionPool replaces them; test code only. --------- Co-authored-by: Raphael Fakhri <153192858+RaphaelFakhri@users.noreply.github.com>
# Conflicts: # docs/release-notes/8.1.0.md
A bug fix with no action required from users.
|
Pushed 27ab24e and f416959 on top of your branch: merged master, which now carries the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4169596db
ℹ️ 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".
The default returned a single getConnection() connection and ignored any getConnectionMap() override; with a pooled provider that connection is borrowed and never returned (redis#4323, redis#4799). Now that iterations read the primaries map, a custom provider overriding only getConnectionMap() would iterate one node through such a connection. Built-in providers override both methods or neither, so they behave as before.
ggivo
left a comment
ggivo
left a comment
There was a problem hiding this comment.
LGTM. scanIteration, ftSearchIteration and ftAggregateIteration now iterate only the primaries, the same nodes all-shards commands are broadcast to, so on a cluster with replicas they no longer return duplicate keys (#4797).
I pushed two commits on top:
- f416959 drops the release-notes entry.
- 25ca6df changes the
getPrimaryNodesConnectionMap()default to returngetConnectionMap(). A custom provider that overrides onlygetConnectionMap()keeps its full map and no longer leaks a pooled connection.
I'll merge once CI is green
|
@RaphaelFakhri |
Summary
On a cluster with replicas,
scanIterationreturns every key once from its primary and again from each replica.JedisCommandIterationBasenow iteratesgetPrimaryNodesConnectionMap()instead ofgetConnectionMap(), soSCANruns once per shard. This matches howKEYSand other all-shards commands are broadcast. Fixes #4797.Key decisions and assumptions
ftSearchIterationandftAggregateIterationalso stop querying replicas.MultiDbConnectionProvideroverrides onlygetConnectionMap(). Without an override,getPrimaryNodesConnectionMap()falls back to the interface default, which borrows a connection that is never returned to the pool. The provider now returns the active database pool from both methods.Behavioral changes
scanIterationreturns each key once. With 3 primaries and 1 replica each, 100 keys previously came back as 200.docs/release-notes/8.1.0.md.Testing
testScanIterationSkipsReplicasmocks a provider whose all-nodes map has a primary and a replica and whose primary map has only the primary, and asserts that each key is returned once. The test fails without the fix (key1, key2, key1, key2) and passes with it. The two existingscanIterationmocked tests now stub the primary-nodes map. Checked by hand against a Redis 8.2 cluster with 3 primaries and 3 replicas: 200 keys before the fix, 100 after.Note
Medium Risk
Changes cluster iteration behavior for scan and related iterators; fixes incorrect duplicates but is a user-visible semantic change for replica-heavy setups.
Overview
Fixes duplicate keys from
scanIterationon Redis Cluster when replicas are included in the provider’s node map.JedisCommandIterationBasenow builds its per-node round-robin fromgetPrimaryNodesConnectionMap()instead ofgetConnectionMap(), soSCANruns once per shard (same idea as other all-shard commands). That also applies to iteration helpers built on the same base (e.g. FT search/aggregate iterations).On
ConnectionProvider, the defaultgetPrimaryNodesConnectionMap()delegates togetConnectionMap()(with docs) so standalone/pooled providers keep the same behavior; cluster providers that override the primary map exclude replicas.Tests stub the primary map for existing
scanIterationmocks, addtestScanIterationSkipsReplicas, and assert the interface default matchesgetConnectionMap().Reviewed by Cursor Bugbot for commit 25ca6df. Bugbot is set up for automated code reviews on this repo. Configure here.