Conversation
A replica is excluded from reads as soon as INFO reports master_link_status:down. That is the right call when the replica has lost a live master, but it is also what every replica of a dead master reports for the whole failover window, while the master's own health bit is never cleared. Every MasterAndSlaves / PreferSlaves read to that shard fails with no_alive_connection until CLUSTER SLOTS names a new master, though the replicas keep serving the freshest data there is. A broken link now excludes a replica only once master_link_down_since_seconds reaches ReplicaLinkDownTolerance (60s by default), which bounds how stale a read can get. Redis reports -1 for a replica that has never synced since it started, and its dataset is then anything from empty to the RDB it booted from, so such a replica stays excluded. MASTERDOWN, the reply of a replica with replica-serve-stale-data set to no, is retried on another host like LOADING: the command was rejected before it ran.
|
I am holding my review ATM, due to plans about splitting this PR. |
b4572f5 to
5b6bca9
Compare
PeterIvanov
left a comment
PeterIvanov
left a comment
There was a problem hiding this comment.
None of my comments are blocking.
| defaultCheckInterval = 5 * time.Second | ||
| defaultWaitToMigrate = 20 * time.Millisecond | ||
| // defaultReplicaLinkDownTolerance outlives a failover at cluster-node-timeout of | ||
| // up to ~50 seconds, and stays well below the ~160 seconds after which redis itself |
There was a problem hiding this comment.
Reader may find 50 seconds a magical number. Where does it come from?
There was a problem hiding this comment.
Fair point — rewrote the comment to derive the number from its components (cluster-node-timeout plus an election, one CheckInterval for the client to notice the new master, and the ~160s bound redis itself uses for promotion) instead of stating ~50s.
| } | ||
|
|
||
| if cluster.opts.ReplicaLinkDownTolerance == 0 { | ||
| cluster.opts.ReplicaLinkDownTolerance = defaultReplicaLinkDownTolerance |
There was a problem hiding this comment.
One might argue that negative value is a safer default.
There was a problem hiding this comment.
Leaving the default as is. A negative default would keep the pre-PR behaviour on for everyone: while the shard has no master it is unavailable even for reads, even when the client explicitly asked for PreferSlaves as its ReplicaPolicy. That outage is what this PR exists to remove, so I'd rather not ship it as the default.
| } | ||
| } | ||
|
|
||
| func (s *shard) setReplicaInfo(res interface{}, n uint64, tolerance time.Duration) { |
There was a problem hiding this comment.
Even within shard scope, tolerance seems too generic to be a good name.
There was a problem hiding this comment.
Good point, renamed to linkDownTolerance.
| } | ||
|
|
||
| func infoInt(info []byte, field string) (int64, bool) { | ||
| key := []byte("\n" + field + ":") |
There was a problem hiding this comment.
Just a note: a pre-PR code did not require \n before loading:1, for example. This function is not being used there, though.
There was a problem hiding this comment.
Good catch. There are actually two problems here. First, a plain bytes.Contains for loading:1 also picks up e.g. async_loading:1. Second, in theory the first field may come without a preceding \n, although in practice Redis/Valkey never emit it that way — INFO always opens with a section header. Fixing both: every lookup now goes through infoField, which matches the name at the start of a line or at the very start of the buffer.
A substring search for loading:1 also matches async_loading:1 on redis 7+, where the replica keeps serving its old dataset while the new one loads. Every field lookup now goes through one parser that anchors the name to the start of a line.
3 participants
A replica is excluded from reads as soon as
INFOreportsmaster_link_status:down.That is the right call when the replica has lost a live master, but it is also what
every replica of a dead master reports for the whole failover window, while the
master's own health bit is never cleared. Every
MasterAndSlaves/PreferSlavesread to that shard fails with
no_alive_connectionuntilCLUSTER SLOTSnames a newmaster, though the replicas keep serving the freshest data there is.
A broken link now excludes a replica only once
master_link_down_since_secondsreaches
ReplicaLinkDownTolerance(60s by default), which bounds how stale a read canget. Redis reports
-1for a replica that has never synced since it started, and itsdataset is then anything from empty to the RDB it booted from, so such a replica stays
excluded.
MASTERDOWN, the reply of a replica withreplica-serve-stale-dataset tono, isretried on another host like
LOADING: the command was rejected before it ran.