Add AZ_AFFINITY_ALL_NODES read policy - #6721
Conversation
Signed-off-by: sz-armin <77587091+sz-armin@users.noreply.github.com>
Signed-off-by: sz-armin <77587091+sz-armin@users.noreply.github.com>
Signed-off-by: sz-armin <77587091+sz-armin@users.noreply.github.com>
Signed-off-by: sz-armin <77587091+sz-armin@users.noreply.github.com>
Signed-off-by: sz-armin <77587091+sz-armin@users.noreply.github.com>
Signed-off-by: sz-armin <77587091+sz-armin@users.noreply.github.com>
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| // behavior of these strategies when no local node is known. | ||
| ReadFromReplicaStrategy::AZAffinity(_az) => round_robin_replica(), | ||
| ReadFromReplicaStrategy::AZAffinityReplicasAndPrimary(_az) => round_robin_all_nodes(), | ||
| ReadFromReplicaStrategy::AZAffinityAllNodes(_az) => round_robin_all_nodes(), |
There was a problem hiding this comment.
Preserve ReplicaRequired here instead of applying the all-node fallback unconditionally. Cluster scan calls node_address_for_slot(..., SlotAddr::ReplicaRequired) (commands/cluster_scan.rs:555-560), and ReplicaRequired explicitly means a replica if one exists (cluster_routing.rs:1341-1343); this arm can now select the primary on every rotation even when replicas exist. Branch on slot_addr so AZAffinityAllNodes uses round_robin_replica() for required-replica routes and round_robin_all_nodes() only for optional reads.
There was a problem hiding this comment.
We basically have two options for ReplicaRequired:
- Respect it on both the main read path and the slot-map path (i.e. replica-only here)
The cleaner fix, though it differs from how e.g. AZ_AFFINITY_REPLICAS_AND_PRIMARY handles this path.
- Use all nodes on both paths for consistency
However that would replicate what looks like a bug, and the existing strategies don't seem to agree with each other anyway (e.g. ALL_NODES is already replica-only for ReplicaRequired in lookup_route but not in the slot map).
Since this PR already handles the main path as replica-only, I went with 1 here too. Happy to revert if consistency is preferred.
When no local node is known, ReplicaRequired routes under the AZAffinityAllNodes strategy now use replica rotation instead of falling back to all nodes, matching the strategy's lookup_route behavior. Signed-off-by: sz-armin <77587091+sz-armin@users.noreply.github.com>
e584ef6 to
29caa2a
Compare
2 participants
Summary
We add the
AZ_AFFINITY_ALL_NODESread policy, which unlikeAZ_AFFINITY_REPLICAS_AND_PRIMARYdoes not prioritize replicas before the primary.Issue link
Closes #6653
Features / Behaviour Changes
AZ_AFFINITY_ALL_NODESread policy is added to both the core and Python wrapperget_next_local_replicanow usestry_get_connectionto avoid blocking on mid-reconnect nodes.create_client_from_uriread_fromparser.Implementation
Most of the implementation follows a similar pattern to the existing policies, and should be mostly isolated from existing functionality, except for the two fixes mentioned above.
Additionally, our new policy makes sure explicit replica routes stay replicas-only, which is unlike the current
AZ_AFFINITY_REPLICAS_AND_PRIMARYwhich could potentially end up sending such requests to a primary.Limitations
Testing
Checklist
Before submitting the PR make sure the following are checked:
valkey-glide-docsupdates to follow once the direction and details are confirmed to be acceptable.make *-linttargets) and Prettier has been run (make prettier-fix).valkey-glide-docsupdates to follow once the direction and details are confirmed to be acceptable.