Doris-Breakwater commented on issue #68892: URL: https://github.com/apache/doris/issues/68892#issuecomment-6098931792
Breakwater-GitHub-Analysis-Slot: slot_ce484c8cde74 **Initial assessment:** The reported unbounded eviction work is supported by source inspection and warrants a file-cache performance fix. The resulting upload-pool contention is mechanically plausible, but the reported Stream Load stalls and latency improvement still need runtime evidence. The issue is open, currently has no labels, and the author is willing to submit a PR; suggested triage is BE/file-cache performance, with elevated priority given the reported ingestion impact. **Verified code behavior** I inspected the configured local Doris repository at tag `4.1.4` (commit `ad35a140c7fd0b842f18c23300bac581f7d04326`) and retrieved current public master at `dd456ab7a894f54c6553415223c6d5c9c7d4e035`. The `find_evict_candidates` function is identical between those revisions. This is static verification; I have not reproduced the workload or run a patched benchmark. - [`find_evict_candidates`](https://github.com/apache/doris/blob/dd456ab7a894f54c6553415223c6d5c9c7d4e035/be/src/io/cache/block_file_cache.cpp#L1388-L1416) traverses the queue until the capacity requirement is satisfied or the queue ends. There is no examined-entry budget, and non-releasable entries are neither removed nor rotated. `removed_size` counts selected eviction candidates, so pinned entries make no progress toward the stopping condition. Repeated reservations can therefore traverse the same pinned prefix, with work proportional to its length each time. - [`get_or_set`](https://github.com/apache/doris/blob/4.1.4/be/src/io/cache/block_file_cache.cpp#L1183-L1247) holds the cache object's `_mutex` across range splitting, reservations, candidate selection and removal. This mutex is shared by operations on that `BlockFileCache` instance; it is not a single mutex spanning every cache directory. [`releasable()`](https://github.com/apache/doris/blob/4.1.4/be/src/io/cache/block_file_cache.h#L159-L166) checks shared ownership, including a special allowance for ownership by a cached reader. Retained block references can prevent eviction. - The S3 writer installs an allocation callback which reaches `FileCacheAllocatorBuilder::allocate_cache_holder` → `get_or_set` → `try_reserve` → `try_reserve_for_lru`. [`UploadFileBuffer::on_upload`](https://github.com/apache/doris/blob/4.1.4/be/src/io/fs/s3_file_bufferpool.cpp#L148-L187) performs remote upload before local cache population, on the upload worker. With `enable_flush_file_cache_async=true`, it signals part completion before cache population, but the worker remains occupied until cache population returns. With it false, completion also waits for cache population. This supports the proposed upload-pool backpressure mechanism; establishing its contribution to the observed stalls requires the effective setting and worker stacks. **PR considerations** The proposed scan cap, rotation and counters address the observed mechanism. Please make the following behavior explicit in the PR: 1. **Propagate budget exhaustion.** The [time-interval eviction loop](https://github.com/apache/doris/blob/dd456ab7a894f54c6553415223c6d5c9c7d4e035/be/src/io/cache/block_file_cache.cpp#L1649-L1689) runs before the helper, and [`try_reserve_for_lru`](https://github.com/apache/doris/blob/dd456ab7a894f54c6553415223c6d5c9c7d4e035/be/src/io/cache/block_file_cache.cpp#L1760-L1791) falls back to the current queue after other-queue failure. A helper-only cap does not bound total reservation work. Distinguish exhaustion from ordinary insufficient candidates so it cannot trigger further full scans. Cover or explicitly scope out the query-limit, startup/disk-resource-limit and advance-eviction paths. Also account for multiple block reservations inside one `get_or_set` lock acquisition. 2. **Preserve safe fallback and accounting.** If the budget is exhausted while space is still insufficient, return reservation failure. [`split_range_into_cells`](https://github.com/apache/doris/blob/4.1.4/be/src/io/cache/block_file_cache.cpp#L1057-L1100) already converts that failure to `SKIP_CACHE`. Define how partially selected candidates are handled; count only actual removals and preserve capacity and query accounting. Reaching the cap after already selecting enough space should still succeed. Treat 4096 as a proposed default requiring measurement, and define mutable-config boundary values. 3. **Rotate without revisiting entries.** `LRUQueue::move_to_end` uses `std::list::splice`. Advance the traversal iterator before moving its current node, and limit traversal to the original entries or another explicit pass boundary. A naive modification of the current range-for loop can skip the remaining queue or revisit rotated entries. Preserve stored iterators and record `MOVETOBACK` through the LRU recorder, as existing touch paths do. Preserve hot-data/TTL policy and define whether rotation changes access time. 4. **Measure remaining lock work.** An entry cap bounds scan work, not elapsed lock time: foreground eviction also takes block mutexes and performs synchronous storage removal under the cache mutex. Add scanned/rotated/truncated counters and a per-reservation scan distribution, preferably separated by cache and eviction path, alongside existing lock-wait and storage-removal latency metrics. **Evidence needed and next steps** Please attach the proposed patch/PR and exact BE build commit, plus a reproducible workload with cache capacity, block size, queue sizes, pinned-block counts, ingestion/read concurrency, and effective write-through, async-flush, async-LRU-touch and query-limit settings. For the same baseline and patched measurement windows, provide scan counts, lock hold/wait distributions, cache hit/bypass rates, ingestion throughput, upload-pool utilization/queueing, and storage-removal latency. The existing per-cache bvars include `file_cache_cache_lock_wait_time_us` and `file_cache_storage_sync_remove_latency_us`. Capture worker stacks and CPU samples of the lock holder during a stall; waiter stacks alone establish contention but do not identify the work holding the lock. Lock-wait and lock-held warning thresholds default to 30 seconds in the inspected tag, so missing warnings would not exclude the reported subsecond/seconds waits. Suggested regression coverage: an all-pinned queue exceeding the budget; a pinned prefix followed by an evictable block within/beyond the budget; repeated reservations followed by holder release; partial eviction and exact-budget success; cross-queue exhaustion; multi-block requests; and preserved LRU recorder/restore and capacity accounting. These checks should establish bounded progress and fallback correctness before interpreting the reported approximately 80 µs average as a general result. -- 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]
