dataroaring commented on PR #59482: URL: https://github.com/apache/doris/pull/59482#issuecomment-3873132051
I reviewed this PR in detail and found two blocking issues before merge: 1) Potential null-pointer write in dryrun prefetch path - In be/src/io/cache/cached_remote_file_reader.cpp, prefetch_range() submits a dryrun task with Slice dummy_buffer((char*)nullptr, size). - In read_at_impl(), there are fallback branches that can still do remote read into result.data + ... when block state is not DOWNLOADED (for example after wait/fallback path). - Because result.data is null in dryrun, this can crash under contention/timeout conditions. - Suggestion: avoid null Slice for dryrun (use a scratch buffer), or guard all copy/fallback write sites when is_dryrun=true. 2) Prefetch thread pool is always created with high min threads even when feature is off - SegmentPrefetchThreadPool is always initialized in ExecEnv::_init with min=32/max=2000. - But prefetch itself is gated by enable_query_segment_file_cache_prefetch / enable_compaction_segment_file_cache_prefetch (both default false). - This adds avoidable idle threads/memory on every BE, including deployments not using this feature. - Suggestion: lazy-init the pool only when prefetch is enabled (and cloud mode), or set min threads to 0 and scale on demand. Given these, I recommend addressing both before 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]
