executor, store/copr: batch full-sampling Analyze requests - #70055
executor, store/copr: batch full-sampling Analyze requests#700550xTars wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @0xTars. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Murphy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
0f49e5e to
d19e055
Compare
e9cc26c to
f86b451
Compare
3181cc0 to
b57f31a
Compare
Pin the coordinated kvproto and client-go revisions together so the behavior commit sees both the explicit merged-task acknowledgment field and nested child-accounting support. These contributor revisions are validation-only. Before this draft can land, a live upstream protocol successor must replace the historical closed kvproto patch, the canonical TiKV implementation must be chosen, and all consumers must repin to landed upstream revisions. Signed-off-by: Murphy <poe1024.liu@gmail.com>
b57f31a to
dbdf961
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Let full-sampling Analyze requests opt in to store-batched coprocessor
tasks, with the batch width derived from the request's own region count
instead of the DAG store-batch session variable.
Width selection: with C the resolved Analyze scan budget (concurrency)
and N the request-wide deduplicated region count, N <= 2*C keeps the
legacy one-task-per-region shape (no batching); otherwise the total
width is B = min(ceil(N/C), max{b : b*(b+1) <= C}), so a batched width
always leaves room for at least B+1 concurrent outer workers. N counts
distinct region IDs once across all KeyRanges partitions, while packing
runs per partition and never merges tasks across a partition boundary.
Every retry and rebuild path leaves the budget zero and thus rebuilds
plain singleton tasks. Analyze ignores tidb_store_batch_size entirely;
the generic DAG store-batch path and the TiKV protocol are unchanged
apart from the explicit merged-task acknowledgment capability flag.
Also fix two numeric issues surfaced while validating the shapes:
* handleBatchCopResponse counted every remaining retry task as a
fallback, so a region split during retry could underflow the
store-batched counter; count the original fallback exactly once.
* an unsigned-PK table split its Analyze scan into two requests at
the int64 boundary, making one table use two independent scan
budgets and region-count decisions. V2 full-sampling Analyze
derives handle order from samples, so keep all ranges in one
request under one budget.
Signed-off-by: Murphy <poe1024.liu@gmail.com>
dbdf961 to
43badf3
Compare
What problem does this PR solve?
Issue Number: ref #70059
Problem Summary:
V2 full-sampling
ANALYZEnormally sends one coprocessor request per TiKVRegion. Large physical tables can therefore create many small requests.
Same-store batching can reduce that RPC count, but the batching policy must use
the request's actual resolved Regions, preserve the configured Analyze
physical-scan budget, and keep retry and non-Analyze paths unchanged.
This replaces #69686 because its source branch cannot be updated. Issue #70059
tracks the cross-repository batching contract and landing gates.
What changed and how does it work?
Mark V2 full-sampling Analyze requests as able to consume merged child
responses. The existing generic DAG path is unchanged.
Resolve all
KeyRangespartitions for one Analyzekv.Requestexactly once,deduplicate their Region IDs, select one request-local width, and pack each
partition independently. Batches never cross stores or partition-builder
boundaries, and descending response order is preserved.
Let
Nbe the number of distinct Regions covered by one Analyzekv.Request(one nonpartitioned table or one current physical partition),and let
C=max(resolved Analyze scan concurrency,1). The selected maximumsame-store Region width is:
The implementation uses overflow-safe integer comparisons.
B=1meanssingleton requests. The triangular cap guarantees
floor(C/B) >= B+1for a batched width.Full-sampling Analyze does not use
tidb_store_batch_size; generic DAGbatching still does.
Derive iterator concurrency from actual built task weights
(
1 + child count) with a dynamic weight histogram. The admitted heaviestprefix never exceeds
C, so the scalar worker count is safe for everyscheduler order and for widths greater than five.
Apply the request-local policy only during the initial build. Region-error
and lock retries rebuild singleton tasks. Fallback counters count each
original failed child once even when one child splits into several retry
tasks.
Keep unsigned integer-PK full-sampling Analyze in one request across
MaxInt64. The sampler reconstructs order itself, so this avoids twoindependent scan budgets without changing histogram bounds or correlation.
The runtime evidence is deliberately limited:
N=385,C=64selectedB=7and emitted the expected 56 of 56 table-Regionrequests with zero retries.
N=449,C=64resource comparison emitted 66 requests instead of 449(
-85.3%). Its single paired wall-clock delta was+0.65%, which is not aperformance conclusion. Statistics and checksums were identical; both arms
stayed under the predefined resource gates and showed no backoff, OOM, swap,
message-size, or fatal signal.
N=257,B=5twelve-pair evidence was performance-neutral. TheN=449,B=7result is one pair only. This PR does not claim universal speedupor memory savings.
B=3andB=7, more than four stores, and a real-clusterN<=2Cgate remain unmeasured.The temporary dependency revisions are validation pins:
a29e794b680d(current compatible base plus the merged-childresponse contract)
949253a114a6(current compatible base plus nested child resourceaccounting)
This Draft cannot land until the corresponding upstream protocol and
client-go changes land and all consumers are repinned to mutually compatible
upstream revisions. These contributor refs are not a dependency exit.
Check List
Tests
./tools/check/failpoint-go-test.sh pkg/executor -run '^TestAnalyzeBuildsSingleBatchableRequest$' -count=1./tools/check/failpoint-go-test.sh pkg/store/copr -run '^(TestBuildTasksWithoutBuckets|TestAnalyzeStoreBatchPolicy|TestAnalyzeStoreBatchWidthUsesRequestRegionCount|TestAnalyzeStoreBatchWidthMatchesSmallIntegerReference|TestHandleBatchCopResponseMergedAndUnansweredTasks|TestHandleBatchCopResponseUpdatesChildBucketsOnVersionNotMatch)$' -count=1./tools/check/failpoint-go-test.sh pkg/store/copr/copr_test -run '^(TestAnalyzeBatchUsesRequestWideDistinctRegionCount|TestBuildCopIteratorWithBatchStoreCopr|TestBuildCopIteratorWithRunawayChecker)$' -count=1make fmtmake lintgo mod tidy(zero diff)go mod verifymake bazel_prepare(two consecutive zero-diff runs)git diff --check 93f713cf551f..HEADlisted above; statistics and checksums matched.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.