release-20.1: kv: batch ranged intent resolution #47032
Merged
Conversation
This file contains 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
Before this change, `var br *roachpb.BatchResponse` was being forced onto the heap: ``` $ goescape . | grep moved ./batcher.go:262:6: moved to heap: br ``` By moving the closures into the RunWorker function, we avoid the allocation: ``` $ goescape . | grep moved | wc -l 0 ``` Release justification: probably none. I'll sit on this if it's alone.
Previously, the only way to access this functionality outside of the roachpb package was through the BatchResponse.Combine method. There doesn't seem to be a strong reason for this encapsulation, given that Response is an exported part of the roachpb interface.
This has long-since been replaced by `t.Helper()`.
This uses of this have long-since been replaceable by `t.Helper()`.
…uests This commit improves the contract around MaxSpanRequestKeys and its interaction with overlapping and unsorted requests. Instead of saying that only sorted, non-overlapping requests were allowed in a batch with a limit (which was not being respected by users of the KV API), we now discuss what callers should expect if they cannot provide one or both of these guarantees. The commit then improves the testing around this area.
Fixes cockroachdb#46752. Resolves the recent perf regression on TPC-C. This commit follows in the footsteps of cockroachdb#34803 and introduces batching for ranged intent resolution, where previously only point intent resolution was batched. As we found in cockroachdb#46752, this is more important than it has been in the past, because implicit SELECT FOR UPDATE acquires a ranged lock on each row that it updates instead of a single-key lock. The change addresses this by adding a third request batcher to IntentResolver. ResolveIntent requests and ResolveIntentRange requests are batched separately, which is necessary for the use of MaxSpanRequestKeys to work properly (in fact, to be accepted by DistSender at all). To accommodate the ranged nature of ResolveIntentRange request, the notion of pagination is introduced into RequestBatcher. A MaxKeysPerBatchReq option is added to the configuration of a RequestBatcher, which corresponds to the MaxSpanRequestKeys value set on each BatchRequest header. The RequestBatcher is then taught about request pagination and how to work with partial responses. See the previous commit for clarification about the semantics at play here. Release justification: important fix to avoid a performance regression when implicit SELECT FOR UPDATE is enabled.
|
This change is |
|
LGTM
…On Sat, Apr 4, 2020 at 11:47 AM cockroach-teamcity ***@***.***> wrote:
This change is [image: Reviewable]
<https://reviewable.io/reviews/cockroachdb/cockroach/47032>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#47032 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC4C4LOM54UXAUBBXFI63TRK5JCFANCNFSM4L6RFBFA>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backport 6/6 commits from #46952.
/cc @cockroachdb/release
Fixes #46752.
Resolves the recent perf regression on TPC-C.
This commit follows in the footsteps of #34803 and introduces batching for ranged intent resolution, where previously only point intent resolution was batched. As we found in #46752, this is more important than it has been in the past, because implicit SELECT FOR UPDATE acquires a ranged lock on each row that it updates instead of a single-key lock.
The change addresses this by adding a third request batcher to IntentResolver. ResolveIntent requests and ResolveIntentRange requests are batched separately, which is necessary for the use of MaxSpanRequestKeys to work properly (in fact, to be accepted by DistSender at all).
To accommodate the ranged nature of ResolveIntentRange request, the notion of pagination is introduced into RequestBatcher. A MaxKeysPerBatchReq option is added to the configuration of a RequestBatcher, which corresponds to the MaxSpanRequestKeys value set on each BatchRequest header. The RequestBatcher is then taught about request pagination and how to work with partial responses. The semantics at play here are clarified in an earlier commit in the PR.
Release justification: important fix to avoid a performance regression when implicit SELECT FOR UPDATE is enabled. All commits except the last are testing-only. The last commit is subtle but small and well-tested.
@andreimatei: I assigned you because I think you know the most about
MaxSpanRequestKeys. I'm mostly interested to get your input on the "rationalize Header.MaxSpanRequestKeys" commit (testing + comments only).