[fix](filecache) Support memory file cache resource management#65722
[fix](filecache) Support memory file cache resource management#65722freemandealer wants to merge 1 commit into
Conversation
|
/review |
|
run buildall |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes. I found eight distinct correctness, lifecycle, compatibility, and test-validity issues in the inline comments.
Critical checkpoint conclusions:
- Goal and proof: The PR correctly identifies that memory storage should use capacity pressure and skip disk-oriented LRU persistence, and it adds focused unit coverage, but the end-to-end goal is not yet met: the production reset route bypasses the memory implementation, clear still touches LRU files, and the pressure transitions/tests are not synchronized.
- Scope and parallel paths: The source diff is mostly focused, but the unchanged factory/HTTP reset and clear paths are required parallel entry points. The reset loop also has an eventual holder-release effect not captured by its immediate accounting.
- Concurrency: Memory mode now has monitor, request, and advance-eviction threads concurrently accessing plain pressure flags without a happens-before relationship. The new monitor test also mutates cache size and observes flags without synchronization.
- Lifecycle and persistence: Skipping restore, recorder replay/dump threads, and explicit dumps for memory storage is directionally correct; clear-time dump deletion remains, and marking held blocks while evicting the full releasable target causes later double eviction.
- Configuration and compatibility: No FE/BE protocol or storage-format change is involved, and the disk LRU path is otherwise preserved. However, equal disk-resource thresholds were previously coherent/accepted and are now silently rewritten to unrelated fallback values.
- Test coverage and validation: Eleven BE unit tests are added, but factory/HTTP reset, memory clear, grow-from-pressure convergence, normal holder destruction, and deterministic LRU replay are not validly covered. Per the automated-review contract, I did not run builds or tests locally; this review is based on the authoritative diff and static control-flow/lifecycle analysis.
- Observability and performance: The new storage-type/size stats and transition logs are useful. Stale pressure flags after capacity growth and double eviction can nevertheless discard valid cache data and sharply reduce hit rate.
- Transactions, data visibility, and protocol transport: Not applicable to this cache-management-only change; no transaction/EditLog, MoW visibility, or new FE-BE variable path is modified.
- User focus: No additional user-provided focus was supplied; the full PR was reviewed.
| if (is_memory_storage()) { | ||
| bool is_size_insufficient = is_insufficient(cast_set<int>(size_percentage)); | ||
| if (is_size_insufficient) { | ||
| _disk_resource_limit_mode = true; |
There was a problem hiding this comment.
[P1] Synchronize the pressure-mode flags
For memory storage the monitor now writes _disk_resource_limit_mode and _need_evict_cache_in_advance here without _mutex, while request threads read the first flag in try_reserve()/is_overflow(), the advance worker reads the second before taking _mutex, and get_stats() reads both unlocked. A reader-side lock does not synchronize with this writer, so a request arriving as the cache crosses 90% has a C++ data race and can miss or inconsistently observe the new mode. Please make these cross-thread signals atomic with consistent loads/stores, or protect every read and write with the same lock.
| static_cast<double>(_capacity) * 100); | ||
| } | ||
|
|
||
| std::string BlockFileCache::reset_capacity(size_t new_capacity) { |
There was a problem hiding this comment.
[P1] Route memory resets around disk validation
The production reset path does not reliably reach this new memory-aware implementation. HTTP RESET calls FileCacheFactory::reset_capacity(), and that function first runs statfs() on every cache key. A memory cache is keyed by the literal memory path and no directory is created for it, so path=memory normally returns before this method; if such a directory happens to exist, RAM capacity is clamped against an unrelated filesystem. The all-cache branch can also reset earlier disk entries before failing on memory. Please branch validation by storage type and prevalidate every target before applying an all-cache reset, with factory/HTTP-path coverage.
TPC-H: Total hot run time: 29635 ms |
TPC-DS: Total hot run time: 178203 ms |
ClickBench: Total hot run time: 25.48 s |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Memory file cache storage still followed disk-oriented LRU persistence behavior and skipped capacity-based resource limit monitoring. The adapted monitor also crashed when a memory cache had zero capacity, its pressure-mode flags were accessed concurrently without synchronization, and the production reset path validated the literal memory cache path with statfs. An all-cache reset could therefore partially mutate disk caches before failing on a memory or invalid target. Adapt the required SelectDB fixes to current Doris master, preserve the newer sharded memory storage duplicate-key handling already present on master, make cross-thread pressure signals atomic, handle zero-capacity statistics safely, and prevalidate storage-aware reset targets before applying any capacity changes.
### Release note
Improve memory file cache resource monitoring and capacity reset behavior while avoiding disk-only persistence and validation work.
### Check List (For Author)
- Test: Unit Test
- `BlockFileCacheMemModeTest.*:FileCacheActionTest.reset_memory_capacity_without_memory_directory` (15 tests passed)
- BE compilation with `./build.sh --be`
- BE clang-format and clang-tidy checks
- Behavior changed: Yes. Memory file cache uses capacity percentage for pressure modes, synchronizes pressure signals, safely handles zero capacity, skips disk-only LRU persistence, and resets capacity without filesystem validation.
- Does this need documentation: No
b62d21e to
05b1324
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes. I found five additional distinct correctness and cache-policy issues in the inline comments; the five unresolved comments from the prior review also remain applicable.
Critical checkpoint conclusions:
- Goal and proof: Capacity-based pressure for memory storage and suppression of disk-oriented LRU persistence are directionally correct, but the end-to-end goal is not yet met. The new memory pressure paths can delete a replacement payload, terminate the monitor after a legal tiny shrink, violate query-LRU size accounting, and evict far below both exit watermarks.
- Scope and parallel paths: The diff is focused. I traced request admission, query accounting, monitor and advance workers, async GC, holder destruction, factory/HTTP reset, clear, and disk LRU compatibility. Five earlier comments remain unresolved: held-block eventual over-eviction, post-reset stale modes, equal-threshold rewriting, memory clear touching LRU files, and dump-before-replay test ordering.
- Concurrency and lifecycle: Converting the policy flags to atomics fixes the earlier flag race, but it does not make the cached policies self-refreshing. The monitor, request, advance, and GC actors still have the logical state/lifecycle failures described inline; lock order itself is unchanged.
- Configuration and compatibility: No new configuration definition, FE/BE protocol, persisted storage format, authentication boundary, or transaction path is introduced. Disk recording/replay/dump behavior is otherwise preserved, subject to the existing equal-threshold and replay-test comments.
- Test coverage and validation: Fifteen focused BE tests are added, but none covers generation-safe advance deletion, tiny held-block shrink through both pressure checks, real-block exit-watermark capping, sustained request misses between monitor ticks, or pressure with query limiting. Per the automated-review contract I did not run local builds or tests;
.worktree_initializedandthirdparty/installedare absent, so this conclusion is based on the authoritative patch and static control-flow/lifecycle analysis. - Observability and performance: The new storage-type/percentage stats and transition logs are useful. The two watermark failures can still drain valid cache contents and sharply reduce hit rate, while the query-accounting mismatch can reject useful admissions.
- Transactions, MoW visibility, EditLog, and FE/BE variable transport: Not applicable to this BE cache-management change.
- User focus: No additional user-provided focus was supplied; the full PR was reviewed.
| const bool previous_mode = _disk_resource_limit_mode.load(std::memory_order_relaxed); | ||
| bool current_mode = previous_mode; | ||
| if (is_memory_storage()) { | ||
| bool is_size_insufficient = is_insufficient(cast_set<int>(size_percentage)); |
There was a problem hiding this comment.
[P1] Keep the computed pressure percentage wide
A valid reset can leave _cur_cache_size far above the new capacity while readers still hold blocks. For example, three held 10 MiB blocks plus RESET capacity=1 (accepted because it is positive) yields 3,145,728,000%; cast_set<int> throws above INT_MAX. This runs from the uncaught monitor-thread entry, so the next check terminates the BE. Compare the size_t percentage against a safely converted threshold (or clamp it) without narrowing, and add a held-block tiny-shrink case that runs both pressure checks.
| if (is_memory_storage()) { | ||
| bool is_size_insufficient = is_insufficient(cast_set<int>(size_percentage)); | ||
| if (is_size_insufficient) { | ||
| current_mode = true; |
There was a problem hiding this comment.
[P2] Stop five-times memory eviction below the exit watermark
This newly lets cache-size pressure enable the disk resource-limit policy for memory storage, but requests only consume the cached flag; they never refresh it after their own eviction. With 95/100 MiB cached, successive 1 MiB misses can evict 5 MiB then admit 1 MiB, taking usage 95 -> 91 -> 87. Although that is already below the 88% exit watermark, more requests keep evicting five-for-one until the 5-second monitor runs, potentially draining all releasable cache or returning SKIP_CACHE. This is distinct from the reset-growth thread because no reset is involved. Recheck/clear the memory policy from the locked current size before applying the enlarged eviction target, while preserving the real request size for query accounting, and cover sustained misses between monitor ticks.
| size_percentage < config::file_cache_exit_disk_resource_limit_mode_percent) { | ||
| current_mode = false; | ||
| } | ||
| _disk_resource_limit_mode.store(current_mode, std::memory_order_relaxed); |
There was a problem hiding this comment.
[P1] Keep query accounting at the actual block size
This newly enables resource-limit mode for memory caches, but try_reserve() multiplies its local size by five before passing it to QueryFileCacheContext::reserve(). The caller still creates the cell with the original current_size, so a 10 MiB block is recorded as 50 MiB in the query LRU. The next over-limit admission hits DCHECK(iter->size == cell_size) (or, without checks, evicts/rejects using inconsistent byte counts). Keep the real block size for query accounting and use a separate enlarged eviction target, then cover memory pressure with enable_file_cache_query_limit=true.
| size_percentage < config::file_cache_exit_need_evict_cache_in_advance_percent) { | ||
| current_mode = false; | ||
| } | ||
| _need_evict_cache_in_advance.store(current_mode, std::memory_order_relaxed); |
There was a problem hiding this comment.
[P1] Prevent a stale memory recycle from deleting a replacement
This newly enables the advance-eviction worker for memory storage, but that worker removes metadata with sync=false and queues only the block's hash/offset for GC. Before GC consumes the entry, a request for the same block can create a new cell and replace the still-present payload in ShardMemHashTable; the old queued remove() then erases that replacement by the same key. The new cell remains DOWNLOADED, so its next read fails with key not found in in-memory cache map. Make memory advance deletion synchronous, or give queued deletions a generation/identity check, and cover pause-GC -> evict -> redownload -> resume-GC deterministically.
|
|
||
| // Skip if eviction not needed or too many pending recycles | ||
| if (!_need_evict_cache_in_advance || | ||
| if (!_need_evict_cache_in_advance.load(std::memory_order_relaxed) || |
There was a problem hiding this comment.
[P2] Stop memory advance eviction at the exit watermark
This worker now consumes the memory-capacity flag, but its fixed batch ignores the exit watermark and only the 5-second monitor clears the flag. With the defaults, a 90/100 MiB memory cache needs to release only 5 MiB to reach the 85% exit point, yet the first 1-second worker pass can evict 30 MiB and fall to 60 MiB; later passes can continue toward empty before the monitor refreshes the flag. This is distinct from the existing reset-growth thread because no reset is involved. Under _mutex, cap each memory pass to the bytes above the exit watermark and clear/skip when already at or below it, then cover first-pass capping and later-pass termination with real blocks.
TPC-H: Total hot run time: 29575 ms |
TPC-DS: Total hot run time: 177379 ms |
ClickBench: Total hot run time: 24.98 s |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Memory file cache storage still followed disk-oriented LRU persistence behavior and skipped capacity-based resource limit monitoring. This could leave unused LRU records without a consumer, access disk persistence paths in memory mode, and fail to enter capacity pressure modes. Adapt the required parts of the SelectDB fixes to current Doris master, while retaining the newer sharded memory storage duplicate-key handling already present on master.
Release note
Improve memory file cache capacity monitoring and avoid disk-only LRU persistence work.
Check List (For Author)
BlockFileCacheMemModeTest.*(11 tests passed)./build.sh --be