Conversation
bchrobot
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: 822402dc6c
ℹ️ 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".
| endpoint = _as_str(node.get("endpoint", "")) | ||
| host = endpoint or _as_str(node.get("ip", "")) | ||
|
|
||
| port_keys = ("tls-port", "port") if prefer_tls_port else ("port", "tls-port") |
There was a problem hiding this comment.
When a caller opts into ClusterShardsTopologyProvider() on a TLS cluster using ssl=True or rediss://, Redis can return both the non-TLS port and the tls-port in CLUSTER SHARDS; this default ordering selects the plain TCP port first, then NodesManager builds SSL connections to that non-TLS port and routed commands fail after discovery. The client already knows it is using SSL, so the shards topology path should select tls-port automatically in that configuration instead of requiring a second, easy-to-miss provider flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reasonable point, but deferring it rather than changing it here.
Two reasons. Auto-detecting from the client SSL config would mean the parser reads connection kwargs, which costs the property that makes it cheap to test — it is currently pure and covered directly across all four response shapes. And ssl=True does not strictly imply the TLS port is wanted; a node can advertise both, and the choice is a deployment detail.
The explicit prefer_tls_port flag is discoverable from the provider signature and documented in the Topology Discovery section. If real usage shows the default trips people up on TLS clusters, auto-selection with the flag as an override is a clean follow-up.
Leaving this thread open for a maintainer to weigh in.
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 bbaf4b1. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbaf4b1cf3
ℹ️ 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".
|
Hey @bchrobot, thank you for your contribution! We will have a look at it soon! |
Mukller
left a comment
Mukller
left a comment
There was a problem hiding this comment.
Verified locally against upstream master and this branch (Python 3.13; no live cluster in this environment — noted where it matters).
Test results:
pytest tests/test_cluster.py -k "shards or topology or slots"— 40 passed (8 setup errors = server-dependent fixtures without a local Redis)- Same filter, async suite — 24 passed, 4 failed; those exact 4 fail identically on upstream master (
test_cluster_slots[pool],test_cluster_addslots[pool], etc. — ConnectionRefused class), so unrelated to this diff.
Design assessment — this is a clean abstraction:
- The new
redis/cluster_topology.pyisolates all reply-shape knowledge in one module:_as_mappingnormalizes RESP3 maps vs RESP2 flat arrays,_slot_rangeshandles both paired and flat slot lists, and each provider exposes.command+.parse()— so adding future topology commands is a subclass away. - Default wiring keeps back-compat:
RedisCluster(topology_provider=AsyncClusterSlotsTopologyProvider())reproduces today'sCLUSTER SLOTSbehavior byte-for-byte path-wise. - The LB-endpoint fix (commit title) has a proper rationale comment: a node that doesn't know its own address (behind a load balancer) now yields an empty host instead of the unreachable internal
ip, letting the caller reuse the queried host. That's the right call — substitutingipthere produces connections that can never succeed.
Docs: clustering.rst/connections.rst additions document the provider injection point accurately per the implementation.
Caveat stated plainly: I could not exercise live CLUSTER SHARDS round-trips here, so parser correctness against real 7.x/8.x SHARDS replies rests on the branch's own added tests (which are thorough — 444 new lines covering RESP2/RESP3 shapes, TLS preference, unknown endpoints).
Approving — solid groundwork for SHARDS support.
bbaf4b1 to
1111849
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1111849b02
ℹ️ 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".
1111849 to
7370104
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 737010414f
ℹ️ 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".
NodesManager hardcoded CLUSTER SLOTS and read the reply positionally inline, leaving no seam for an alternative topology source. Introduce ClusterTopologyProvider / AsyncClusterTopologyProvider, mirroring the existing PolicyResolver pattern: a provider names the topology command and parses its reply, while NodesManager keeps connection handling and cache construction. Adding a source is now a new subclass rather than a branch in initialize(). CLUSTER SHARDS requires Redis 7.0+ and returns one entry per shard rather than per slot range, so replies stay small on clusters with fragmented slot maps. It is strictly opt-in via the new topology_provider kwarg; the default remains CLUSTER SLOTS. The shards parser is tolerant of every shape the reply arrives in, because the async stack installs a CLUSTER SHARDS response callback on its node connections while the sync stack receives the raw wire reply. Replicas reported as failed or loading are excluded; an unhealthy primary is retained, since dropping it would leave its slots uncovered.
97d72a7 to
c7b7177
Compare
|
Rebased on upstream |
3 participants
Description of change
Add support for cluster discovery via
CLUSTER SHARDSrather thanCLUSTER SLOTS, deprecated in Redis 7.0+.This introduces
ClusterTopologyProvider/AsyncClusterTopologyProvider, mirroring the existingPolicyResolverpattern: a provider names the topology command and parses its reply, whileNodesManagerkeeps connection handling and cache construction. Adding a source is now a new subclass rather than a branch ininitialize().CLUSTER SHARDSrequires Redis 7.0+ and returns one entry per shard rather than per slot range, so replies stay small on clusters with fragmented slot maps. It is strictly opt-in via the newtopology_providerkwarg; the default remainsCLUSTER SLOTS.The shards parser is tolerant of every shape the reply arrives in, because the async stack installs a
CLUSTER SHARDSresponse callback on its node connections while the sync stack receives the raw wire reply.Replicas reported as failed or loading are excluded; an unhealthy primary is retained, since dropping it would leave its slots uncovered.
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Note
Medium Risk
Changes how cluster slot maps are built on init and refresh; wrong parsing or replica filtering could misroute traffic, though defaults preserve prior behavior and coverage is extensive.
Overview
Introduces pluggable cluster topology discovery via a new
topology_providerargument on sync and asyncRedisCluster. Default behavior is unchanged (CLUSTER SLOTSthroughClusterSlotsTopologyProvider); callers can opt intoCLUSTER SHARDS(Redis 7.0+) withClusterShardsTopologyProviderfor more compact replies on fragmented slot maps.NodesManagerno longer parsesCLUSTER SLOTSinline—it runs the provider’s command and builds slot/node caches from normalized(start_slot, end_slot, primary, replicas)tuples. The shards parser handles RESP2/RESP3 shapes, picks primaries by role, skips unhealthy replicas, supportsprefer_tls_port, and leaves empty/null endpoints for the existing “use the queried startup host” fallback (without routing through internalipbehind load balancers).Topology refresh errors now distinguish unsupported topology commands (“unknown subcommand”) from cluster mode disabled. Docs cover configuration, async providers, and subclassing
ClusterTopologyProviderfor custom discovery.Reviewed by Cursor Bugbot for commit c7b7177. Bugbot is set up for automated code reviews on this repo. Configure here.