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 5 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds SQL/Plan digest propagation through the MPP pipeline (session → dispatch requests → task metadata → connection establishment), expands Bazel Go repository entries in Changes
Sequence Diagram(s)sequenceDiagram
participant Session
participant Coordinator
participant KV as "KV Dispatcher"
participant Store as "Coprocessor/Store"
Session->>Coordinator: Start MPP execution (context contains SQLDigest, PlanDigest)
Coordinator->>KV: Build MPPDispatchRequest (include SQLDigest, PlanDigest)
KV->>Store: DispatchTask (TaskMeta with SqlDigest, PlanDigest)
Store->>Store: Execute task (trace/log with digests)
Store-->>KV: Task response / errors (include digests)
KV-->>Coordinator: Dispatch response (propagate digests)
Coordinator-->>Session: Report results / errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed 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 togithub.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 |
|
@JaySon-Huang: 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. |
windtalker
left a comment
windtalker
left a comment
There was a problem hiding this comment.
why this pr has many unrelated changes?
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