Conversation
📝 WalkthroughWalkthroughA logging level adjustment in the segment read task scheduler reduces message verbosity by changing a pending pool submission status log from info to debug level, with no control flow changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp (1)
71-77:⚠️ Potential issue | 🟡 MinorLogging macro level deviates from storage logging policy.
This change switches the event log to
LOG_DEBUG, but this path’s guideline requiresLOG_INFOwith contextual fields in storage engine code. Please keep this asLOG_INFO(context is already good).As per coding guidelines,
dbms/src/Storages/**/{DeltaMerge,KVStore,Page,S3}/**/*.{h,hpp,cpp}should useLoggerPtrandLOG_INFO(log, ...)with relevant context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp` around lines 71 - 77, The log call uses LOG_DEBUG but must follow storage logging policy and use LOG_INFO with the LoggerPtr; replace the LOG_DEBUG(...) invocation with LOG_INFO(pool->getLogger(), "Submitted, pool_id={} segment_count={} pending_pools={} cost={}ns", pool->pool_id, pool->getPendingSegmentCount(), pending_pools.size(), sw.elapsed()) so the storage code in SegmentReadTaskScheduler.cpp uses LOG_INFO and the existing contextual fields (pool->getLogger(), pool->pool_id, pool->getPendingSegmentCount(), pending_pools.size(), sw.elapsed()) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp`:
- Around line 71-77: The log call uses LOG_DEBUG but must follow storage logging
policy and use LOG_INFO with the LoggerPtr; replace the LOG_DEBUG(...)
invocation with LOG_INFO(pool->getLogger(), "Submitted, pool_id={}
segment_count={} pending_pools={} cost={}ns", pool->pool_id,
pool->getPendingSegmentCount(), pending_pools.size(), sw.elapsed()) so the
storage code in SegmentReadTaskScheduler.cpp uses LOG_INFO and the existing
contextual fields (pool->getLogger(), pool->pool_id,
pool->getPendingSegmentCount(), pending_pools.size(), sw.elapsed()) remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2275723-4ecc-4c7d-b458-fc4bc7771959
📒 Files selected for processing (1)
dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CalvinNeo 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 |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #10816
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit