pkg/executor, pkg/kv, pkg/store: propagate SQL digest for MPP task metadata#66763
pkg/executor, pkg/kv, pkg/store: propagate SQL digest for MPP task metadata#66763JaySon-Huang wants to merge 4 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note
|
Review Complete Findings: 1 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/XXL
|
[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 |
|
Hi @JaySon-Huang. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
|
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:
📝 WalkthroughWalkthroughBumps vendored github.com/pingcap/kvproto (DEPS.bzl and go.mod) and threads SQLDigest/PlanDigest through the MPP pipeline: session → kv.MPPDispatchRequest → mpp.TaskMeta → dispatch/cancel/connection RPCs, with related logging and minor comment fixes; no other public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Session as "Session\n(SessionVars/StmtsCtx)"
participant Coordinator as "LocalMPPCoordinator"
participant KV as "KV Dispatcher\n(kv package)"
participant Store as "Store / Coprocessor\n(mpp package)"
Session->>Coordinator: Start MPP execution (includes SQLDigest, PlanDigest)
Coordinator->>KV: Build & send MPPDispatchRequest\n(include SQLDigest, PlanDigest)
KV->>Store: DispatchTask / EstablishConn / Cancel\n(TaskMeta/ReceiverMeta includes SqlDigest, PlanDigest)
Store->>Store: Execute task / log using digests
Store-->>KV: Task responses / errors (digests preserved)
KV-->>Coordinator: Dispatch response
Coordinator-->>Session: Report results / errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
![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
🧹 Nitpick comments (1)
DEPS.bzl (1)
6816-6826: Track upstream merge for kvproto fork.The kvproto dependency is temporarily pointing to a forked version (
JaySon-Huang/kvproto) to consume theTaskMeta.sql_digestfield before it's merged upstream. Ensure this is tracked (e.g., via a follow-up issue or the referenced#66762) so the dependency can be switched back github.com/pingcap/kvprotoonce the upstream kvproto PR is merged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEPS.bzl` around lines 6816 - 6826, The DEPS.bzl entry for name="com_github_pingcap_kvproto" currently points to the JaySon-Huang fork to consume TaskMeta.sql_digest; create a tracked follow-up (issue or link to `#66762`) and add a TODO comment in the same DEPS.bzl entry referencing that tracking issue/PR; when upstream kvproto is merged, update importpath back to "github.com/pingcap/kvproto", replace strip_prefix, sha256 and the urls to the official upstream release, and document the change in the follow-up so we can revert the forked dependency cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 378-379: The go.mod contains a temporary replace directive
pointing to a personal fork (github.com/JaySon-Huang/kvproto) which must be
removed before merge; once the upstream pingcap/kvproto contains the
TaskMeta.SqlDigest field, delete the replace line and update the module
dependency to the canonical github.com/pingcap/kvproto module (ensuring the
version includes TaskMeta.SqlDigest) so imports that reference
TaskMeta.SqlDigest resolve to the official upstream package rather than the
fork.
---
Nitpick comments:
In `@DEPS.bzl`:
- Around line 6816-6826: The DEPS.bzl entry for
name="com_github_pingcap_kvproto" currently points to the JaySon-Huang fork to
consume TaskMeta.sql_digest; create a tracked follow-up (issue or link to
`#66762`) and add a TODO comment in the same DEPS.bzl entry referencing that
tracking issue/PR; when upstream kvproto is merged, update importpath back to
"github.com/pingcap/kvproto", replace strip_prefix, sha256 and the urls to the
official upstream release, and document the change in the follow-up so we can
revert the forked dependency cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 645f4ad1-6b39-4415-a206-04e789ae722a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
DEPS.bzlgo.modpkg/executor/internal/mpp/local_mpp_coordinator.gopkg/kv/mpp.gopkg/store/copr/mpp.go
|
/cc @windtalker @gengliqi PTAL. Can we push forward the kvproto changes and these tidb changes? |
|
@coderabbitai review |
windtalker
left a comment
windtalker
left a comment
There was a problem hiding this comment.
why this pr has many unrelated changes?
JaySon-Huang
force-pushed
the
feat/mpp-sql-digest-propagation
branch
from
7b82377 to
223dbcb
Compare
@windtalker I think the "many unrelated changes" you means is so many line changes in the file |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DEPS.bzl`:
- Around line 6819-6825: Remove the temporary replace directive that pins
github.com/JaySon-Huang/kvproto in go.mod (the replace at or around go.mod:379),
revert it to the official github.com/pingcap/kvproto module, then regenerate
Bazel metadata by running make bazel_prepare to update DEPS.bzl; do not manually
edit DEPS.bzl—ensure the replace is removed from go.mod first so the generated
DEPS.bzl references the official kvproto.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 222190c5-bec1-42f8-b1bc-04cb4a0bc74f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
DEPS.bzlgo.modpkg/executor/internal/mpp/local_mpp_coordinator.gopkg/kv/mpp.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
ti-chi-bot
bot
added
size/M
|
kvproto has been upgraded to a commit on pingcap/kvproto. @windtalker @gengliqi PTAL |
|
/retest |
|
@JaySon-Huang: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66763 +/- ##
================================================
- Coverage 77.7082% 77.5755% -0.1328%
================================================
Files 2016 1939 -77
Lines 552461 543568 -8893
================================================
- Hits 429308 421676 -7632
- Misses 121411 121890 +479
+ Partials 1742 2 -1740
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
JaySon-Huang
force-pushed
the
feat/mpp-sql-digest-propagation
branch
from
9ca1e48 to
8e8bd74
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/store/copr/mpp.go (1)
204-206: Consider multi-line formatting for readability.This single-line struct initialization is quite long and harder to scan compared to the multi-line style used in
DispatchMPPTask(lines 105-116). Consider aligning with the existing style for consistency.💅 Proposed formatting
firstReq := reqs[0] killReq := &mpp.CancelTaskRequest{ - Meta: &mpp.TaskMeta{StartTs: firstReq.StartTs, GatherId: firstReq.GatherID, QueryTs: firstReq.MppQueryID.QueryTs, LocalQueryId: firstReq.MppQueryID.LocalQueryID, ServerId: firstReq.MppQueryID.ServerID, MppVersion: firstReq.MppVersion.ToInt64(), ResourceGroupName: firstReq.ResourceGroupName, SqlDigest: firstReq.SQLDigest, PlanDigest: firstReq.PlanDigest}, + Meta: &mpp.TaskMeta{ + StartTs: firstReq.StartTs, + GatherId: firstReq.GatherID, + QueryTs: firstReq.MppQueryID.QueryTs, + LocalQueryId: firstReq.MppQueryID.LocalQueryID, + ServerId: firstReq.MppQueryID.ServerID, + MppVersion: firstReq.MppVersion.ToInt64(), + ResourceGroupName: firstReq.ResourceGroupName, + SqlDigest: firstReq.SQLDigest, + PlanDigest: firstReq.PlanDigest, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/copr/mpp.go` around lines 204 - 206, The one-line initialization of killReq with mpp.CancelTaskRequest and its nested mpp.TaskMeta is hard to read; refactor the creation of killReq into multi-line, aligned field assignments (one field per line) similar to DispatchMPPTask, breaking out Meta: &mpp.TaskMeta{ ... } across multiple lines and listing StartTs, GatherId, QueryTs, LocalQueryId, ServerId, MppVersion, ResourceGroupName, SqlDigest, and PlanDigest each on its own line using firstReq and firstReq.MppQueryID where appropriate to improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/store/copr/mpp.go`:
- Around line 204-206: The one-line initialization of killReq with
mpp.CancelTaskRequest and its nested mpp.TaskMeta is hard to read; refactor the
creation of killReq into multi-line, aligned field assignments (one field per
line) similar to DispatchMPPTask, breaking out Meta: &mpp.TaskMeta{ ... } across
multiple lines and listing StartTs, GatherId, QueryTs, LocalQueryId, ServerId,
MppVersion, ResourceGroupName, SqlDigest, and PlanDigest each on its own line
using firstReq and firstReq.MppQueryID where appropriate to improve readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20b20043-f554-4e5a-8860-32efceaf355d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
DEPS.bzlgo.modpkg/executor/internal/mpp/local_mpp_coordinator.gopkg/kv/mpp.gopkg/store/copr/mpp.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/kv/mpp.go
- DEPS.bzl
|
/retest |
|
@JaySon-Huang: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/test unit-test |
|
@JaySon-Huang: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
2 participants
What problem does this PR solve?
Issue Number: ref #66762
Problem Summary:
Short MPP queries (below slow log threshold) are hard to correlate between TiDB dispatch logs and TiFlash task logs because SQL digest is not propagated in
mpp.TaskMeta.What changed and how does it work?
SQLDigesttokv.MPPDispatchRequest.StmtCtx.SQLDigest()in local MPP coordinator, log it in dispatch/retry/error paths, and carry it in dispatch requests.TaskMeta.SqlDigestin TiDB MPP client for dispatch/cancel/establish connection request metadata.replaceto the branch containingTaskMeta.sql_digest) and refresh Bazel dependency metadata viamake bazel_prepare.Check List
Tests
Validation commands run:
go test ./pkg/kv -run TestNonExistentgo test ./pkg/store/copr -run TestNonExistentgo test ./pkg/executor/internal/mpp -run TestNonExistentmake lintmake bazel_prepare(with writable local Bazel cache/output paths in this environment)Before this PR
After this PR, the sql digest is added to the logging
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Chores