kvserver: scan only intents during rts scan #71295
Conversation
|
This change is |
There was a problem hiding this comment.
Exciting! Just left some drive-by comments. I'd be interested to hear what kind of impact you are seeing this have on changefeed restart latency.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)
pkg/kv/kvserver/replica_rangefeed.go, line 380 at r1 (raw file):
p = rangefeed.NewProcessor(cfg) onlySeperatedIntents := r.store.cfg.Settings.Version.ActiveVersionOrEmpty(ctx).IsActive(clusterversion.PostSeparatedIntentsMigration)
I don't think you need the ActiveVersionOrEmpty(ctx) part.
pkg/kv/kvserver/replica_rangefeed.go, line 385 at r1 (raw file):
if onlySeperatedIntents { rtsIter = func() rangefeed.IntentScanner { lowerBound := keys.LockTableSingleKeyStart
If we're providing a lower bound (which itself shouldn't be necessary), shouldn't it be keys.LockTableSingleKey(desc.Key.AsRawKey(), nil)?
pkg/kv/kvserver/rangefeed/task.go, line 104 at r1 (raw file):
ltStart, _ := keys.LockTableSingleKey(startKey, nil) var meta enginepb.MVCCMetadata for valid, err := s.iter.SeekEngineKeyGE(storage.EngineKey{Key: ltStart}); ; valid, err = s.iter.NextEngineKey() {
This is relying on the iterator's upper bound to terminate the iteration. That's not necessarily a problem, but we should make that clear both here and in the SeperatedIntentScanner contract.
pkg/kv/kvserver/rangefeed/task.go, line 113 at r1 (raw file):
engineKey, err := s.iter.EngineKey() if err != nil { // TODO(ssd): should this return or continue?
I assume you're asking the question because of the corresponding logic in computeMinIntentTimestamp. That logic looks wrong. We were continuing on all error cases at one point during the review process, and we switched it over, but must have missed that case. Do you mind fixing that over there as well?
pkg/kv/kvserver/rangefeed/task.go, line 125 at r1 (raw file):
} if meta.Txn != nil {
meta.Txn == nil is an error case, so we should treat it as such.
stevendanna
force-pushed
the
rangefeed-rts-scan
branch
2 times, most recently
from
8867cd1
to
c6789af
Compare
There was a problem hiding this comment.
I'd be interested to hear what kind of impact you are seeing this have on changefeed restart latency.
Me too! Haven't been able to test this well yet.
Reviewable status:
pkg/kv/kvserver/replica_rangefeed.go, line 380 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I don't think you need the
ActiveVersionOrEmpty(ctx)part.
Done. Thanks!
pkg/kv/kvserver/replica_rangefeed.go, line 385 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
If we're providing a lower bound (which itself shouldn't be necessary), shouldn't it be
keys.LockTableSingleKey(desc.Key.AsRawKey(), nil)?
Done. Thanks!
pkg/kv/kvserver/rangefeed/task.go, line 113 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I assume you're asking the question because of the corresponding logic in
computeMinIntentTimestamp. That logic looks wrong. We were continuing on all error cases at one point during the review process, and we switched it over, but must have missed that case. Do you mind fixing that over there as well?
Done -- your are exactly right about why I was asking, thanks for confirming!
pkg/kv/kvserver/rangefeed/task.go, line 125 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
meta.Txn == nilis an error case, so we should treat it as such.
Done.
stevendanna
changed the title
stevendanna
force-pushed
the
rangefeed-rts-scan
branch
from
c6789af
to
5142f49
Compare
|
I'd still like to write a benchmark here, but I think the general shape of this should be ready for review. |
|
Drive-by comment: we may want to use storage APIs to access the separated intents rather than directly using the engine and keys all over the place. I wrote a small utility function for this a while ago: cockroach/pkg/storage/engine.go Line 987 in db0c631 Although it'd be even nicer if we had an intent iterator or something in there. Not sure if we have something like that laying around already, otherwise it might be easy to whip one up (i.e. adapt what you have here). Don't let this block the PR though, just a thought. |
|
I definitely like the idea of an intent iterator. I didn't reach for the
utility function just because I wasn't sure if accumulating the intents
would be OK. But, I imagine it's probably fine.
…On Fri, 8 Oct 2021, 19:32 Erik Grinaker, ***@***.***> wrote:
Drive-by comment: we may want to use storage APIs to access the separated
intents rather than directly using the engine and keys all over the place.
I wrote a small utility function for this a while ago:
https://github.com/cockroachdb/cockroach/blob/db0c631a6ed37917d57a67693fbe407ace87cf36/pkg/storage/engine.go#L987
Although it'd be even nicer if we had an intent iterator or something in
there. Not sure if we have something like that laying around already,
otherwise it might be easy to whip one up.
Don't let this block the PR though, just a thought.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#71295 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGQDE532D2BWAOKHK3UW4LUF42L3ANCNFSM5FSAKIEQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I agree, we should use an iterator here. Feel free to punt it for now though. |
stevendanna
force-pushed
the
rangefeed-rts-scan
branch
from
5142f49
to
915e699
Compare
|
More to do here: |
stevendanna
force-pushed
the
rangefeed-rts-scan
branch
from
915e699
to
2e920ac
Compare
|
That error was a result of me accidentally misusing the scratch buffer provided by keys.LockSingleKey. Should be resolved now, let's see what CI says. |
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/rangefeed/task.go, line 90 at r6 (raw file):
// separated intents are in use. // // EngineIterator Contract:
Any way to enforce this contract?
|
@nvanbenschoten I've run some ad-hoc tests. While running the tpcc workload (w=100) against a 4 node local cluster, the impact on changefeed startup (without a backfill) is pretty substantial. You can see it pretty dramatically in p99 sql latency:
I would take the scale there with a grain of salt, but I've run this a few times in different orders and get the same shape of difference each time. |
|
This is awesome!
…On Thu, Oct 14, 2021, 7:00 AM Steven Danna ***@***.***> wrote:
@nvanbenschoten <https://github.com/nvanbenschoten> I've run some ad-hoc
tests. While running the tpcc workload (w=100) against a 4 node local
cluster, the impact on changefeed startup is pretty substantial. You can
see it pretty dramatically in p99 sql latency:
[image: Screenshot 2021-10-14 at 11 24 47]
<https://user-images.githubusercontent.com/852371/137304876-0e9a66ea-895c-4260-91e7-bd032ad0c3a0.png>
I would take the scale there with a grain of salt, but I've run this a few
times in different orders and get the same shape of difference each time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#71295 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA4FVCPD4EFPNDVBA33YL3UG2Z4RANCNFSM5FSAKIEQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
There was a problem hiding this comment.
Reviewed 3 of 5 files at r2, 1 of 1 files at r3, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status:
pkg/kv/kvserver/replica_rangefeed.go, line 380 at r6 (raw file):
p = rangefeed.NewProcessor(cfg) onlySeparatedIntents := r.store.cfg.Settings.Version.IsActive(ctx, clusterversion.PostSeparatedIntentsMigration)
nit: any reason not to push this into the rtsIter closure?
pkg/kv/kvserver/replica_rangefeed.go, line 402 at r6 (raw file):
iter := r.Engine().NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{ UpperBound: desc.EndKey.AsRawKey(), // TODO(nvanbenschoten): To facilitate fast restarts of rangefeed
I think we can get rid of this TODO now 🥳
pkg/kv/kvserver/rangefeed/task.go, line 43 at r6 (raw file):
type initResolvedTSScan struct { p *Processor intentConsumer IntentScanner
nit: the naming here is a little off. We have a type named IntentScanner and we call it c in some contexts and intentConsumer in others. And we also have a type named eventConsumer. Can I suggest using is in all contexts?
pkg/kv/kvserver/rangefeed/task.go, line 114 at r6 (raw file):
return err } else if !valid { // We assume the iterator has an UpperBound
s/assume the iterator has/depend on the iterator having/
In 21.2, seperated intents are the default. Once migrated, we can then use this to iterate over substantially less data to find all intents for a given keyspan. The hope is that this will make rangefeed start-up substantially cheaper. Release note: None
stevendanna
force-pushed
the
rangefeed-rts-scan
branch
from
2e920ac
to
134de82
Compare
|
bors r=nvanbenschoten |
|
Build succeeded: |
This setting was added in a 21.2 backport cockroachdb#72315 but was never added to master: cockroachdb#71295 So from the perspective of someone who upgrades from 21.2, it is now a retired setting. Release justification: Low-risk bug fix Release note: None
78186: ui: improve UI for statuses on the jobs page r=jocrl a=jocrl Addresses #71963 Previously the `cancel-requested`, `pause-requested`, and `revert-failed` had blue badges.  Now, `cancel-requested` and `pause-requested` have gray badges and `revert-failed` has a red badge.  Release note (ui): Improved colors for status badges on the Jobs page. Three status on the Jobs page, `cancel-requested`, `pause-requested`, and `revert-failed`, previously had blue status badge colors that were uninformative of their meaning. This commit modifies the badge colors to reflect their meaning. Now `cancel-requested` and `pause-requested` have gray badges and `revert-failed` has a red badge. 78213: distsql: simple projection in experimental distsql planner panics r=msirek a=msirek Previously, selecting a given column from a table more than once could cause an `index out of range` panic when experimental_distsql_planning is set to always, for example: ``` CREATE TABLE kv (k INT PRIMARY KEY, v INT); INSERT INTO kv VALUES (1, 1), (2, 1), (3, 2); SET experimental_distsql_planning = always; SELECT v, k, k, v FROM kv; ``` This commit fixes the issue, which is due to incorrect mapping of selected columns to source column ordinals in `ConstructSimpleProject`. Release note: none 78243: backupccl: fix paper cut in latest files directory structure r=DarrylWong a=adityamaru Prior to this change, the LATEST files of a backup were written to `metadatalatest/` instead of `metadata/latest/`, this patch fixes that. Release note (bug fix): The LATEST file that points to the latest full backup in a collection was written to a directory path with the wrong structure. 78246: settings: retire kv.rangefeed.separated_intent_scan.enabled r=nvanbenschoten a=stevendanna This setting was added in a 21.2 backport #72315 but was never added to master: #71295 So from the perspective of someone who upgrades from 21.2, it is now a retired setting. Release justification: Low-risk bug fix Release note: None Co-authored-by: Josephine Lee <josephine@cockroachlabs.com> Co-authored-by: Mark Sirek <sirek@cockroachlabs.com> Co-authored-by: Aditya Maru <adityamaru@gmail.com> Co-authored-by: Steven Danna <danna@cockroachlabs.com>
In 21.2, seperated intents are the default. Once migrated, we can then
use this to iterate over substantially less data to find all intents
for a given keyspan. The hope is that this will make rangefeed
start-up substantially cheaper.
Informs #70920
Fixes #69697
Release note: None