github-actions[bot] commented on code in PR #67313:
URL: https://github.com/apache/doris/pull/67313#discussion_r3888520764
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -3034,6 +3034,114 @@ TEST_F(BlockFileCacheTest, test_async_load_with_limit) {
}
}
+TEST_F(BlockFileCacheTest, test_async_load_respects_capacity_after_reset) {
+ if (fs::exists(cache_base_path)) {
+ fs::remove_all(cache_base_path);
+ }
+ fs::create_directories(cache_base_path);
+ const auto original_monitor_interval =
config::file_cache_background_monitor_interval_ms;
+ config::file_cache_background_monitor_interval_ms = 1000000;
+ Defer cleanup {[&] {
+ config::file_cache_background_monitor_interval_ms =
original_monitor_interval;
+ if (fs::exists(cache_base_path)) {
+ fs::remove_all(cache_base_path);
+ }
+ }};
+
+ io::FileCacheSettings settings;
+ settings.query_queue_size = 100;
+ settings.query_queue_elements = 20;
+ settings.capacity = 100;
+ settings.max_file_block_size = 5;
+ settings.max_query_cache_size = 0;
+ io::BlockFileCache cache(cache_base_path, settings);
+ ASSERT_TRUE(cache.initialize());
+ wait_until_cache_ready(cache);
+
+ const bool original_async_open_done =
cache._async_open_done.exchange(false);
+ Defer restore_async_open_done {[&] { cache._async_open_done =
original_async_open_done; }};
+ cache._disk_resource_limit_mode = false;
Review Comment:
[P2] Synchronize mode setup with the background monitor
These tests assign the plain-bool _disk_resource_limit_mode directly after
initialize(). run_background_monitor() performs its first
check_disk_resource_limit() before waiting, so setting
file_cache_background_monitor_interval_ms to 1000000 does not order that first
access; wait_until_cache_ready() only waits for _async_open_done. On a schedule
where the monitor has not completed its first check, the assignment at this
line (and the analogous writes in the second async test/reset test) races the
monitor's read/write and can make the setup nondeterministic under TSAN. Gate
the first monitor check with a SyncPoint or otherwise establish synchronization
before mutating the flag.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2577,6 +2569,15 @@ BlockFileCache::get_hot_blocks_meta(const
UInt128Wrapper& hash) const {
bool BlockFileCache::try_reserve_during_async_load(size_t size,
std::lock_guard<std::mutex>& cache_lock) {
+ size_t evict_target =
Review Comment:
[P1] Enforce the reduced capacity on persisted async-load inserts
try_reserve_during_async_load() is now capacity-aware, but it is only
reached from split_range_into_cells() for newly created holes. While
_async_open_done is false,
FSFileCacheStorage::load_cache_info_into_memory_from_db()/from_fs() and
load_blocks_directly_unlocked() still call add_cell(... DOWNLOADED) directly.
If the loader is held, reset_capacity(100 -> 30) can evict the currently loaded
cells, then the pending metadata batches (or a lazy hit for a not-yet-loaded
hash) repopulate _files without consulting _capacity, leaving _cur_cache_size
above the new limit. Please route every async metadata insertion through the
same capacity-aware admission/eviction policy (or defer publication until load
completes), and add a loader-held shrink/direct-hit regression test.
--
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]
