github-actions[bot] commented on code in PR #67313:
URL: https://github.com/apache/doris/pull/67313#discussion_r3888847245
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2585,7 +2597,7 @@ bool BlockFileCache::try_reserve_during_async_load(size_t
size,
std::vector<FileBlockCell*> to_evict;
auto collect_eliminate_fragments = [&](LRUQueue& queue) {
for (const auto& [entry_key, entry_offset, entry_size] : queue) {
- if (!_disk_resource_limit_mode || removed_size >= size) {
+ if (removed_size >= evict_target) {
Review Comment:
[P1] Bound the async-load candidate scan under `_mutex`
When a full/restored cache has a capacity gap but its queued blocks are held
by active readers, `evict_target` is positive while `removed_size` never
advances because `releasable()` is false. The new loop therefore traverses all
disposable, normal, and index LRU entries before the permissive return, and
every block-sized miss repeats that O(total cached blocks) walk while
`_async_open_done` is false. This was an immediate break in normal mode before
this change. Please bound/defer this scan (or maintain a releasable-candidate
index) so async-load misses cannot block all cache access for an unbounded
queue traversal.
##########
be/test/io/cache/block_file_cache_test.cpp:
##########
@@ -3034,6 +3034,233 @@ 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; }};
+ io::CacheContext context;
+ ReadStatistics rstats;
+ context.stats = &rstats;
+ context.cache_type = io::FileCacheType::NORMAL;
+ auto key = io::BlockFileCache::hash("async-load-capacity");
+ for (size_t offset = 0; offset < 100; offset += 5) {
+ auto holder = cache.get_or_set(key, offset, 5, context);
+ auto blocks = fromHolder(holder);
+ ASSERT_EQ(blocks.size(), 1);
+ ASSERT_EQ(blocks[0]->get_or_set_downloader(),
io::FileBlock::get_caller_id());
+ download(blocks[0]);
+ }
+ ASSERT_EQ(cache._cur_cache_size, 100);
+
+ cache.reset_capacity(30);
Review Comment:
[P2] Synchronize the capacity tests with the monitor
`wait_until_cache_ready()` does not wait for the monitor's first
`check_need_evict_cache_in_advance()`, and that function reads plain
`_capacity`/`_cur_cache_size` without `_mutex`. If
`enable_evict_file_cache_in_advance` is left true by `evict_in_advance` (for
example under `--gtest_shuffle`), this read can overlap the
`reset_capacity(30)`/`_capacity = 30` writes below and trigger TSAN or
nondeterministic setup; increasing the interval does not order the first check.
Please disable/restore the feature for these tests or gate the first monitor
pass with a SyncPoint/latch.
--
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]
