Uh oh!
There was an error while loading. Please reload this page.
branch-4.1: [opt](file-cache) support config hot reload of file cache microbench in running state - #62595
Conversation
Thearas
commented
Apr 17, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
gavinchou
commented
Apr 17, 2026
run buildall |
…in running state Pick apache#58922 to branch-4.1
e3c830f to
2e49b76Comparegavinchou
commented
Apr 22, 2026
run buildall |
hello-stephen
commented
Apr 22, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
commented
Apr 23, 2026
/review |
There was a problem hiding this comment.
Summary
Requesting changes.
The new hot-reload path currently has correctness issues in both the cache factory and the microbench service, and the read-path refactor introduces an eager buffer-allocation regression that can OOM large jobs.
Critical Checkpoints
- Correctness: fail. Cache-path removals are silently ignored during reload, so the hot-reloaded configuration can diverge from the active cache set.
- Concurrency/thread-safety: fail.
reload_file_cache()destroys liveBlockFileCacheinstances even though callers use raw pointers and assume_cachesis immutable after startup. - Reliability/operability: fail.
/reload_confignever executes a normal reload, the microbench now preallocates one max-sized read buffer per file, and startup can crash ifDORIS_HOMEis unset. - Testing/validation: not evidenced in this diff for the new reload paths.
- Repository approval policy (
tools/maintainers/check_review.py): not satisfied until the PR has 2 latest-stateAPPROVEDreviews.
User Focus
No additional user-provided review focus was supplied.
| return Status::OK(); | ||
| } | ||
| Status FileCacheFactory::reload_file_cache(const std::vector<CachePath>& cache_base_paths) { |
There was a problem hiding this comment.
reload_file_cache() says file-cache count changes are unsupported, but this only validates additions. If the new config removes an existing path, the method still succeeds and leaves the old BlockFileCache in _caches/_path_to_cache, so pick_one_cache_path() and get_by_path() can continue routing traffic to a cache that is no longer configured. We should reject removals as well, or explicitly rebuild the full cache set.
| } | ||
| // deconstruct target reload first | ||
| *cache_iter = std::unique_ptr<BlockFileCache>(); |
There was a problem hiding this comment.
Destroying and replacing the live BlockFileCache in place is not safe here. get_by_path() and several other callers return raw BlockFileCache* without taking _mtx, and this file explicitly documents that _caches is treated as immutable after startup. A concurrent reload can therefore free a cache object while reads/evictions are still using it, turning this into a use-after-free. It also drops the old cache before initialize() succeeds. We need a publish/swap scheme that preserves the old instance until users are quiesced or otherwise protected.
| std::string reload_details_stat() { | ||
| return _reloading.load() ? "Reloading config" : "Reload finished or not started"; | ||
| } |
There was a problem hiding this comment.
reload_request() sets _reloading = true and calls notify_one(), but this wait predicate only returns when _stop_thread is true. In other words, normal reload requests never wake the worker, so /reload_config returns OK while no reload ever runs, and /show_reload_status can stay stuck at Reloading config forever. The JobCount == 0 gating mentioned in the comment also is not implemented.
| } | ||
| private: | ||
| doris::S3ClientConf create_s3_client_conf(const JobConfig& config) { |
There was a problem hiding this comment.
Preallocating read_buffers for every entry in read_files changes the memory footprint from roughly O(num_threads * read_length) to O(num_files * read_length_right). For large jobs this can allocate gigabytes up front and OOM the microbench even though only num_threads reads run concurrently. The previous code only kept a buffer per active task.
| auto status = exec_env->init_mem_env(); | ||
| std::unique_ptr<doris::ThreadPool> s3_upload_pool; | ||
| static_cast<void>(doris::ThreadPoolBuilder("MicrobenchS3FileUploadThreadPool") |
There was a problem hiding this comment.
getenv() can return nullptr. Constructing std::string directly from that is undefined / throws before we even reach the empty() check, so the tool can crash immediately when DORIS_HOME is not exported. The old code explicitly handled the unset case by defaulting to ".".
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Pick #58922 to branch-4.1
Changes
reload_file_cachemethod to FileCacheFactory for hot reloading file cache configurationRELOADoperation to FileCacheAction HTTP endpointBenchEnvManagerclass to manage config loading and reload workerreload_configandshow_reload_statusRPC endpointsRelated Issue
close#58922