meta: serve a shard's replicas nearest-first to the caller that asked - #144
Merged
Conversation
MasterGetTableTopoRequest, the wire type behind /MasterService/GetTableTopo, has carried an idc field all along, and the route drops it: only namespace, table name and version are forwarded. So every caller announces its location on every topology request, and every caller gets back the same answer. That answer is ordered by placement preference, not proximity, and placement deliberately spreads replicas as far apart as the topology allows - that is what the separation ladder in meta/location.rs is for. So most of a shard's replicas are far from any given caller by construction. With replicas in east/zone-a, east/zone-b and west/zone-c, the order every caller receives today is node-a, node-c, node-b: the replica on the other side of the country offered second, ahead of the one in the neighbouring zone. GetTableTopologyRequest gains an optional client_location, the route stops dropping idc, and each shard's replicas are ordered nearest-first using the shared_prefix_len that meta/location.rs already provides for placement. Three properties hold it in place. The primary never moves: reordering is about which copy to read, not who owns the shard. replica_endpoints is reordered with replicas, because the two lists are positional and reordering one alone would hand every caller the wrong address for every replica. And the sort is stable, so among servers a caller cannot tell apart the placement scan's load ordering survives. Locations are hierarchical, so "no replica in my zone" is not the end of the question: a caller in east/zone-d shares east with two of the three replicas and gets both ahead of the western one. The field is serde-default, and an empty or unrecognised location leaves the order byte-identical to today, with a test for exactly that. Six tests: the caller's own zone first, the hierarchical fallback, the primary staying put, endpoints staying aligned, silence changing nothing, and the stable-sort tie-break.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The caller already tells us where it is, and we throw it away
MasterGetTableTopoRequest— the wire type behindPOST /MasterService/GetTableTopo— has carried anidcfield all along:The route forwards
namespace,table_nameand the version, and drops the rest. So every caller announces its location on every topology request, and every caller gets back the same answer.That answer is ordered by placement preference, not by proximity. And placement deliberately spreads replicas as far apart as the topology allows — that is the whole point of the separation ladder in
meta/location.rs. Which means most of a shard's replicas are far from any given caller by construction.The test makes this concrete. Three replicas —
east/zone-a,east/zone-b,west/zone-c— and the order every caller currently receives:The replica on the other side of the country is offered second, ahead of the one in the neighbouring zone, to a caller that may well be sitting in
east/zone-b.What this changes
GetTableTopologyRequestgains an optionalclient_location, the route stops droppingidc, and each shard's replicas are ordered nearest-first using theshared_prefix_lenthatmeta/location.rsalready provides for placement.Three properties hold it in place:
replica_endpointsis reordered withreplicas. The two lists are positional; reordering one alone would hand every caller the wrong address for every replica.Locations are hierarchical, so "no replica in my zone" is not the end of the question: a caller in
east/zone-dshareseastwith two of the three replicas and gets both of them ahead of the western one.Compatibility
The field is
#[serde(default)], and an empty or unrecognised location leaves the order byte-identical to today. A caller that says nothing sees exactly what it saw before — there is a test for precisely that.Tests
6 new, covering: the caller's own zone coming first, the hierarchical fallback, the primary staying put, endpoints staying aligned, silence changing nothing, and the stable-sort tie-break.
Verification:
cargo test -p temporalstore-rust --lib meta -- --test-threads=1— 267 passed, 2 failed.cargo check -p temporalstore-rust --all-targets— clean.Correction. I originally wrote that these two fail on clean
main. That was wrong. Re-runningproxy::testson its own gives 54 passed, 0 failed both onmainand on this branch. The failures I saw were port collisions — the proxy tests bind fixed ports, and another suite was running against the same ports in a second worktree at the time; one of them reportedAddrInUseoutright, which I should have read as the signal it was.mainwas not red, and this change does not affect these tests.Correction to the note above about the two proxy tests.
I wrote that they fail on clean
main. That was wrong, and I am striking it. Runningproxy::testson its own gives 54 passed, 0 failed — onmainand on this branch alike.What I actually hit was a port collision: the proxy tests bind fixed ports, and a second worktree was running its own suite against the same ports at the time. One of the failures said
AddrInUseoutright, which I should have read as the signal it was instead of treating the set as a baseline.mainwas not red, and this change does not touch these tests.