github-actions[bot] commented on code in PR #65722:
URL: https://github.com/apache/doris/pull/65722#discussion_r3600423297
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2023,31 +2059,70 @@ std::string BlockFileCache::reset_capacity(size_t
new_capacity) {
ss << " index_queue released " << queue_released;
queue_released = remove_blocks(_ttl_queue);
ss << " ttl_queue released " << queue_released;
-
- _disk_resource_limit_mode = true;
- _disk_limit_mode_metrics->set_value(1);
ss << " total_space_released=" << space_released;
}
old_capacity = _capacity;
_capacity = new_capacity;
+ size_percentage = calc_size_percentage_unlocked(cache_lock);
_cache_capacity_metrics->set_value(_capacity);
+ _cur_cache_size_metrics->set_value(_cur_cache_size);
+ _disk_limit_mode_metrics->set_value(
+ _disk_resource_limit_mode.load(std::memory_order_relaxed));
+ _need_evict_cache_in_advance_metrics->set_value(
+ _need_evict_cache_in_advance.load(std::memory_order_relaxed));
}
- auto use_time = duration_cast<milliseconds>(steady_clock::time_point() -
adjust_start_time);
+ auto use_time = duration_cast<milliseconds>(steady_clock::now() -
adjust_start_time);
LOG(INFO) << "Finish tag deleted block. path=" << _cache_base_path
+ << " storage_type=" <<
file_cache_storage_type_to_string(_storage->get_type())
+ << " size_percent=" << size_percentage
<< " use_time=" << cast_set<int64_t>(use_time.count());
ss << " old_capacity=" << old_capacity << " new_capacity=" << new_capacity;
LOG(INFO) << ss.str();
return ss.str();
}
void BlockFileCache::check_disk_resource_limit() {
- if (_storage->get_type() != FileCacheStorageType::DISK) {
+ // ATTN: due to that can be changed dynamically, set it to default value
if it's invalid
+ // FIXME: reject with config validator
+ if (config::file_cache_enter_disk_resource_limit_mode_percent <=
+ config::file_cache_exit_disk_resource_limit_mode_percent) {
+ LOG_WARNING("config error, set to default value")
+ .tag("enter",
config::file_cache_enter_disk_resource_limit_mode_percent)
+ .tag("exit",
config::file_cache_exit_disk_resource_limit_mode_percent);
+ config::file_cache_enter_disk_resource_limit_mode_percent = 88;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+ }
+ size_t size_percentage = 0;
+ {
+ SCOPED_CACHE_LOCK(_mutex, this);
+ size_percentage = calc_size_percentage_unlocked(cache_lock);
+ }
+ auto is_insufficient = [](const int& percentage) {
+ return percentage >=
config::file_cache_enter_disk_resource_limit_mode_percent;
+ };
+ 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));
Review 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.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2023,31 +2059,70 @@ std::string BlockFileCache::reset_capacity(size_t
new_capacity) {
ss << " index_queue released " << queue_released;
queue_released = remove_blocks(_ttl_queue);
ss << " ttl_queue released " << queue_released;
-
- _disk_resource_limit_mode = true;
- _disk_limit_mode_metrics->set_value(1);
ss << " total_space_released=" << space_released;
}
old_capacity = _capacity;
_capacity = new_capacity;
+ size_percentage = calc_size_percentage_unlocked(cache_lock);
_cache_capacity_metrics->set_value(_capacity);
+ _cur_cache_size_metrics->set_value(_cur_cache_size);
+ _disk_limit_mode_metrics->set_value(
+ _disk_resource_limit_mode.load(std::memory_order_relaxed));
+ _need_evict_cache_in_advance_metrics->set_value(
+ _need_evict_cache_in_advance.load(std::memory_order_relaxed));
}
- auto use_time = duration_cast<milliseconds>(steady_clock::time_point() -
adjust_start_time);
+ auto use_time = duration_cast<milliseconds>(steady_clock::now() -
adjust_start_time);
LOG(INFO) << "Finish tag deleted block. path=" << _cache_base_path
+ << " storage_type=" <<
file_cache_storage_type_to_string(_storage->get_type())
+ << " size_percent=" << size_percentage
<< " use_time=" << cast_set<int64_t>(use_time.count());
ss << " old_capacity=" << old_capacity << " new_capacity=" << new_capacity;
LOG(INFO) << ss.str();
return ss.str();
}
void BlockFileCache::check_disk_resource_limit() {
- if (_storage->get_type() != FileCacheStorageType::DISK) {
+ // ATTN: due to that can be changed dynamically, set it to default value
if it's invalid
+ // FIXME: reject with config validator
+ if (config::file_cache_enter_disk_resource_limit_mode_percent <=
+ config::file_cache_exit_disk_resource_limit_mode_percent) {
+ LOG_WARNING("config error, set to default value")
+ .tag("enter",
config::file_cache_enter_disk_resource_limit_mode_percent)
+ .tag("exit",
config::file_cache_exit_disk_resource_limit_mode_percent);
+ config::file_cache_enter_disk_resource_limit_mode_percent = 88;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+ }
+ size_t size_percentage = 0;
+ {
+ SCOPED_CACHE_LOCK(_mutex, this);
+ size_percentage = calc_size_percentage_unlocked(cache_lock);
+ }
+ auto is_insufficient = [](const int& percentage) {
+ return percentage >=
config::file_cache_enter_disk_resource_limit_mode_percent;
+ };
+ 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));
+ if (is_size_insufficient) {
+ current_mode = true;
+ } else if (current_mode &&
+ 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);
Review 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`.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2023,31 +2059,70 @@ std::string BlockFileCache::reset_capacity(size_t
new_capacity) {
ss << " index_queue released " << queue_released;
queue_released = remove_blocks(_ttl_queue);
ss << " ttl_queue released " << queue_released;
-
- _disk_resource_limit_mode = true;
- _disk_limit_mode_metrics->set_value(1);
ss << " total_space_released=" << space_released;
}
old_capacity = _capacity;
_capacity = new_capacity;
+ size_percentage = calc_size_percentage_unlocked(cache_lock);
_cache_capacity_metrics->set_value(_capacity);
+ _cur_cache_size_metrics->set_value(_cur_cache_size);
+ _disk_limit_mode_metrics->set_value(
+ _disk_resource_limit_mode.load(std::memory_order_relaxed));
+ _need_evict_cache_in_advance_metrics->set_value(
+ _need_evict_cache_in_advance.load(std::memory_order_relaxed));
}
- auto use_time = duration_cast<milliseconds>(steady_clock::time_point() -
adjust_start_time);
+ auto use_time = duration_cast<milliseconds>(steady_clock::now() -
adjust_start_time);
LOG(INFO) << "Finish tag deleted block. path=" << _cache_base_path
+ << " storage_type=" <<
file_cache_storage_type_to_string(_storage->get_type())
+ << " size_percent=" << size_percentage
<< " use_time=" << cast_set<int64_t>(use_time.count());
ss << " old_capacity=" << old_capacity << " new_capacity=" << new_capacity;
LOG(INFO) << ss.str();
return ss.str();
}
void BlockFileCache::check_disk_resource_limit() {
- if (_storage->get_type() != FileCacheStorageType::DISK) {
+ // ATTN: due to that can be changed dynamically, set it to default value
if it's invalid
+ // FIXME: reject with config validator
+ if (config::file_cache_enter_disk_resource_limit_mode_percent <=
+ config::file_cache_exit_disk_resource_limit_mode_percent) {
+ LOG_WARNING("config error, set to default value")
+ .tag("enter",
config::file_cache_enter_disk_resource_limit_mode_percent)
+ .tag("exit",
config::file_cache_exit_disk_resource_limit_mode_percent);
+ config::file_cache_enter_disk_resource_limit_mode_percent = 88;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+ }
+ size_t size_percentage = 0;
+ {
+ SCOPED_CACHE_LOCK(_mutex, this);
+ size_percentage = calc_size_percentage_unlocked(cache_lock);
+ }
+ auto is_insufficient = [](const int& percentage) {
+ return percentage >=
config::file_cache_enter_disk_resource_limit_mode_percent;
+ };
+ 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));
+ if (is_size_insufficient) {
+ current_mode = true;
Review 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.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2113,7 +2175,41 @@ void BlockFileCache::check_disk_resource_limit() {
}
void BlockFileCache::check_need_evict_cache_in_advance() {
- if (_storage->get_type() != FileCacheStorageType::DISK) {
+ // ATTN: due to that can be changed dynamically, set it to default value
if it's invalid
+ // FIXME: reject with config validator
+ if (config::file_cache_enter_need_evict_cache_in_advance_percent <=
+ config::file_cache_exit_need_evict_cache_in_advance_percent) {
+ LOG_WARNING("config error, set to default value")
+ .tag("enter",
config::file_cache_enter_need_evict_cache_in_advance_percent)
+ .tag("exit",
config::file_cache_exit_need_evict_cache_in_advance_percent);
+ config::file_cache_enter_need_evict_cache_in_advance_percent = 78;
+ config::file_cache_exit_need_evict_cache_in_advance_percent = 75;
+ }
+ size_t size_percentage = 0;
+ {
+ SCOPED_CACHE_LOCK(_mutex, this);
+ size_percentage = calc_size_percentage_unlocked(cache_lock);
+ }
+ auto is_insufficient = [](const int& percentage) {
+ return percentage >=
config::file_cache_enter_need_evict_cache_in_advance_percent;
+ };
+ const bool previous_mode =
_need_evict_cache_in_advance.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));
+ if (is_size_insufficient) {
+ current_mode = true;
+ } else if (current_mode &&
+ 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);
Review 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.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2327,7 +2408,7 @@ void BlockFileCache::run_background_evict_in_advance() {
batch = config::file_cache_evict_in_advance_batch_bytes;
// 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) ||
Review 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.
--
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]
