kv: don't perform async intent resolution on 1PC with point lock spans#98630
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Mar 15, 2023
Merged
Conversation
Member
|
This change is |
arulajmani
approved these changes
Mar 14, 2023
Collaborator
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
-- commits line 19 at r1:
Nice!
pkg/kv/kvserver/replica_write.go line 592 at r1 (raw file):
var externalLocks []roachpb.Span for _, sp := range etArg.LockSpans { if len(sp.EndKey) == 0 {
nit: let's add a comment explaining why we're special case handling point locks?
pkg/kv/kvserver/replica_test.go line 6978 at r1 (raw file):
for _, tc := range []struct { intent roachpb.Span
Should we rename this to "span" and generalize some of the intent commentary below as well?
Fixes cockroachdb#98571. This commit fixes the regression detected in cockroachdb#98571. In that issue, we saw that the bug fix in 86a5852 (cockroachdb#98044) caused a regression in kv0 (and other benchmarks). This was due to a bug in `kvserverbase.IntersectSpan`, which was considering local point spans to be external to the provided range span. This commit fixes the bug by not calling `kvserverbase.IntersectSpan` for point lock spans. The commit also makes the utility panic instead of silently returning incorrect results. There's an existing TODO on the utility to generalize it. For now, we just make it harder to misuse. Finally, we add a test that asserts against the presence of async intent resolution after one-phase commits when external intent resolution is not needed. ``` name old time/op new time/op delta KV/Insert/Native/rows=1-10 61.2µs ± 3% 48.9µs ± 3% -20.10% (p=0.000 n=8+9) KV/Insert/Native/rows=10-10 93.3µs ±15% 76.2µs ± 3% -18.34% (p=0.000 n=9+9) KV/Insert/Native/rows=1000-10 2.84ms ±12% 2.42ms ± 4% -14.97% (p=0.000 n=9+9) KV/Insert/Native/rows=100-10 365µs ± 5% 320µs ± 8% -12.40% (p=0.000 n=10+9) KV/Insert/Native/rows=10000-10 27.6ms ± 6% 24.4ms ± 3% -11.53% (p=0.000 n=9+9) name old alloc/op new alloc/op delta KV/Insert/Native/rows=1000-10 4.66MB ± 1% 2.76MB ± 1% -40.89% (p=0.000 n=9+9) KV/Insert/Native/rows=100-10 478kB ± 1% 287kB ± 1% -39.90% (p=0.000 n=10+10) KV/Insert/Native/rows=10000-10 54.2MB ± 2% 34.3MB ± 3% -36.73% (p=0.000 n=10+10) KV/Insert/Native/rows=10-10 64.2kB ± 1% 42.1kB ± 1% -34.39% (p=0.000 n=10+9) KV/Insert/Native/rows=1-10 22.1kB ± 1% 17.3kB ± 1% -21.56% (p=0.000 n=9+10) name old allocs/op new allocs/op delta KV/Insert/Native/rows=1000-10 21.5k ± 0% 14.7k ± 0% -31.70% (p=0.000 n=8+9) KV/Insert/Native/rows=10000-10 212k ± 0% 146k ± 0% -31.31% (p=0.000 n=9+10) KV/Insert/Native/rows=100-10 2.34k ± 1% 1.61k ± 0% -31.31% (p=0.000 n=10+10) KV/Insert/Native/rows=10-10 392 ± 1% 276 ± 0% -29.59% (p=0.000 n=8+8) KV/Insert/Native/rows=1-10 173 ± 1% 123 ± 0% -29.04% (p=0.000 n=9+8) ``` Release note: None
d70ec06 to
3300ba2
Compare
nvb
commented
Mar 15, 2023
Contributor
Author
nvb
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/kv/kvserver/replica_write.go line 592 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: let's add a comment explaining why we're special case handling point locks?
Done.
pkg/kv/kvserver/replica_test.go line 6978 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should we rename this to "span" and generalize some of the intent commentary below as well?
Good idea. Done.
Contributor
Author
|
bors r=arulajmani |
Contributor
|
Build succeeded: |
18 tasks
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
3 participants
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.
Fixes #98571.
This commit fixes the regression detected in #98571. In that issue, we saw that the bug fix in 86a5852 (#98044) caused a regression in kv0 (and other benchmarks). This was due to a bug in
kvserverbase.IntersectSpan, which was considering local point spans to be external to the provided range span.This commit fixes the bug by not calling
kvserverbase.IntersectSpanfor point lock spans.The commit also makes the utility panic instead of silently returning incorrect results. There's an existing TODO on the utility to generalize it. For now, we just make it harder to misuse.
Finally, we add a test that asserts against the presence of async intent resolution after one-phase commits when external intent resolution is not needed.
Release note: None