Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kv: ensure secondary tenants route follower reads to the closest replica #85853

Merged
merged 5 commits into from
Aug 14, 2022
Merged

kv: ensure secondary tenants route follower reads to the closest replica #85853

merged 5 commits into from
Aug 14, 2022

Commits on Aug 14, 2022

  1. server: add cluster setting to control tenant trace redaction 

    Previously, all operations run on the server on behalf of a secondary
tenant would have their logs redacted. This patch introduces a new
cluster setting that controls whether logs should be redacted or
not. This is only settable by the system tenant and maintains the
previous behaviour by default.

Release note (ops change): introduce a new cluster setting
(`server.secondary_tenants.redact_trace`) which controls if traces
should be redacted for ops run on behalf of secondary tenants.
    @arulajmani
    arulajmani committed Aug 14, 2022
    Configuration menu
    Copy the full SHA
    8ebdf27 View commit details
    Browse the repository at this point in the history

  2. kvfollowerreadsccl: skip tests that break when run as secondary tenants 

    We probabilistically run SQL using secondary tenant connections
in tests. This breaks TestBoundedStalenessDataDriven and
TestFollowerReadsWithStaleDescriptor.

Both these tests configure SQL testing knobs to trace statements and
then make assertions on the collected traces. As these testing knobs
aren't shared by the secondary tenant `TestServer` starts the test
simply doesn't work. For now, we simply eschew running these tests with
secondary tenants.

Release note: None
    @arulajmani
    arulajmani committed Aug 14, 2022
    Configuration menu
    Copy the full SHA
    a7d5abf View commit details
    Browse the repository at this point in the history

  3. kv: ensure secondary tenants route follower reads to the closest replica 

    The dist sender uses node locality information to rank replicas of a
range by latency. Previously, this node locality information was read
off a node descriptor available in Gossip. Unfortunately, secondary
tenants do not have access to Gossip, and as such, would end up
randomizing this list of replicas. This manifested itself through
unpredictable latencies when running follower reads.

We're no longer susceptible to this hazard with this patch. This is done
by eschewing the need of a node descriptor from gossip in the
DistSender; instead, we now instantiate the DistSender with locality
information.

However, we do still use Gossip to get the current node's
ID when ranking replicas. This is done to ascertain if there is a local
replica, and if there is, to always route to it. Unfortunately, because
secondary tenants don't have access to Gossip, they can't conform to
these semantics. They're susceptible to a hazard where a request may
be routed to another replica in the same locality tier as the client
even though the client has a local replica as well. This shouldn't be
a concern in practice given the diversity heuristic. It also shouldn't
be a concern given tenant SQL pods don't run in process with KV nodes
today.

Resolves cockroachdb#81000

Release note (bug fix): fix an issue where secondary tenants could
route follower reads to a random, far away replica instead of one
closer.
    @arulajmani
    arulajmani committed Aug 14, 2022
    Configuration menu
    Copy the full SHA
    3d87dde View commit details
    Browse the repository at this point in the history

  4. kvcoord: remove ds.getNodeDescriptor 

    This function is no longer used by the DistSender to optimize replica
order.

Release note: None
    @arulajmani
    arulajmani committed Aug 14, 2022
    Configuration menu
    Copy the full SHA
    befd340 View commit details
    Browse the repository at this point in the history

  5. sql: use locality information when initializing span resolver oracles 

    Previously, we used a node descriptor for locality information about
the current node when doing dist SQL planning. This only worked for
the system tenant as secondary tenant's used a fake node descriptor.
Given the changes to `OptimizeReplicaOrder` in the prior commit, this
patch switches to threading in the tenant pod's locality to allow
secondary tenant's to make use of this information in dist sql
planning.

Release note: None
    @arulajmani
    arulajmani committed Aug 14, 2022
    Configuration menu
    Copy the full SHA
    0e08303 View commit details
    Browse the repository at this point in the history