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 3 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. |
📝 WalkthroughWalkthroughThe PR adds SQL digest tracking throughout the MPP (Massively Parallel Processing) execution pipeline, extracting SQL digests from session context and propagating them through dispatch requests and task metadata. It also expands Bazel dependencies and adds a Go module replacement for kvproto. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
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
|
|
||
| replace github.com/pingcap/kvproto => github.com/JaySon-Huang/kvproto v0.0.0-20260307160337-78644d3dc465 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the upstream kvproto has the SqlDigest field in TaskMeta
# Search for SqlDigest in the kvproto mpp.proto or generated Go files
echo "Checking if SqlDigest exists in pingcap/kvproto master branch..."
curl -s "https://raw.githubusercontent.com/pingcap/kvproto/master/pkg/mpp/mpp.pb.go" | grep -i "SqlDigest" || echo "SqlDigest not found in upstream kvproto master"Repository: pingcap/tidb
Length of output: 167
🏁 Script executed:
# Check what files were changed in this PR involving go.mod changes
# Look for Bazel-related metadata or BUILD files that may have been modified
git log -1 --name-only --pretty=format: | head -20Repository: pingcap/tidb
Length of output: 1336
🏁 Script executed:
# Also check if there are any BUILD, BUILD.bazel files or bazel-related changes
fd -e BUILD -e BUILD.bazel -e WORKSPACE | head -20Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
# Check the actual go.mod file to confirm the replace directive
if [ -f go.mod ]; then
echo "=== go.mod content around lines 375-380 ==="
sed -n '375,380p' go.mod
fiRepository: pingcap/tidb
Length of output: 429
🏁 Script executed:
# Search for any bazel-related files or configuration
find . -type f \( -name "*.bazel" -o -name "BUILD*" -o -name "WORKSPACE*" \) 2>/dev/null | grep -v node_modules | head -20Repository: pingcap/tidb
Length of output: 592
🏁 Script executed:
# Check if there are go_mod_* or bazel_* related changes
git status --short 2>/dev/null | grep -E "(bazel|BUILD|WORKSPACE|go\.(mod|sum))" || echo "No bazel/go.mod changes in current status"Repository: pingcap/tidb
Length of output: 97
🏁 Script executed:
# Check if MODULE.bazel or other bazel files were modified
# Look at the git diff to see what changed with bazel files
git diff --name-only HEAD~1..HEAD 2>/dev/null | grep -E "(\.bazel|MODULE|go\.mod|go\.sum)" || echo "Cannot get git diff"Repository: pingcap/tidb
Length of output: 75
🏁 Script executed:
# Alternative: Check git log to see what files were changed in the latest commit
git log -1 --name-status 2>/dev/null | grep -E "(\.bazel|MODULE|go\.mod|go\.sum)" || echo "Cannot get git log details"Repository: pingcap/tidb
Length of output: 30429
Remove temporary kvproto replace directive before merge.
This replace directive points to a personal fork (github.com/JaySon-Huang/kvproto). Ensure the upstream kvproto PR containing the TaskMeta.SqlDigest field is merged into pingcap/kvproto, then update the canonical dependency and remove this replace directive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 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.
|
@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. |
|
/cc @windtalker @gengliqi PTAL. Can we push forward the kvproto changes and these tidb changes? |
1 participant
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