WIP: *: batching analyze requests - #69686
Open
0xPoe wants to merge 9 commits into
Open
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR enables store-batched coprocessor execution for Analyze requests by wiring StoreBatchSize into request construction, updating task and response handling, adjusting kvproto replacements, and adding batching and acknowledgement tests. ChangesAnalyze Request Store Batching
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AnalyzeColumnsExec
participant RequestBuilder
participant Coprocessor
participant TiKV
AnalyzeColumnsExec->>RequestBuilder: SetStoreBatchSize(SessionVars.StoreBatchSize)
RequestBuilder->>Coprocessor: Build Analyze request
Coprocessor->>Coprocessor: Group Analyze tasks for store batching
Coprocessor->>TiKV: Send batched Analyze request
TiKV-->>Coprocessor: Return merged acknowledgement or no task response
Coprocessor-->>AnalyzeColumnsExec: Complete merged tasks or redispatch unanswered tasks
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #69686 +/- ##
================================================
- Coverage 76.3256% 76.3254% -0.0003%
================================================
Files 2041 2041
Lines 558170 558161 -9
================================================
- Hits 426027 426019 -8
+ Misses 131243 131242 -1
Partials 900 900
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/store/copr/coprocessor_ack_test.go`:
- Around line 94-103: Extend the test around the merged task response to assert
that the worker’s collected execution metrics include the batched response’s
ScanDetail values, specifically ProcessedVersions of 7. Use the existing
worker.stats.ScanDetail or corresponding metrics container, ensuring the
assertion verifies metrics come from the batch ExecDetailsV2 rather than the
main response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 659b7034-dd42-46e1-9c6b-fd140d8150e3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
DEPS.bzlgo.modpkg/executor/analyze_col.gopkg/store/copr/coprocessor.gopkg/store/copr/coprocessor_ack_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- pkg/executor/analyze_col.go
- DEPS.bzl
0xPoe
force-pushed
the
poe-batch-analyze
branch
2 times, most recently
from
July 23, 2026 09:10
6d5651b to
763e499
Compare
ti-chi-bot
Bot
added
size/L
13 tasks
ti-chi-bot
Bot
added
size/XL
ti-chi-bot
Bot
added
size/L
This was referenced Jul 24, 2026
13 tasks
0xPoe
force-pushed
the
poe-batch-analyze
branch
from
August 3, 2026 10:05
af66b65 to
0a16f89
Compare
ti-chi-bot
Bot
added
the
size/XL
0xPoe
force-pushed
the
poe-batch-analyze
branch
10 times, most recently
from
August 13, 2026 13:19
aa750cf to
47435ea
Compare
ti-chi-bot
Bot
removed
the
size/XL
|
[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 |
ti-chi-bot
Bot
added
size/XXL
0xPoe
force-pushed
the
poe-batch-analyze
branch
4 times, most recently
from
August 17, 2026 09:58
30c611a to
72cf2a8
Compare
handleBatchCopResponse passed the parent response's lock to handleLockErr, but that field is always nil in the batched-response loop, so a child's lock was never resolved and the task retried without backoff. Pass the child's lock instead. Signed-off-by: 0xPoe <[email protected]>
Record that an internal Analyze request without row-count hints stays on the flat builder until its caller opts into merged child responses. This gives the later batching change a clear before-and-after test. Signed-off-by: 0xPoe <[email protected]>
Record that a batched child without a task response is retried and counted as a fallback. This is the compatibility baseline for stores that do not return merge acknowledgements. Signed-off-by: 0xPoe <[email protected]>
Route every append of unconsumed coprocessor runtime stats through one nil-safe helper that holds the stats lock. This lets merged child responses retain execution details without duplicating synchronization. Signed-off-by: 0xPoe <[email protected]>
Use the temporary 0xPoe/kvproto revision that adds the allow_batch_task_data_merge request capability, the per-task data_merged_into_response acknowledgement, and the execute_batch_tasks_serially execution control. Return to pingcap/kvproto after pingcap/kvproto#1497 merges. Signed-off-by: 0xPoe <[email protected]>
Add an explicit opt-in for store batching when a request is internal, non-DAG, or lacks row-count hints. Callers must bound response size and accept both merged and per-task response shapes. Send the capability to TiKV. Treat merge acknowledgements as completed children, retain each child's execution details, and retry children with no response or acknowledgement for mixed-version compatibility. Signed-off-by: 0xPoe <[email protected]>
Record that client-go's empty synthetic EpochNotMatch flattens a store batch after Region cache invalidation. The next commit changes this retry shape while preserving the same ranges. Signed-off-by: 0xPoe <[email protected]>
Recognize client-go's empty synthetic EpochNotMatch only when no RPC context or task result exists. In that case, relocate and regroup the whole batch without changing the iterator's shared request. Keep RPC-backed and partial responses on the existing flat reconciliation path. Signed-off-by: 0xPoe <[email protected]>
Add request-builder controls for same-store batching, merged child responses, and serial child-task execution, then use them for full-sampling column Analyze without changing its scan concurrency. Introduce tidb_analyze_store_batch_size as an independent global and session variable. Default it to four, let zero disable batching, and refresh the global value before Auto Analyze runs on a pooled internal session. Signed-off-by: 0xPoe <[email protected]>
|
@0xPoe: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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
1 participant
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.
What problem does this PR solve?
Issue Number: ref #67449
Problem Summary:
Full-sampling
ANALYZEcurrently sends and receives one coprocessor result per region, causing avoidable RPC and response-merging overhead.What changed and how does it work?
ANALYZEto usetidb_store_batch_sizewithout row-count hints once large-table NDV sampling explicitly opts in, while preserving Analyze scan concurrency; executor: remove unnecessary ordering from full-sampling Analyze requests #70275 removed the obsoleteKeepOrderrequirement.allow_batch_task_data_mergeand explicitdata_merged_into_responseacknowledgements.Depends on #70275, pingcap/kvproto#1497, and tikv/tikv#19854.
Check List
Tests
ANALYZEthrough the real endpoint)Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.