executor, store/copr: batch full-sampling Analyze requests#70055
executor, store/copr: batch full-sampling Analyze requests#700550xTars wants to merge 3 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. |
|
|
0f49e5e to
d19e055
Compare
Pin the coordinated kvproto and client-go revisions together so every downstream commit has both the explicit merged-task acknowledgement and nested child-accounting support. Use the replacement PR forks until their upstream changes are available from stable module revisions. Keeping both pins in one commit avoids transient dependency states that expose only half of the cross-repo contract. Signed-off-by: 0xTars <1196089730@qq.com>
Capture the existing boundaries before enabling the feature: Analyze requests do not join store batching yet, and tasks without a per-task response remain eligible for retry. Keep the new unanswered-response scenario and existing bucket-version scenario as sequential helper calls under one top-level test. This preserves the package limit of 50 tests without adding subtests to a sequential regression suite. Signed-off-by: 0xTars <1196089730@qq.com>
Enable store batching only for V2 full-sampling Analyze requests. Derive the outer RPC concurrency and child count from the resolved Analyze scan budget in the same commit, so batching cannot multiply either manual or system Analyze concurrency. Carry an explicit merge-capability marker from AnalyzeColumnsExec through task construction and the outgoing protobuf request. Keep other Analyze callers on the non-batched path, and cover the opt-in boundary and actual wire negotiation. Allow TiKV to merge acknowledged child results into the main response. Consume those acknowledgements without emitting empty responses, retain their execution details for runtime and runaway accounting, and keep unanswered tasks on the existing retry path. Use a maximum physical width of five scans, leave KeepOrder disabled for the sample-based consumer, and bypass the row-count-hint gate only after explicit opt-in. Extend the behavior tests to cover bounded batching and merged acknowledgements. Signed-off-by: 0xTars <1196089730@qq.com>
d19e055 to
e9cc26c
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 |
What problem does this PR solve?
Issue Number: ref #70059
Problem Summary:
Full-sampling Analyze sends many region coprocessor requests. Store batching can reduce RPC overhead, but the client and store need an explicit merged-result contract, complete per-task resource accounting, and a concurrency budget that does not multiply the existing Analyze DistSQL or system-process scan limit by the batch width.
This replaces #69686 because its source branch cannot be updated. Issue #70059 tracks this batching design; #67449 is related sample-based NDV work, not the design source for this PR.
What changed and how does it work?
StoreBatchTasks.C:width = min(C, 5), outer concurrencyC / width, andwidth - 1child tasks. Therefore total logical scan concurrency remains at mostCfor both manual Analyze and restricted/system Analyze.Related replacement chain:
Check List
Tests
./tools/check/failpoint-go-test.sh pkg/executor -run '^TestAnalyzeBatchScanBudget$' -count=1./tools/check/failpoint-go-test.sh pkg/store/copr -run '^TestHandleBatchCopResponse$' -count=1./tools/check/failpoint-go-test.sh pkg/store/copr/copr_test -run '^TestBuildCopIteratorWithBatchStoreCopr$' -count=1make lintmake bazel_prepare(zero generated diff)Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.