feat: support cluster topology discovery via CLUSTER SHARDS command - #4266
feat: support cluster topology discovery via CLUSTER SHARDS command#4266bchrobot wants to merge 3 commits into
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.
| # If there's only one server in the cluster, its ``host`` is '' | ||
| # Fix it to the host in startup_nodes | ||
| if ( | ||
| len(cluster_slots) == 1 |
There was a problem hiding this comment.
Misleading shards discovery error
Medium Severity
Any ResponseError from the topology command is rewritten as “Cluster mode is not enabled on this node”. With ClusterShardsTopologyProvider, Redis older than 7.0 returns an unknown-command error for CLUSTER SHARDS, so initialization fails with a false diagnosis instead of indicating an unsupported command.
Additional Locations (1)
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".
| # it at the host the topology command was sent to, so leave the host empty | ||
| # rather than substituting ``ip``, which is the unreachable internal address. | ||
| endpoint = node.get("endpoint") | ||
| host = "" if endpoint is None else _as_str(endpoint) |
There was a problem hiding this comment.
When CLUSTER SHARDS reports endpoint as ? (Redis uses this for hostname-preferred nodes without an announced hostname), this line caches ? as the node host. NodesManager then builds connections to names like ?:7000, so routed commands fail even though the same node map still carries its ip; handle ? as an unknown endpoint, for example by falling back to ip when available or failing discovery clearly instead of treating it as connectable.
Useful? React with 👍 / 👎.
|
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.
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.
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".
| topology = [] | ||
| for slot in response: | ||
| start, end = int(slot[0]), int(slot[1]) | ||
| primary = (_as_str(slot[2][0]), int(slot[2][1])) |
There was a problem hiding this comment.
When an async single-node cluster returns the valid unknown-endpoint form [None, port, ...] from CLUSTER SLOTS, this parser preserves None, while the new NodesManager fallback only recognizes ""; initialization therefore caches a None:port node and later attempts connections with a null host. Before this change, async initialization used not cluster_slots[0][2][0], so this case correctly reused the startup host. This is fresh evidence distinct from the fixed CLUSTER SHARDS path: the current shared SLOTS parser still passes None through, so normalize it to an empty host and allow the queried-host fallback to run.
AGENTS.md reference: AGENTS.md:L159-L163
Useful? React with 👍 / 👎.
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
Touches cluster bootstrap and slot mapping for both sync and async clients. Default remains CLUSTER SLOTS, but a parser bug in the new path could mis-route commands on opt-in deployments.
Overview
Makes cluster slot discovery pluggable so clients can opt into
CLUSTER SHARDS(Redis 7.0+) instead of the defaultCLUSTER SLOTS. Default behavior is unchanged.A
topology_provideron sync and asyncRedisClusternames the command and parses the reply;NodesManagerstill builds the node/slot caches.ClusterShardsTopologyProviderreturns one entry per shard (better on fragmented maps), skips failed/loading replicas while keeping unhealthy primaries, and can prefer TLS ports. Empty/null endpoints stay empty so load-balanced nodes are reached at the queried host, not their internal IP.Custom sources can subclass the provider. Docs and tests cover RESP2/RESP3 reply shapes and parity with
CLUSTER SLOTS.Reviewed by Cursor Bugbot for commit 1111849. Bugbot is set up for automated code reviews on this repo. Configure here.