Reduce/downgrade some MPPTask logs#10821
Conversation
Signed-off-by: gengliqi <gengliqiii@gmail.com>
ti-chi-bot
Bot
added
release-note-none
|
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 selected for processing (1)
📝 WalkthroughWalkthroughThe MPP components now implement more selective logging strategies. Dispatch completion messages in MPPHandler are downgraded to debug level, while MPPTask adjusts log priorities dynamically based on operation timing thresholds and removes redundant runtime reporting from destructor logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
🧹 Nitpick comments (1)
dbms/src/Flash/Mpp/MPPTask.cpp (1)
158-158: Destructor finish log downgraded; note that total run-time is now only DEBUG-level.Downgrading the destructor log to DEBUG is fine for reducing noise. Just be aware the only remaining emission of
total_run_time_msis the DEBUG log at Line 789 ("task ends, time cost is {} ms."), so at the default INFO log level operators will no longer see per-task run-time. If you still want quick visibility for long-running tasks, consider applying the same duration-thresholdLOG_IMPLpattern you used inunregisterTaskat Line 789 (e.g., INFO iftotal_run_time_ms > threshold, DEBUG otherwise). Otherwise, LGTM.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbms/src/Flash/Mpp/MPPTask.cpp` at line 158, The destructor's finish log (LOG_DEBUG in MPPTask:: ~MPPTask or finish function where LOG_DEBUG(log, "finish MPPTask: {}", id.toString()) is emitted) currently only logs at DEBUG so total_run_time_ms is only visible at DEBUG; update the destructor's logging to use the same duration-threshold LOG_IMPL pattern used in unregisterTask (the code around unregisterTask that checks total_run_time_ms against a threshold and emits INFO when > threshold, DEBUG otherwise) so that long-running tasks still emit INFO with the total_run_time_ms while short runs remain DEBUG; locate the LOG_DEBUG call for finish MPPTask and replace it with the threshold-based LOG_IMPL/conditional that references total_run_time_ms and the same threshold variable/logic used in unregisterTask.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dbms/src/Flash/Mpp/MPPTask.cpp`:
- Line 158: The destructor's finish log (LOG_DEBUG in MPPTask:: ~MPPTask or
finish function where LOG_DEBUG(log, "finish MPPTask: {}", id.toString()) is
emitted) currently only logs at DEBUG so total_run_time_ms is only visible at
DEBUG; update the destructor's logging to use the same duration-threshold
LOG_IMPL pattern used in unregisterTask (the code around unregisterTask that
checks total_run_time_ms against a threshold and emits INFO when > threshold,
DEBUG otherwise) so that long-running tasks still emit INFO with the
total_run_time_ms while short runs remain DEBUG; locate the LOG_DEBUG call for
finish MPPTask and replace it with the threshold-based LOG_IMPL/conditional that
references total_run_time_ms and the same threshold variable/logic used in
unregisterTask.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb5c07ca-a2fe-4f7f-90ac-b249149f46a0
📒 Files selected for processing (3)
dbms/src/Flash/Mpp/MPPHandler.cppdbms/src/Flash/Mpp/MPPTask.cppdbms/src/Flash/Pipeline/Schedule/Events/Event.cpp
| current_memory_tracker = query_memory_tracker; | ||
| abortTunnels("", true); | ||
| LOG_INFO(log, "finish MPPTask: {}, total run time is {} ms", id.toString(), total_run_time_ms); | ||
| LOG_DEBUG(log, "finish MPPTask: {}", id.toString()); |
There was a problem hiding this comment.
why delete total_run_time?
There was a problem hiding this comment.
It's because the total_run_time_ms has already printed in this debug log LOG_DEBUG(log, "task ends, time cost is {} ms.", total_run_time_ms);
| { | ||
| schedule(); | ||
| LOG_INFO(log, "Event is scheduled"); | ||
| LOG_DEBUG(log, "Event is scheduled"); |
There was a problem hiding this comment.
This log should use info level
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: windtalker, xzhangxian1008 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/cherrypick release-8.5 |
|
@gengliqi: new pull request created to branch 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 ti-community-infra/tichi repository. |
close #10820\n\nReduce some MPPTask logs\n\nSigned-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>\nSigned-off-by: gengliqi <gengliqiii@gmail.com>\n\nCo-authored-by: Liqi Geng <gengliqiii@gmail.com>\nCo-authored-by: gengliqi <gengliqiii@gmail.com>
4 participants
What problem does this PR solve?
Issue Number: close #10820
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit