github-actions[bot] commented on code in PR #65722:
URL: https://github.com/apache/doris/pull/65722#discussion_r3595599283
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2023,29 +2056,64 @@ 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);
+
_need_evict_cache_in_advance_metrics->set_value(_need_evict_cache_in_advance);
}
- 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;
+ };
+ bool previous_mode = _disk_resource_limit_mode;
+ if (is_memory_storage()) {
+ bool is_size_insufficient =
is_insufficient(cast_set<int>(size_percentage));
+ if (is_size_insufficient) {
+ _disk_resource_limit_mode = true;
Review 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.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1980,13 +1987,39 @@ int disk_used_percentage(const std::string& path,
std::pair<int, int>* percent)
return 0;
}
+namespace {
+
+const char* file_cache_storage_type_to_string(FileCacheStorageType type) {
+ switch (type) {
+ case FileCacheStorageType::DISK:
+ return "disk";
+ case FileCacheStorageType::MEMORY:
+ return "memory";
+ }
+ DORIS_CHECK(false) << "Unknown file cache storage type: " << type;
+ __builtin_unreachable();
+}
+
+} // namespace
+
+bool BlockFileCache::is_memory_storage() const {
+ return _storage->get_type() == FileCacheStorageType::MEMORY;
+}
+
+size_t
BlockFileCache::calc_size_percentage_unlocked(std::lock_guard<std::mutex>&)
const {
+ DORIS_CHECK(_capacity > 0);
+ return static_cast<size_t>(static_cast<double>(_cur_cache_size) /
+ static_cast<double>(_capacity) * 100);
+}
+
std::string BlockFileCache::reset_capacity(size_t new_capacity) {
Review 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.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2516,6 +2589,9 @@ size_t BlockFileCache::replay_lru_logs_once() {
}
void BlockFileCache::dump_lru_queues(bool force) {
+ if (is_memory_storage()) {
Review Comment:
[P2] Cover clear-time LRU deletion for memory storage
This guard covers explicit/background dumps, and initialization now covers
restore/replay, but `clear_file_cache_impl()` still unconditionally calls
`_lru_dumper->remove_lru_dump_files()` both before and after clearing. For a
memory cache that probes and can delete
`memory/lru_dump_{disposable,index,normal,ttl}.tail`, so the stated
no-disk-persistence contract is still violated through HTTP/factory CLEAR.
Please make the persistence helper storage-aware (or guard both clear calls)
and add a memory-clear test that proves sentinel LRU files are not touched.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2023,29 +2056,64 @@ 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);
Review Comment:
[P2] Recompute memory pressure modes after capacity growth
These metrics merely republish the pre-reset flags. For example, a 95/100
MiB memory cache has both modes enabled; growing it to 1 GiB computes about 9%
at line 2063 but leaves both flags true until the next 5-second monitor tick.
During that window request reservations still demand five-times eviction and
the 1-second advance worker can keep discarding valid blocks despite ample free
capacity. Recompute both memory-mode states from the post-reset percentage
under the synchronization used for the flags (or wake and synchronize with the
monitor) before returning, and add a grow-from-pressured test.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -2023,29 +2056,64 @@ 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);
+
_need_evict_cache_in_advance_metrics->set_value(_need_evict_cache_in_advance);
}
- 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 <=
Review Comment:
[P2] Preserve the supported equal-threshold case
Changing this check from `<` to `<=` silently rewrites a previously accepted
equal threshold. Equality is coherent here: enter at `>= T`, remain enabled at
`T`, and exit below `T`; the existing `reset_range` test also deliberately sets
99/99. After this change that configuration becomes 88/80, so a cache at 90%
unexpectedly enters the five-times eviction path. Please keep equality valid
(or add an explicit validated compatibility change rather than mutating it in
the monitor) and cover the equal case.
##########
be/src/io/cache/block_file_cache.cpp:
##########
@@ -1998,14 +2031,14 @@ std::string BlockFileCache::reset_capacity(size_t
new_capacity) {
if (need_remove_size <= 0) {
break;
}
- need_remove_size -= entry_size;
- space_released += entry_size;
- queue_released += entry_size;
auto* cell = get_cell(entry_key, entry_offset, cache_lock);
if (!cell->releasable()) {
cell->file_block->set_deleting();
continue;
}
+ need_remove_size -= entry_size;
Review Comment:
[P2] Count held blocks only once during a shrink
The held block above is marked deleting, so normal `FileBlocksHolder`
destruction will remove it later, but this moved accounting no longer charges
that eventual deletion against `need_remove_size`. In the new test-shaped case
(40 MiB with one held 10 MiB block, shrink to 15 MiB), the loop removes all
three releasable blocks to reach 10 MiB immediately and then holder destruction
removes the marked block too, leaving 0 MiB. Avoid marking held blocks when
current releasable eviction already meets the target, or count marked eventual
deletions in a separately defined budget. The test should let the holder
destruct normally and assert the eventual size.
##########
be/test/io/cache/block_file_cache_mem_storage_mode_test.cpp:
##########
@@ -0,0 +1,400 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <functional>
+
+#include "io/cache/block_file_cache_test_common.h"
+
+namespace doris::io {
+
+class BlockFileCacheMemModeTest : public BlockFileCacheTest {
+public:
+ void SetUp() override {
+ const auto* test_info =
::testing::UnitTest::GetInstance()->current_test_info();
+ _cache_path = caches_dir / test_info->name() / "";
+ fs::remove_all(_cache_path);
+ fs::create_directories(_cache_path);
+
+ _origin_enter_need_evict =
config::file_cache_enter_need_evict_cache_in_advance_percent;
+ _origin_exit_need_evict =
config::file_cache_exit_need_evict_cache_in_advance_percent;
+ _origin_enter_disk_limit =
config::file_cache_enter_disk_resource_limit_mode_percent;
+ _origin_exit_disk_limit =
config::file_cache_exit_disk_resource_limit_mode_percent;
+ _origin_enable_evict_in_advance =
config::enable_evict_file_cache_in_advance;
+ _origin_lru_dump_tail_record_num =
config::file_cache_background_lru_dump_tail_record_num;
+ }
+
+ void TearDown() override {
+ config::file_cache_enter_need_evict_cache_in_advance_percent =
_origin_enter_need_evict;
+ config::file_cache_exit_need_evict_cache_in_advance_percent =
_origin_exit_need_evict;
+ config::file_cache_enter_disk_resource_limit_mode_percent =
_origin_enter_disk_limit;
+ config::file_cache_exit_disk_resource_limit_mode_percent =
_origin_exit_disk_limit;
+ config::enable_evict_file_cache_in_advance =
_origin_enable_evict_in_advance;
+ config::file_cache_background_lru_dump_tail_record_num =
_origin_lru_dump_tail_record_num;
+
+ auto* sync_point = SyncPoint::get_instance();
+ sync_point->disable_processing();
+ sync_point->clear_all_call_backs();
+
+ fs::remove_all(_cache_path);
+ }
+
+protected:
+ FileCacheSettings make_settings(const std::string& storage = "memory")
const {
+ FileCacheSettings settings;
+ settings.storage = storage;
+ settings.capacity = 100_mb;
+ settings.max_file_block_size = 10_mb;
+ settings.max_query_cache_size = 100_mb;
+ settings.query_queue_size = 100_mb;
+ settings.query_queue_elements = 16;
+ settings.index_queue_size = 30_mb;
+ settings.index_queue_elements = 8;
+ settings.disposable_queue_size = 30_mb;
+ settings.disposable_queue_elements = 8;
+ settings.ttl_queue_size = 30_mb;
+ settings.ttl_queue_elements = 8;
+ return settings;
+ }
+
+ void wait_async_open(BlockFileCache& cache) const {
+ for (int i = 0; i < 100; ++i) {
+ if (cache.get_async_open_success()) {
+ return;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
+ FAIL() << "cache async open did not finish";
+ }
+
+ bool wait_until(const std::function<bool()>& predicate, int64_t timeout_ms
= 2000) const {
+ auto deadline = std::chrono::steady_clock::now() +
std::chrono::milliseconds(timeout_ms);
+ while (std::chrono::steady_clock::now() < deadline) {
+ if (predicate()) {
+ return true;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(5));
+ }
+ return predicate();
+ }
+
+ void fill_memory_cache(BlockFileCache& cache, size_t total_size) const {
+ TUniqueId query_id;
+ query_id.hi = 1;
+ query_id.lo = 1;
+ CacheContext context;
+ ReadStatistics rstats;
+ context.stats = &rstats;
+ context.cache_type = FileCacheType::NORMAL;
+ context.query_id = query_id;
+ auto key = BlockFileCache::hash("mem-mode-key");
+
+ for (size_t offset = 0; offset < total_size; offset += 10_mb) {
+ auto holder = cache.get_or_set(key, offset, 10_mb, context);
+ auto blocks = fromHolder(holder);
+ ASSERT_EQ(blocks.size(), 1);
+ ASSERT_TRUE(blocks[0]->get_or_set_downloader() ==
FileBlock::get_caller_id());
+ download_into_memory(blocks[0]);
+ }
+ }
+
+ void fill_cache(BlockFileCache& cache, const UInt128Wrapper& key, size_t
total_size,
+ bool use_memory_download) const {
+ TUniqueId query_id;
+ query_id.hi = 11;
+ query_id.lo = 11;
+ CacheContext context;
+ ReadStatistics rstats;
+ context.stats = &rstats;
+ context.cache_type = FileCacheType::NORMAL;
+ context.query_id = query_id;
+
+ for (size_t offset = 0; offset < total_size; offset += 10_mb) {
+ auto holder = cache.get_or_set(key, offset, 10_mb, context);
+ auto blocks = fromHolder(holder);
+ ASSERT_EQ(blocks.size(), 1);
+ ASSERT_TRUE(blocks[0]->get_or_set_downloader() ==
FileBlock::get_caller_id());
+ if (use_memory_download) {
+ download_into_memory(blocks[0]);
+ } else {
+ download(blocks[0]);
+ }
+ }
+ }
+
+ fs::path _cache_path;
+ int64_t _origin_enter_need_evict = 0;
+ int64_t _origin_exit_need_evict = 0;
+ int64_t _origin_enter_disk_limit = 0;
+ int64_t _origin_exit_disk_limit = 0;
+ bool _origin_enable_evict_in_advance = false;
+ int64_t _origin_lru_dump_tail_record_num = 0;
+};
+
+TEST_F(BlockFileCacheMemModeTest, check_need_evict_memory_enter_exit) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_need_evict_cache_in_advance_percent = 70;
+ config::file_cache_exit_need_evict_cache_in_advance_percent = 65;
+
+ cache._cur_cache_size = 80_mb;
+ cache.check_need_evict_cache_in_advance();
+ ASSERT_TRUE(cache._need_evict_cache_in_advance);
+
+ cache._cur_cache_size = 60_mb;
+ cache.check_need_evict_cache_in_advance();
+ ASSERT_FALSE(cache._need_evict_cache_in_advance);
+}
+
+TEST_F(BlockFileCacheMemModeTest, check_need_evict_memory_config_fallback) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_need_evict_cache_in_advance_percent = 70;
+ config::file_cache_exit_need_evict_cache_in_advance_percent = 75;
+
+ cache._cur_cache_size = 80_mb;
+ cache.check_need_evict_cache_in_advance();
+
+ ASSERT_EQ(config::file_cache_enter_need_evict_cache_in_advance_percent,
78);
+ ASSERT_EQ(config::file_cache_exit_need_evict_cache_in_advance_percent, 75);
+ ASSERT_TRUE(cache._need_evict_cache_in_advance);
+}
+
+TEST_F(BlockFileCacheMemModeTest, check_disk_mode_memory_enter_exit) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+
+ cache._cur_cache_size = 95_mb;
+ cache.check_disk_resource_limit();
+ ASSERT_TRUE(cache._disk_resource_limit_mode);
+
+ cache._cur_cache_size = 70_mb;
+ cache.check_disk_resource_limit();
+ ASSERT_FALSE(cache._disk_resource_limit_mode);
+}
+
+TEST_F(BlockFileCacheMemModeTest, check_disk_mode_disk_compat) {
+ auto settings = make_settings("disk");
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+
+ auto* sync_point = SyncPoint::get_instance();
+ sync_point->set_call_back("BlockFileCache::disk_used_percentage:1",
+ [](std::vector<std::any>&& values) {
+ auto* percent = try_any_cast<std::pair<int,
int>*>(values.back());
+ percent->first = 95;
+ percent->second = 70;
+ });
+ sync_point->enable_processing();
+ cache.check_disk_resource_limit();
+ ASSERT_TRUE(cache._disk_resource_limit_mode);
+
+ sync_point->clear_all_call_backs();
+ sync_point->set_call_back("BlockFileCache::disk_used_percentage:1",
+ [](std::vector<std::any>&& values) {
+ auto* percent = try_any_cast<std::pair<int,
int>*>(values.back());
+ percent->first = 70;
+ percent->second = 70;
+ });
+ cache.check_disk_resource_limit();
+ ASSERT_FALSE(cache._disk_resource_limit_mode);
+}
+
+TEST_F(BlockFileCacheMemModeTest,
reset_capacity_memory_should_work_and_mode_converge) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ fill_memory_cache(cache, 60_mb);
+ ASSERT_EQ(cache._cur_cache_size, 60_mb);
+
+ cache._disk_resource_limit_mode = false;
+ cache._disk_limit_mode_metrics->set_value(0);
+
+ auto message = cache.reset_capacity(30_mb);
+ ASSERT_FALSE(message.empty());
+ ASSERT_LE(cache._cur_cache_size, 30_mb);
+ ASSERT_FALSE(cache._disk_resource_limit_mode);
+
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+ cache.check_disk_resource_limit();
+ ASSERT_TRUE(cache._disk_resource_limit_mode);
+}
+
+TEST_F(BlockFileCacheMemModeTest,
reset_capacity_memory_should_not_overcount_unreleasable_blocks) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ TUniqueId query_id;
+ query_id.hi = 7;
+ query_id.lo = 7;
+ CacheContext context;
+ ReadStatistics rstats;
+ context.stats = &rstats;
+ context.cache_type = FileCacheType::NORMAL;
+ context.query_id = query_id;
+ auto key = BlockFileCache::hash("mem-held-key");
+
+ auto holder = cache.get_or_set(key, 0, 10_mb, context);
+ auto blocks = fromHolder(holder);
+ ASSERT_EQ(blocks.size(), 1);
+ ASSERT_TRUE(blocks[0]->get_or_set_downloader() ==
FileBlock::get_caller_id());
+ download_into_memory(blocks[0]);
+
+ fill_memory_cache(cache, 30_mb);
+ ASSERT_EQ(cache._cur_cache_size, 40_mb);
+
+ auto message = cache.reset_capacity(15_mb);
+ ASSERT_FALSE(message.empty());
+ ASSERT_LE(cache._cur_cache_size, 15_mb);
+ ASSERT_EQ(cache._cur_cache_size, 10_mb);
+
+ blocks.clear();
+ holder.file_blocks.clear();
+}
+
+TEST_F(BlockFileCacheMemModeTest, run_background_monitor_memory_mode_flip) {
+ auto settings = make_settings();
+ config::enable_evict_file_cache_in_advance = true;
+ config::file_cache_enter_need_evict_cache_in_advance_percent = 70;
+ config::file_cache_exit_need_evict_cache_in_advance_percent = 65;
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+
+ auto* sync_point = SyncPoint::get_instance();
+ sync_point->set_call_back("BlockFileCache::set_sleep_time",
+ [](auto&& args) {
*try_any_cast<int64_t*>(args[0]) = 10; });
+ sync_point->enable_processing();
+
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ cache._cur_cache_size = 95_mb;
+ ASSERT_TRUE(wait_until([&cache] {
+ return cache._need_evict_cache_in_advance &&
cache._disk_resource_limit_mode;
+ }));
+
+ cache._cur_cache_size = 60_mb;
+ ASSERT_TRUE(wait_until([&cache] {
+ return !cache._need_evict_cache_in_advance &&
!cache._disk_resource_limit_mode;
+ }));
+}
+
+TEST_F(BlockFileCacheMemModeTest,
stats_memory_contains_storage_type_and_size_percentage) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ cache._cur_cache_size = 25_mb;
+ cache._cur_cache_size_metrics->set_value(cache._cur_cache_size);
+
+ auto stats = cache.get_stats();
+ ASSERT_TRUE(stats.contains("size_percentage"));
+ ASSERT_TRUE(stats.contains("storage_type"));
+ ASSERT_EQ(stats["size_percentage"], 25);
+ ASSERT_EQ(stats["storage_type"], FileCacheStorageType::MEMORY);
+}
+
+TEST_F(BlockFileCacheMemModeTest,
memory_storage_should_skip_lru_restore_and_dump) {
+ config::file_cache_background_lru_dump_tail_record_num = 100;
+ auto settings = make_settings();
+ auto key = BlockFileCache::hash("memory-lru-restore-key");
+ auto memory_dump_dir = fs::current_path() / "memory";
+ fs::remove_all(memory_dump_dir);
+ fs::create_directories(memory_dump_dir);
+
+ {
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ fill_cache(cache, key, 20_mb, true);
+ ASSERT_EQ(cache._cur_cache_size, 20_mb);
+ cache.dump_lru_queues(true);
+ }
+
+ auto dump_file = memory_dump_dir / "lru_dump_normal.tail";
+ ASSERT_FALSE(fs::exists(dump_file));
+
+ std::ofstream(dump_file, std::ios::binary) << "stale-memory-dump";
+ ASSERT_TRUE(fs::exists(dump_file));
+
+ BlockFileCache cache2(_cache_path.string(), settings);
+ ASSERT_TRUE(cache2.initialize().ok());
+ wait_async_open(cache2);
+
+ ASSERT_EQ(cache2._cur_cache_size, 0);
+ ASSERT_EQ(cache2._normal_queue.get_elements_num_unsafe(), 0);
+ ASSERT_TRUE(cache2.get_blocks_by_key(key).empty());
+
+ fs::remove_all(memory_dump_dir);
+}
+
+TEST_F(BlockFileCacheMemModeTest,
memory_storage_should_skip_lru_queue_recorder) {
+ config::file_cache_background_lru_dump_tail_record_num = 100;
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ auto key = BlockFileCache::hash("memory-storage-skip-lru-recorder");
+ fill_cache(cache, key, 10_mb, true);
+
+
EXPECT_EQ(cache._lru_recorder->get_lru_queue_update_cnt_from_last_dump(FileCacheType::NORMAL),
+ 0);
+}
+
+TEST_F(BlockFileCacheMemModeTest, disk_storage_should_keep_lru_restore) {
+ config::file_cache_background_lru_dump_tail_record_num = 100;
+ auto settings = make_settings("disk");
+ auto key = BlockFileCache::hash("disk-lru-restore-key");
+ auto origin_cache_base_path = cache_base_path;
+ cache_base_path = _cache_path.string();
+
+ {
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ fill_cache(cache, key, 10_mb, false);
+ cache.dump_lru_queues(true);
Review Comment:
[P2] Replay the LRU event before forcing the dump
`fill_cache()` only enqueues the ADD in `LRUQueueRecorder`; the dump
snapshots the recorder's shadow queue, which is populated asynchronously by
`replay_queue_event()`. `force=true` bypasses the update-count threshold but
does not drain that queue. If the 1 ms replay thread has not run yet, this
writes an empty dump and the restore assertions fail even though persistence is
correct. Please deterministically call/assert `replay_lru_logs_once()` (as the
existing LRU tests do), or wait on an explicitly synchronized shadow-queue
condition before dumping.
##########
be/test/io/cache/block_file_cache_mem_storage_mode_test.cpp:
##########
@@ -0,0 +1,400 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <functional>
+
+#include "io/cache/block_file_cache_test_common.h"
+
+namespace doris::io {
+
+class BlockFileCacheMemModeTest : public BlockFileCacheTest {
+public:
+ void SetUp() override {
+ const auto* test_info =
::testing::UnitTest::GetInstance()->current_test_info();
+ _cache_path = caches_dir / test_info->name() / "";
+ fs::remove_all(_cache_path);
+ fs::create_directories(_cache_path);
+
+ _origin_enter_need_evict =
config::file_cache_enter_need_evict_cache_in_advance_percent;
+ _origin_exit_need_evict =
config::file_cache_exit_need_evict_cache_in_advance_percent;
+ _origin_enter_disk_limit =
config::file_cache_enter_disk_resource_limit_mode_percent;
+ _origin_exit_disk_limit =
config::file_cache_exit_disk_resource_limit_mode_percent;
+ _origin_enable_evict_in_advance =
config::enable_evict_file_cache_in_advance;
+ _origin_lru_dump_tail_record_num =
config::file_cache_background_lru_dump_tail_record_num;
+ }
+
+ void TearDown() override {
+ config::file_cache_enter_need_evict_cache_in_advance_percent =
_origin_enter_need_evict;
+ config::file_cache_exit_need_evict_cache_in_advance_percent =
_origin_exit_need_evict;
+ config::file_cache_enter_disk_resource_limit_mode_percent =
_origin_enter_disk_limit;
+ config::file_cache_exit_disk_resource_limit_mode_percent =
_origin_exit_disk_limit;
+ config::enable_evict_file_cache_in_advance =
_origin_enable_evict_in_advance;
+ config::file_cache_background_lru_dump_tail_record_num =
_origin_lru_dump_tail_record_num;
+
+ auto* sync_point = SyncPoint::get_instance();
+ sync_point->disable_processing();
+ sync_point->clear_all_call_backs();
+
+ fs::remove_all(_cache_path);
+ }
+
+protected:
+ FileCacheSettings make_settings(const std::string& storage = "memory")
const {
+ FileCacheSettings settings;
+ settings.storage = storage;
+ settings.capacity = 100_mb;
+ settings.max_file_block_size = 10_mb;
+ settings.max_query_cache_size = 100_mb;
+ settings.query_queue_size = 100_mb;
+ settings.query_queue_elements = 16;
+ settings.index_queue_size = 30_mb;
+ settings.index_queue_elements = 8;
+ settings.disposable_queue_size = 30_mb;
+ settings.disposable_queue_elements = 8;
+ settings.ttl_queue_size = 30_mb;
+ settings.ttl_queue_elements = 8;
+ return settings;
+ }
+
+ void wait_async_open(BlockFileCache& cache) const {
+ for (int i = 0; i < 100; ++i) {
+ if (cache.get_async_open_success()) {
+ return;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ }
+ FAIL() << "cache async open did not finish";
+ }
+
+ bool wait_until(const std::function<bool()>& predicate, int64_t timeout_ms
= 2000) const {
+ auto deadline = std::chrono::steady_clock::now() +
std::chrono::milliseconds(timeout_ms);
+ while (std::chrono::steady_clock::now() < deadline) {
+ if (predicate()) {
+ return true;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(5));
+ }
+ return predicate();
+ }
+
+ void fill_memory_cache(BlockFileCache& cache, size_t total_size) const {
+ TUniqueId query_id;
+ query_id.hi = 1;
+ query_id.lo = 1;
+ CacheContext context;
+ ReadStatistics rstats;
+ context.stats = &rstats;
+ context.cache_type = FileCacheType::NORMAL;
+ context.query_id = query_id;
+ auto key = BlockFileCache::hash("mem-mode-key");
+
+ for (size_t offset = 0; offset < total_size; offset += 10_mb) {
+ auto holder = cache.get_or_set(key, offset, 10_mb, context);
+ auto blocks = fromHolder(holder);
+ ASSERT_EQ(blocks.size(), 1);
+ ASSERT_TRUE(blocks[0]->get_or_set_downloader() ==
FileBlock::get_caller_id());
+ download_into_memory(blocks[0]);
+ }
+ }
+
+ void fill_cache(BlockFileCache& cache, const UInt128Wrapper& key, size_t
total_size,
+ bool use_memory_download) const {
+ TUniqueId query_id;
+ query_id.hi = 11;
+ query_id.lo = 11;
+ CacheContext context;
+ ReadStatistics rstats;
+ context.stats = &rstats;
+ context.cache_type = FileCacheType::NORMAL;
+ context.query_id = query_id;
+
+ for (size_t offset = 0; offset < total_size; offset += 10_mb) {
+ auto holder = cache.get_or_set(key, offset, 10_mb, context);
+ auto blocks = fromHolder(holder);
+ ASSERT_EQ(blocks.size(), 1);
+ ASSERT_TRUE(blocks[0]->get_or_set_downloader() ==
FileBlock::get_caller_id());
+ if (use_memory_download) {
+ download_into_memory(blocks[0]);
+ } else {
+ download(blocks[0]);
+ }
+ }
+ }
+
+ fs::path _cache_path;
+ int64_t _origin_enter_need_evict = 0;
+ int64_t _origin_exit_need_evict = 0;
+ int64_t _origin_enter_disk_limit = 0;
+ int64_t _origin_exit_disk_limit = 0;
+ bool _origin_enable_evict_in_advance = false;
+ int64_t _origin_lru_dump_tail_record_num = 0;
+};
+
+TEST_F(BlockFileCacheMemModeTest, check_need_evict_memory_enter_exit) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_need_evict_cache_in_advance_percent = 70;
+ config::file_cache_exit_need_evict_cache_in_advance_percent = 65;
+
+ cache._cur_cache_size = 80_mb;
+ cache.check_need_evict_cache_in_advance();
+ ASSERT_TRUE(cache._need_evict_cache_in_advance);
+
+ cache._cur_cache_size = 60_mb;
+ cache.check_need_evict_cache_in_advance();
+ ASSERT_FALSE(cache._need_evict_cache_in_advance);
+}
+
+TEST_F(BlockFileCacheMemModeTest, check_need_evict_memory_config_fallback) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_need_evict_cache_in_advance_percent = 70;
+ config::file_cache_exit_need_evict_cache_in_advance_percent = 75;
+
+ cache._cur_cache_size = 80_mb;
+ cache.check_need_evict_cache_in_advance();
+
+ ASSERT_EQ(config::file_cache_enter_need_evict_cache_in_advance_percent,
78);
+ ASSERT_EQ(config::file_cache_exit_need_evict_cache_in_advance_percent, 75);
+ ASSERT_TRUE(cache._need_evict_cache_in_advance);
+}
+
+TEST_F(BlockFileCacheMemModeTest, check_disk_mode_memory_enter_exit) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+
+ cache._cur_cache_size = 95_mb;
+ cache.check_disk_resource_limit();
+ ASSERT_TRUE(cache._disk_resource_limit_mode);
+
+ cache._cur_cache_size = 70_mb;
+ cache.check_disk_resource_limit();
+ ASSERT_FALSE(cache._disk_resource_limit_mode);
+}
+
+TEST_F(BlockFileCacheMemModeTest, check_disk_mode_disk_compat) {
+ auto settings = make_settings("disk");
+ BlockFileCache cache(_cache_path.string(), settings);
+
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+
+ auto* sync_point = SyncPoint::get_instance();
+ sync_point->set_call_back("BlockFileCache::disk_used_percentage:1",
+ [](std::vector<std::any>&& values) {
+ auto* percent = try_any_cast<std::pair<int,
int>*>(values.back());
+ percent->first = 95;
+ percent->second = 70;
+ });
+ sync_point->enable_processing();
+ cache.check_disk_resource_limit();
+ ASSERT_TRUE(cache._disk_resource_limit_mode);
+
+ sync_point->clear_all_call_backs();
+ sync_point->set_call_back("BlockFileCache::disk_used_percentage:1",
+ [](std::vector<std::any>&& values) {
+ auto* percent = try_any_cast<std::pair<int,
int>*>(values.back());
+ percent->first = 70;
+ percent->second = 70;
+ });
+ cache.check_disk_resource_limit();
+ ASSERT_FALSE(cache._disk_resource_limit_mode);
+}
+
+TEST_F(BlockFileCacheMemModeTest,
reset_capacity_memory_should_work_and_mode_converge) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ fill_memory_cache(cache, 60_mb);
+ ASSERT_EQ(cache._cur_cache_size, 60_mb);
+
+ cache._disk_resource_limit_mode = false;
+ cache._disk_limit_mode_metrics->set_value(0);
+
+ auto message = cache.reset_capacity(30_mb);
+ ASSERT_FALSE(message.empty());
+ ASSERT_LE(cache._cur_cache_size, 30_mb);
+ ASSERT_FALSE(cache._disk_resource_limit_mode);
+
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+ cache.check_disk_resource_limit();
+ ASSERT_TRUE(cache._disk_resource_limit_mode);
+}
+
+TEST_F(BlockFileCacheMemModeTest,
reset_capacity_memory_should_not_overcount_unreleasable_blocks) {
+ auto settings = make_settings();
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ TUniqueId query_id;
+ query_id.hi = 7;
+ query_id.lo = 7;
+ CacheContext context;
+ ReadStatistics rstats;
+ context.stats = &rstats;
+ context.cache_type = FileCacheType::NORMAL;
+ context.query_id = query_id;
+ auto key = BlockFileCache::hash("mem-held-key");
+
+ auto holder = cache.get_or_set(key, 0, 10_mb, context);
+ auto blocks = fromHolder(holder);
+ ASSERT_EQ(blocks.size(), 1);
+ ASSERT_TRUE(blocks[0]->get_or_set_downloader() ==
FileBlock::get_caller_id());
+ download_into_memory(blocks[0]);
+
+ fill_memory_cache(cache, 30_mb);
+ ASSERT_EQ(cache._cur_cache_size, 40_mb);
+
+ auto message = cache.reset_capacity(15_mb);
+ ASSERT_FALSE(message.empty());
+ ASSERT_LE(cache._cur_cache_size, 15_mb);
+ ASSERT_EQ(cache._cur_cache_size, 10_mb);
+
+ blocks.clear();
+ holder.file_blocks.clear();
+}
+
+TEST_F(BlockFileCacheMemModeTest, run_background_monitor_memory_mode_flip) {
+ auto settings = make_settings();
+ config::enable_evict_file_cache_in_advance = true;
+ config::file_cache_enter_need_evict_cache_in_advance_percent = 70;
+ config::file_cache_exit_need_evict_cache_in_advance_percent = 65;
+ config::file_cache_enter_disk_resource_limit_mode_percent = 90;
+ config::file_cache_exit_disk_resource_limit_mode_percent = 80;
+
+ auto* sync_point = SyncPoint::get_instance();
+ sync_point->set_call_back("BlockFileCache::set_sleep_time",
+ [](auto&& args) {
*try_any_cast<int64_t*>(args[0]) = 10; });
+ sync_point->enable_processing();
+
+ BlockFileCache cache(_cache_path.string(), settings);
+ ASSERT_TRUE(cache.initialize().ok());
+ wait_async_open(cache);
+
+ cache._cur_cache_size = 95_mb;
Review Comment:
[P2] Remove data races from the monitor test
After `initialize()` the monitor reads `_cur_cache_size` while holding
`_mutex`, but this test writes that field without the mutex; the polling
lambdas also read the two flags while the monitor writes them. Shortening the
sleep interval does not create a happens-before edge, so this test is undefined
under TSAN and its pass result does not prove the transition. Please update the
size under the cache lock and observe the transition through a latch/sync point
or synchronized accessor after fixing the production flags.
--
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]
