kv: Don't evict from leaseholder cache on context cancellations #30163
Conversation
|
This change is |
|
It may be a fluke, but this also has me hitting higher tpmC efficiencies than before (98.7 vs 94.6 on a 5 minute run; 99.7% on a 15 minute run). |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/kv/dist_sender.go, line 1338 at r2 (raw file):
// account that the local node can't be down) it won't take long until we // talk to a replica that tells us who the leaseholder is. if ctx.Err() == nil {
nit: is this preferable to if err != context.Canceled && err != context.DeadlineExceeded {?
|
Looks like this was actually somewhat new #23885. |
There was a problem hiding this comment.
Reviewable status:
pkg/kv/dist_sender.go, line 1338 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: is this preferable to
if err != context.Canceled && err != context.DeadlineExceeded {?
An argument could be made either way. I like this way because with all the wrapping of errors that we do throughout our code base I don't necessarily trust SendNext to have returned the context's error directly. Also this matches the check that we do for ctx.Err() below to determine whether to mark the error as a SendError.
If you prefer the other way for similarly compelling reasons, I can change it.
There was a problem hiding this comment.
Reviewable status:
pkg/kv/dist_sender.go, line 1338 at r2 (raw file):
Previously, a-robinson (Alex Robinson) wrote…
An argument could be made either way. I like this way because with all the wrapping of errors that we do throughout our code base I don't necessarily trust
SendNextto have returned the context's error directly. Also this matches the check that we do forctx.Err()below to determine whether to mark the error as aSendError.If you prefer the other way for similarly compelling reasons, I can change it.
The wrapping argument is valid. Let's keep this as is.
This avoids a bunch of unnecessary latency lookups and sorting in the common case. Release note: None
This was a major contributor to the hundreds of NotLeaseHolderErrors per second that we see whenever we run tpc-c at scale. A non-essential batch request like a QueryTxn would get cancelled, causing the range to be evicted from the leaseholder cache and the next request to that range to have to guess at the leaseholder. Release note: None
a-robinson
force-pushed
the
notleaseholder
branch
from
5437d1c to
397409c
Compare
|
bors r+ |
29692: ui: various glue fixes r=vilterp,couchand a=benesch This PR restores the "no UI installed" message in short binaries:  I came across a few minor nits that seemed worth fixing too. 30135: sql: add '2.0' setting for distsql r=jordanlewis a=jordanlewis The 2.0 setting for distsql (both a cluster setting and a session setting) instructs the executor to use the 2.0 method of determining how to execute a query: the query runs via local sql unless the query is both distributable and recommended to be distributed, in which case it runs via the distsql and is actually distributed. Release note (sql change): add the '2.0' value for both the distsql session setting and the sql.defaults.distsql cluster setting, which instructs the database to use the 2.0 'auto' behavior for determining whether queries run via distsql or not. 30148: storage: add new metrics for the RaftEntryCache r=nvanbenschoten a=nvanbenschoten Four new metrics are introduced: - `raft.entrycache.bytes` - `raft.entrycache.size` - `raft.entrycache.accesses` - `raft.entrycache.hits` 30163: kv: Don't evict from leaseholder cache on context cancellations r=a-robinson a=a-robinson This was a major contributor to the hundreds of NotLeaseHolderErrors per second that we see whenever we run tpc-c at scale. A non-essential batch request like a QueryTxn would get cancelled, causing the range to be evicted from the leaseholder cache and the next request to that range to have to guess at the leaseholder. This is effectively an extension of #26764 that we should have thought to inspect more closely at the time. Actually fixes #23543, which was not fully fixed before. Although I still haven't seen the errors drop all the way to 0, so I'm letting tpc-c 10k continue to run for a while longer to verify that they do. They are continuing to decrease about 15 minutes in. I don't think getting to 0 will be possible because there are still occasional splits and lease transfers), but it looks like it should be able to get down to single digit errors per second from the hundreds it was at before this change. Also, avoid doing unnecessary sorting by latency of replicas in the dist_sender in the common case when we know who the leaseholder is and plan on sending our request there. 30197: sql/parser: fix the action for empty rules r=knz a=knz Fixes #30141. Co-authored-by: Nikhil Benesch <[email protected]> Co-authored-by: Jordan Lewis <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Alex Robinson <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
This was a major contributor to the hundreds of NotLeaseHolderErrors per
second that we see whenever we run tpc-c at scale. A non-essential batch
request like a QueryTxn would get cancelled, causing the range to be
evicted from the leaseholder cache and the next request to that range to
have to guess at the leaseholder.
This is effectively an extension of #26764 that we should have thought to inspect more closely at the time.
Actually fixes #23543, which was not fully fixed before. Although I still haven't seen the errors drop all the way to 0, so I'm letting tpc-c 10k continue to run for a while longer to verify that they do. They are continuing to decrease about 15 minutes in. I don't think getting to 0 will be possible because there are still occasional splits and lease transfers), but it looks like it should be able to get down to single digit errors per second from the hundreds it was at before this change.
Also, avoid doing unnecessary sorting by latency of replicas in the dist_sender in the common case when we know who the leaseholder is and plan on sending our request there.