Reduce/downgrade some MPPTask logs#10821
Conversation
Signed-off-by: gengliqi <gengliqiii@gmail.com>
|
[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 |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLogging verbosity reduced across Flash MPP components: several INFO logs downgraded to DEBUG; MPPTask adds timing around unregisterTask and uses dynamic log priority based on elapsed durations, plus dynamic startup log priority. 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)
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 |
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
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