kv, storage: mandate presence of range/store ID in batch requests #25061
Conversation
|
This change is |
|
Reviewed 5 of 5 files at r1. Comments from Reviewable |
benesch
force-pushed
the
mandatory-addressing
branch
from
5c309e4 to
76a32d9
Compare
|
Reviewed 5 of 5 files at r1, 1 of 1 files at r2. pkg/storage/client_replica_test.go, line 552 at r2 (raw file):
nit: give this argument a better name. Comments from Reviewable |
|
That was a lot less painful than I was expecting. This closes #9014. Reviewed 5 of 5 files at r1, 1 of 1 files at r2. Comments from Reviewable |
benesch
force-pushed
the
mandatory-addressing
branch
from
76a32d9 to
70cb938
Compare
Some test code was relying on an ancient and obscure feature of the Stores sender where batch requests without a range and/or store ID could be routed by the request key. Normally, this routing is handled by DistSender. Remove this feature so that requests in tests follow the same path as requests in production. Fix the few tests that were relying on it to manually route their requests to the correct range/store. There are no mixed-version compatibility concerns here. Production code paths, with one exception, have long since guaranteed that batch request headers contain a range ID and store ID by the time they reach the Stores sender. The one exception is during bootstrapping of a fresh cluster, but there are no concerns about version compatibility because the requests are directed to the local store before any other nodes have joined the cluster. That code has been adjusted to use a wrapping sender that installs an appropriate range and store ID. Release note: None
Thanks to the previous commit, Stores.LookupReplica is now unused save for a few tests that can be trivially rewritten to call LookupReplica on the one store within. Rewrite those tests and remove it. We get to remove Stores.FirstRange, too, since its only caller was the test for stores.LookupReplica. Fix cockroachdb#9014. Release note: None
benesch
force-pushed
the
mandatory-addressing
branch
from
70cb938 to
2a6a43a
Compare
Ha! So it does. Thanks for spotting that. With a little more work I was able to nuke Review status: 4 of 8 files reviewed at latest revision, 1 unresolved discussion. pkg/storage/client_replica_test.go, line 552 at r2 (raw file): Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Done. (I thought Comments from Reviewable |
|
LGTM Reviewed 4 of 4 files at r4. Comments from Reviewable |
|
bors r=nvanbenschoten,bdarnell,tschottdorf |
Build failed (retrying...) |
25061: kv, storage: mandate presence of range/store ID in batch requests r=nvanbenschoten,bdarnell,tschottdorf a=benesch Even if this looks good I'll wait until next week to merge it. Stability period and all. Also apologies if I'm sending this to the wrong folks. Still learning who owns what on core. --- Some test code was relying on an ancient and obscure feature of the Stores sender where batch requests without a range and/or store ID could be routed by the request key. Normally, this routing is handled by DistSender. Remove this feature so that requests in tests follow the same path as requests in production. Fix the few tests that were relying on it to manually route their requests to the correct range/store. There are no mixed-version compatibility concerns here. Production code paths, with one exception, have long since guaranteed that batch request headers contain a range ID and store ID by the time they reach the Stores sender. The one exception is during bootstrapping of a fresh cluster, but there are no concerns about version compatibility because the requests are directed to the local store before any other nodes have joined the cluster. That code has been adjusted to use a wrapping sender that installs an appropriate range and store ID. Release note: None Co-authored-by: Nikhil Benesch <[email protected]>
Build succeeded |
Even if this looks good I'll wait until next week to merge it. Stability period and all.
Also apologies if I'm sending this to the wrong folks. Still learning who owns what on core.
Some test code was relying on an ancient and obscure feature of the
Stores sender where batch requests without a range and/or store ID could
be routed by the request key. Normally, this routing is handled by
DistSender. Remove this feature so that requests in tests follow the
same path as requests in production. Fix the few tests that were relying
on it to manually route their requests to the correct range/store.
There are no mixed-version compatibility concerns here. Production code
paths, with one exception, have long since guaranteed that batch request
headers contain a range ID and store ID by the time they reach the
Stores sender. The one exception is during bootstrapping of a fresh
cluster, but there are no concerns about version compatibility because
the requests are directed to the local store before any other nodes have
joined the cluster. That code has been adjusted to use a wrapping sender
that installs an appropriate range and store ID.
Release note: None