Add latency-based Redis Cluster read load balancing - #4323
CaptainDredge wants to merge 2 commits into
Conversation
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: 6252dad316
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df8501300b
ℹ️ 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".
| latency_attempts = { | ||
| node_name: self._nodes_manager.read_load_balancer.start_request( | ||
| node_name | ||
| ) | ||
| for node_name in nodes |
There was a problem hiding this comment.
When a synchronous pipeline waits for an available connection or performs a slow HIMPORT preflight, these attempts are started only after get_connection() and _himport_prepare_pipeline() have completed, so concurrent reads see no in-flight load and can continue selecting the already-congested node. The async path starts its attempt before ClusterNode.execute_pipeline(), which performs connection acquisition internally, so move the sync start before connection acquisition as well to preserve load accounting and sync/async behavior.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
| getattr(self, "load_balancing_strategy", None) | ||
| == LoadBalancingStrategy.LATENCY_BASED | ||
| ) | ||
| latency_sampling = latency_balancing and self._is_replica_safe(command) |
There was a problem hiding this comment.
When a replica-safe blocking read such as XREAD BLOCK completes normally, this condition enables sampling and records the caller-requested blocking interval as node latency. A successful timeout or delayed stream entry can therefore add seconds to the node's peak estimate and divert unrelated reads from that healthy node for the evidence TTL; blocking commands should retain in-flight accounting but not update historical latency.
Useful? React with 👍 / 👎.
1 participant
Description of change
This pull request resolves #4185 by adding an opt-in
LoadBalancingStrategy.LATENCY_BASEDfor Redis Cluster reads, with matching synchronous and asynchronous behavior. For each read, it samples two eligible nodes from the slot's primary and replicas and chooses the lower client-observed latency-and-load score:The implementation keeps per-client node state, records latency only after successful replica-safe reads, and always releases in-flight accounting on success, errors, redirects, timeouts, or cancellation. Writes contribute only to in-flight load, so a busy primary becomes less attractive without mixing write latency into the read estimator. Pipelines and transactions contribute in-flight load but do not train latency because batch durations are not comparable. Topology refresh preserves surviving node history, prunes departed nodes, and uses generation-tagged attempt tokens so stale completions cannot corrupt replacement-node state. Latency eligibility reuses the command metadata resolver, including registered custom reads, rather than maintaining a second static read-command list.
Assumptions and tradeoffs
RANDOMandROUND_ROBIN; replica staleness semantics are unchanged.0.2, baseline alpha is0.05, the decay period is10s, evidence expires after30s, and cold nodes start at a1msbaseline. These remain internal constants to avoid expanding the public API before operational evidence justifies tuning knobs.The change includes the implementation specification, sync/async unit and integration coverage, user documentation, and a reusable benchmark that externally degrades a Redis replica with
CLIENT PAUSErather than injecting delay inside redis-py.Benchmark results
Two eligible nodes (one primary and one replica):
0.01%0.626 ms; round-robin6,056.736 ms29.862s0Three eligible nodes (one primary and two replicas):
32.29% / 36.57% / 31.14%0.01%51.24% / 48.75%0.794 ms; round-robin20,043.952 ms29.838s0Test coverage
125 passed1,635 passed, 2 skipped3,584 passed, 674 skipped, 26 xpassed2,216 passed, 1,089 skipped, 2 xpassedinvoke all-tests: passed, including Ruff lint/format and Vultureinvoke build-docs: passed (existing documentation warnings remain)git diff --check: passedPull Request check-list
Resolves #4185.
Note
Medium Risk
Changes cluster read routing and command lifecycle in sync/async clients (including pipelines/transactions), but the strategy is opt-in and existing strategies are unchanged; replica staleness and per-client measurement semantics still apply.
Overview
Adds an opt-in
LoadBalancingStrategy.LATENCY_BASEDfor Redis Cluster reads. For each eligible read, the client samples two nodes (primary + replicas), scores them with decaying peak EWMA × in-flight load, and routes to the lower score. Successful replica-safe reads update latency; writes, failures, ASK redirects, and pipeline/transaction batches only affect in-flight accounting. Sync and async_execute_command, slot/keyless routing, topologyreconcile, and pipeline/transaction paths are wired tostart_request/finish_request.Docs (
clustering.rst), an internal spec, a localcluster_latency_load_balancingbenchmark (external replica degradation viaCLIENT PAUSE), and broad unit/integration tests cover the new strategy without changing existing round-robin/random behavior.Reviewed by Cursor Bugbot for commit df85013. Bugbot is set up for automated code reviews on this repo. Configure here.