dataroaring commented on PR #59925:
URL: https://github.com/apache/doris/pull/59925#issuecomment-3774165135
# Code Review: [fix](filecache) fix global metrics leakage when stat is
nullptr
## Summary
This PR fixes a bug where global `FileCacheMetrics` were not being updated
when `io_ctx->file_cache_stats` was null. The original code had both updates
gated behind the null check.
## Analysis
**Before:**
```cpp
if (io_ctx->file_cache_stats && !is_dryrun) {
// update stats in io_ctx, for query profile
_update_stats(stats, io_ctx->file_cache_stats,
io_ctx->is_inverted_index);
// update stats increment for file cache metrics
FileCacheStatistics fcache_stats_increment;
_update_stats(stats, &fcache_stats_increment, io_ctx->is_inverted_index);
io::FileCacheMetrics::instance().update(&fcache_stats_increment); //
<-- only if file_cache_stats != null
}
```
**After:**
```cpp
if (is_dryrun) {
return;
}
// update stats increment for file cache metrics (ALWAYS)
FileCacheStatistics fcache_stats_increment;
_update_stats(stats, &fcache_stats_increment, io_ctx->is_inverted_index);
io::FileCacheMetrics::instance().update(&fcache_stats_increment);
if (io_ctx->file_cache_stats) {
// update stats in io_ctx, for query profile (only if non-null)
_update_stats(stats, io_ctx->file_cache_stats,
io_ctx->is_inverted_index);
}
```
## Verdict
**LGTM** - The fix correctly separates the two concerns:
1. Global metrics should always be updated (when not dryrun)
2. Per-query profile stats only updated when the stats object exists
The logic is correct and the change is minimal and focused.
### Minor Suggestion
The early return for `is_dryrun` improves readability. Consider adding a
brief comment explaining why dryrun skips metrics:
```cpp
if (is_dryrun) {
// Dryrun mode is used for cache warming - don't pollute metrics
return;
}
```
**Approved** for merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
