WIP: *: batching analyze requests - #69686
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[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
the
size/M
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❌ Patch coverage is Please upload reports for the commit af66b65 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #69686 +/- ##
================================================
- Coverage 76.3238% 73.9461% -2.3778%
================================================
Files 2041 2058 +17
Lines 559771 579047 +19276
================================================
+ Hits 427239 428183 +944
- Misses 131631 150504 +18873
+ Partials 901 360 -541
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 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
| resp := &coprocessor.Response{ | ||
| Data: []byte("merged-main-response-data"), | ||
| BatchResponses: []*coprocessor.StoreBatchTaskResponse{ | ||
| { | ||
| TaskId: mergedTask.taskID, | ||
| DataMergedIntoResponse: true, | ||
| ExecDetailsV2: &kvrpcpb.ExecDetailsV2{ | ||
| ScanDetailV2: &kvrpcpb.ScanDetailV2{ProcessedVersions: 7}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert on the collection of execution details.
The test mock correctly provides ExecDetailsV2 for the merged task, but there are no assertions verifying that these metrics were actually collected by the worker. Adding an assertion to verify worker.stats.ScanDetail (or the corresponding execution metrics container) would enforce correctness and would have caught the bug where the main response was being used for execution details instead of the batched response.
🤖 Prompt for 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.
In `@pkg/store/copr/coprocessor_ack_test.go` around lines 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.
6d5651b to
763e499
Compare
ti-chi-bot
Bot
added
size/L
ti-chi-bot
Bot
added
size/XL
Capture the two boundaries that later commits must change or preserve: analyze requests do not yet join store batching, while batched tasks without a per-task response remain eligible for retry. Signed-off-by: 0xPoe <poe.liu@pm.me>
Forward tidb_store_batch_size to V2 full-sampling Analyze and admit those internal requests to store batching without row-count hints. Leave KeepOrder unset because the consumers derive correlation and other statistics from the collected samples rather than scan order. Signed-off-by: 0xPoe <poe.liu@pm.me>
Opt Analyze into kvproto batch-task data merging and consume explicit per-task acknowledgements without emitting empty responses. A task without an acknowledgement remains on the existing retry path, so mixed-version stores cannot cause silent statistics loss. The acknowledgement path keeps each task's execution details as unconsumed stats through a nil-safe runtime-stats append, extracted and shared with the existing unconsumed-stats collection. Temporarily replace kvproto with the revision from pingcap/kvproto#1497 until that protocol change merges. Signed-off-by: 0xPoe <poe.liu@pm.me>
ti-chi-bot
Bot
added
size/L
|
@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. |
1 participant
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?
tidb_store_batch_sizeto V2 full-samplingANALYZEand allow batching without row-count hints; executor: remove unnecessary ordering from full-sampling Analyze requests #70275 removes the obsoleteKeepOrderrequirement.allow_batch_task_data_mergeand explicitdata_merged_into_responseacknowledgements.Depends on #70275, pingcap/kvproto#1497, and the TiKV branch
0xPoe/tikv:batch-cleanup-no-hll.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.