Skip to content

RouteByLatency picks a strict minimum, so equally-close nodes get no read traffic #3974

Description

@jozenstar

Expected Behavior

With RouteByLatency enabled on a cluster or failover-cluster client, read-only commands
should be distributed across nodes that are effectively equidistant from the client, so a set
of replicas in the same locality shares the read load.

Current Behavior

clusterState.slotClosestNode selects the strict minimum, with no tolerance band:

for _, n := range nodes {
    if closestNode == nil || n.Latency() < minLatency {
        closestNode = n
        minLatency = n.Latency()
        ...
    }
}

Every client therefore converges on whichever node measured marginally lowest, and its
equally-close peers take no read traffic at all. The estimate driving that decision is coarse
updateLatency takes the mean of ten PINGs of a sub-millisecond operation, and the value
is refreshed at most every 10s (minLatencyMeasurementInterval) — so a difference well inside
the measurement noise decides the entire read load for the next interval.

Measured on a sentinel-backed NewFailoverClusterClient across three availability zones,
GETs per second on a five-replica set:

97.7   71.4   7.6   4.3   0.17        -> 590x spread
per-pod CPU: 38m 34m 39m 26m 27m      -> flat

CPU is flat, so the split follows the latency ranking rather than any real difference in
capacity. Locality itself works well — enabling RouteByLatency cut our cross-AZ transfer by
about 70% — but within a zone one replica of five was serving 0.17 reads/s. Egress showed
the same imbalance (4.81 vs 1.73 Gbit/s between the busiest and quietest), which is the part
that bites: the busiest node was at 77% of its instance's sustained network baseline while
others idled.

Possible Solution

An opt-in tolerance: treat nodes within a configurable duration of the fastest as equal
candidates and round-robin between them using the existing ShardPicker. A node an
availability zone away is roughly 4x further off (~0.6ms vs ~0.15ms), so it stays outside any
sensible band and locality is preserved. A zero default keeps today's behaviour byte for byte.

Steps to Reproduce

  1. Run a Redis Sentinel set with several replicas reachable at similar latency (in practice,
    two or more replicas in the same availability zone as the client).
  2. Create a client with RouteByLatency: true:
    rdb := redis.NewFailoverClusterClient(&redis.FailoverOptions{
        MasterName:     "mymaster",
        SentinelAddrs:  []string{":26379"},
        RouteByLatency: true,
    })
  3. Issue read-only commands from several client processes.
  4. Compare per-node cmdstat_get from INFO commandstats. The equally-close replicas do not
    share the load; one takes essentially all of it, and the winner changes when the latency
    estimate is refreshed.

Context (Environment)

Sentinel-backed failover cluster client, three availability zones, five replicas per set,
go-redis v9.18.0 (behaviour is unchanged on current master).

We adopted RouteByLatency specifically for zone locality and it delivered — but we now have
replicas that are provisioned and paid for while serving almost no reads, and a hot node
approaching its network baseline. There is no way to express "these nodes are close enough,
spread across them" with the current options: RouteRandomly discards locality entirely, and
ShardPicker is Next(total int) int, an index with no node identity, so it cannot make a
locality-aware choice either.

Detailed Description

While reading slotClosestNode I also think there is a latent bug, independent of the above.
closestNonFailingNode is only assigned inside the n.Latency() < minLatency branch, so if
the fastest node is failing and a healthy node is slower, the healthy node can never be
recorded — it cannot enter the if. closestNonFailingNode stays nil, allNodesFailing
stays true, and selection falls through to the "all nodes are marked as failed" path even
though a healthy node was available.

Possible Implementation

Opened as #3973, which adds ClusterOptions.RouteByLatencyTolerance (plumbed through
FailoverOptions), round-robins the within-tolerance candidates via the existing
ShardPicker, and fixes the healthy-node selection bug above with a test that fails on the
current implementation. Happy to adjust the API shape — a ratio rather than a duration, or
folding it into RouteByLatency itself — if you'd prefer a different design.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions