From 5a822fe6f65c0729077d891815bf0e6f07f19d3b Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Tue, 15 Sep 2026 00:58:20 +0800 Subject: [PATCH 1/7] [fix](filecache) convert blocks back to TTL when an expired TTL is extended The TTL manager decided whether to promote a tablet's cached blocks into the TTL queue from `was_zero_ttl`, which is true only when `_ttl_info_map` holds no entry for the tablet. Since the entry is erased only when the tablet's TTL is zero, that flag really answers "has this BE ever seen a non-zero TTL here", not "were the blocks outside the TTL queue". The expiration check demotes blocks back to NORMAL but leaves the non-zero TTL in the map. So once a TTL expired, the promotion path was closed for that tablet for good: extending the expired TTL to a value that has not expired updated FE, Meta Service and `_ttl_info_map`, yet the blocks stayed in the normal queue and no later round ever brought them back. They were then evicted under the normal policy while `SHOW CREATE TABLE` and `information_schema.file_cache_info` disagreed about whether TTL applied. Replace the inference with a recorded state. `TtlInfo` now carries `blocks_are_ttl`, the cache type last applied to the tablet's blocks, and `is_ttl_active()` derives the type the blocks should have. Conversions run only on a transition between the two, in both directions. Both background threads now funnel through `reconcile_tablet_blocks()`, which is serialized per tablet by a striped lock and re-reads the tablet's state after taking it. The expiration check previously decided from a snapshot of `_ttl_info_map` that could be a full gc interval old, so it could demote blocks the update thread had just promoted under a newly extended TTL, and neither thread would touch them again. Its snapshot is now only a candidate list. Two problems fall out of the same change. The block scan no longer runs while `_ttl_info_mutex` is held - it walks the meta store and takes the cache lock once per block, which on a tablet with a few thousand cached blocks blocked every other user of the map for the duration. And an expired tablet is now scanned once rather than on every gc round, because `blocks_are_ttl` records that the demotion already happened. The decision is a state comparison, not a comparison of TTL values: a tablet whose TTL is rewritten to another still-valid value keeps its blocks where they are. Deployments that rewrite this property on a schedule would otherwise rescan the whole tablet every time. Tests: - an expired TTL extended to a valid one promotes the blocks back - an extended TTL that is still in the past leaves them alone - a TTL extended while a demotion scan is in flight ends up as TTL - rewriting a valid TTL to another valid value triggers no rescan --- be/src/io/cache/block_file_cache_ttl_mgr.cpp | 149 ++++++++++------ be/src/io/cache/block_file_cache_ttl_mgr.h | 38 +++- .../cache/block_file_cache_ttl_mgr_test.cpp | 166 ++++++++++++++++++ 3 files changed, 299 insertions(+), 54 deletions(-) diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.cpp b/be/src/io/cache/block_file_cache_ttl_mgr.cpp index b394591d42decb..67590bac074cbf 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.cpp +++ b/be/src/io/cache/block_file_cache_ttl_mgr.cpp @@ -168,6 +168,61 @@ FileBlocks BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id(int64_t tablet_i return result; } +void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id, bool force_demote_scan) { + // Serialize all conversions of this tablet. Whichever caller takes this lock last re-reads + // the current state below and has the final say, so the update and expiration threads cannot + // fight over the same blocks and strand them in the loser's cache type. + std::lock_guard transition_lock(transition_lock_for(tablet_id)); + + bool want_ttl = false; + bool blocks_are_ttl = false; + { + // Deliberately re-read the map rather than trust what the caller saw: the expiration + // thread picks its candidates up to a full gc interval before getting here. + std::lock_guard lock(_ttl_info_mutex); + auto it = _ttl_info_map.find(tablet_id); + if (it != _ttl_info_map.end()) { + want_ttl = it->second.is_ttl_active(UnixSeconds()); + blocks_are_ttl = it->second.blocks_are_ttl; + } + } + + // Nothing to apply. Note this is a state comparison, not a comparison of TTL values: a + // tablet whose TTL is rewritten to another still-valid value keeps its blocks where they + // are, instead of rescanning the whole tablet every time the property is touched. + if (want_ttl == blocks_are_ttl && !(force_demote_scan && !want_ttl)) { + return; + } + + // Scan and convert outside _ttl_info_mutex: this walks the meta store and takes the cache + // lock once per block, which is far too long to hold a mutex the other thread needs. + const auto target_type = want_ttl ? FileCacheType::TTL : FileCacheType::NORMAL; + FileBlocks blocks = get_file_blocks_from_tablet_id(tablet_id); + for (auto& block : blocks) { + if (block->cache_type() == target_type) { + continue; + } + auto st = block->change_cache_type(target_type); + if (!st.ok()) { + LOG(WARNING) << "Failed to convert block to " << cache_type_to_string(target_type) + << " cache_type, tablet_id=" << tablet_id << ", err=" << st; + } + } + + { + std::lock_guard lock(_ttl_info_mutex); + auto it = _ttl_info_map.find(tablet_id); + if (it != _ttl_info_map.end()) { + it->second.blocks_are_ttl = want_ttl; + // A tablet with no TTL whose blocks are all back in the normal queue needs no + // further tracking, and dropping it keeps the expiration thread's scan short. + if (it->second.ttl == 0 && !want_ttl) { + _ttl_info_map.erase(it); + } + } + } +} + void BlockFileCacheTtlMgr::run_backgroud_update_ttl_info_map() { Thread::set_self_name("ttl_mgr_update"); @@ -220,47 +275,38 @@ void BlockFileCacheTtlMgr::run_backgroud_update_ttl_info_map() { } } - // Update TTL info map - bool need_convert_from_ttl = false; + // Record the TTL this tablet currently has, then let reconcile_tablet_blocks() + // decide whether that moves its blocks between the TTL and normal queues. + bool tracked = false; { std::lock_guard lock(_ttl_info_mutex); + auto it = _ttl_info_map.find(tablet_id); if (ttl > 0) { - auto old_info_it = _ttl_info_map.find(tablet_id); - bool was_zero_ttl = (old_info_it == _ttl_info_map.end() || - old_info_it->second.ttl == 0); - _ttl_info_map[tablet_id] = TtlInfo {ttl, tablet_ctime}; - - // If TTL changed from 0 to non-zero, convert blocks to TTL type - if (was_zero_ttl) { - FileBlocks blocks = get_file_blocks_from_tablet_id(tablet_id); - for (auto& block : blocks) { - if (block->cache_type() != FileCacheType::TTL) { - auto change_status = - block->change_cache_type(FileCacheType::TTL); - if (!change_status.ok()) { - LOG(WARNING) << "Failed to convert block to TTL cache_type"; - } - } - } + if (it == _ttl_info_map.end()) { + _ttl_info_map.emplace( + tablet_id, + TtlInfo {ttl, tablet_ctime, /*blocks_are_ttl=*/false}); + } else { + // Keep blocks_are_ttl: it describes the blocks, not the tablet meta. + it->second.ttl = ttl; + it->second.tablet_ctime = tablet_ctime; } - } else { - // Periodically reconcile blocks restored from persisted TTL metadata, - // because _ttl_info_map is rebuilt only in memory after restart. - need_convert_from_ttl = - _ttl_info_map.erase(tablet_id) > 0 || need_full_reconcile; + tracked = true; + } else if (it != _ttl_info_map.end()) { + // Hold on to the entry until the blocks are actually demoted. It carries + // the blocks_are_ttl bit reconcile_tablet_blocks() needs, and drops + // itself once the tablet has settled back to NORMAL. + it->second.ttl = 0; + tracked = true; } } - if (need_convert_from_ttl) { - FileBlocks blocks = get_file_blocks_from_tablet_id(tablet_id); - for (auto& block : blocks) { - if (block->cache_type() == FileCacheType::TTL) { - auto st = block->change_cache_type(FileCacheType::NORMAL); - if (!st.ok()) { - LOG(WARNING) << "Failed to convert block back to NORMAL cache_type"; - } - } - } + if (tracked) { + reconcile_tablet_blocks(tablet_id, /*force_demote_scan=*/false); + } else if (need_full_reconcile) { + // Periodically reconcile blocks restored from persisted TTL metadata, + // because _ttl_info_map is rebuilt only in memory after restart. + reconcile_tablet_blocks(tablet_id, /*force_demote_scan=*/true); } } @@ -279,29 +325,28 @@ void BlockFileCacheTtlMgr::run_backgroud_expiration_check() { while (!_stop_background.load(std::memory_order_acquire)) { try { - std::map ttl_info_copy; - - // Copy TTL info for processing + // Collect tablets whose TTL has run out while their blocks still sit in the TTL + // queue. Tablets already demoted are skipped, so an expired tablet is scanned once + // instead of once per gc round for as long as it stays in the map. + std::vector expired_tablet_ids; { std::lock_guard lock(_ttl_info_mutex); - ttl_info_copy = _ttl_info_map; + uint64_t current_time = UnixSeconds(); + for (const auto& [tablet_id, ttl_info] : _ttl_info_map) { + if (ttl_info.blocks_are_ttl && !ttl_info.is_ttl_active(current_time)) { + expired_tablet_ids.push_back(tablet_id); + } + } } - uint64_t current_time = UnixSeconds(); - - for (const auto& [tablet_id, ttl_info] : ttl_info_copy) { - if (ttl_info.tablet_ctime + ttl_info.ttl < current_time) { - // Tablet has expired, convert TTL blocks back to NORMAL type - FileBlocks blocks = get_file_blocks_from_tablet_id(tablet_id); - for (auto& block : blocks) { - if (block->cache_type() == FileCacheType::TTL) { - auto st = block->change_cache_type(FileCacheType::NORMAL); - if (!st.ok()) { - LOG(WARNING) << "Failed to convert block back to NORMAL cache_type"; - } - } - } + // This is only a candidate list. reconcile_tablet_blocks() re-reads the tablet's + // state under the per-tablet lock, so a TTL extended between the two never gets + // demoted on the strength of what was observed above. + for (int64_t tablet_id : expired_tablet_ids) { + if (_stop_background.load(std::memory_order_acquire)) { + break; } + reconcile_tablet_blocks(tablet_id, /*force_demote_scan=*/false); } std::this_thread::sleep_for( diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.h b/be/src/io/cache/block_file_cache_ttl_mgr.h index 8cd677446f174b..2b31a474ddabfc 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.h +++ b/be/src/io/cache/block_file_cache_ttl_mgr.h @@ -22,7 +22,9 @@ #include #include +#include #include +#include #include #include #include @@ -38,8 +40,24 @@ class BlockFileCache; class CacheBlockMetaStore; struct TtlInfo { - uint64_t ttl; - uint64_t tablet_ctime; + uint64_t ttl = 0; + uint64_t tablet_ctime = 0; + // Cache type last applied to this tablet's blocks. The manager converts blocks only on a + // state transition, so the applied state has to be remembered explicitly: whether an entry + // exists cannot tell "this tablet never had a TTL" apart from "its TTL already expired and + // the blocks were demoted", and those two need opposite handling when a new TTL arrives. + bool blocks_are_ttl = false; + + // Whether this tablet's blocks belong in the TTL queue right now. + bool is_ttl_active(uint64_t now) const { + if (ttl == 0 || tablet_ctime == 0) { + return false; + } + if (tablet_ctime > std::numeric_limits::max() - ttl) { + return false; + } + return tablet_ctime + ttl >= now; + } }; class BlockFileCacheTtlMgr { @@ -61,6 +79,17 @@ class BlockFileCacheTtlMgr { private: FileBlocks get_file_blocks_from_tablet_id(int64_t tablet_id); + // Drive this tablet's cached blocks to the cache type its current TTL state asks for. + // Both background threads funnel through here and it is serialized per tablet, so they can + // never scan the same tablet concurrently and leave the blocks in whatever type the scan + // that happened to finish last wrote. force_demote_scan additionally scans a tablet the map + // does not track, which is how TTL blocks restored from persisted metadata are cleaned up. + void reconcile_tablet_blocks(int64_t tablet_id, bool force_demote_scan); + + std::mutex& transition_lock_for(int64_t tablet_id) { + return _transition_locks[static_cast(tablet_id) % kTransitionLockStripes]; + } + private: // Tablet ids waiting to be deduplicated + set of unique ids known to have cached data moodycamel::ConcurrentQueue _tablet_id_queue; @@ -79,6 +108,11 @@ class BlockFileCacheTtlMgr { std::mutex _ttl_info_mutex; + // Striped locks serializing block conversions per tablet. Lock order is always + // _transition_locks[i] -> _ttl_info_mutex; _ttl_info_mutex is never held across a block scan. + static constexpr size_t kTransitionLockStripes = 64; + std::array _transition_locks; + std::shared_ptr> _tablet_id_set_size_metrics; }; diff --git a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp index 3aa00e67ab4b8c..4c5627886cb92d 100644 --- a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp +++ b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp @@ -462,4 +462,170 @@ TEST_F(BlockFileCacheTtlMgrTest, TabletTtlRemovedMovesBlocksBackToNormal) { std::chrono::seconds(5))); } + +TEST_F(BlockFileCacheTtlMgrTest, ExpiredTtlExtendedMovesBlocksBackToTtl) { + constexpr int64_t kTabletId = 6006; + auto tablet = std::make_shared(UnixSeconds(), 120); + fake_engine()->add_tablet(kTabletId, tablet); + + UInt128Wrapper hash; + auto block = create_block(kTabletId, "ttl-extend-after-expire", 0, 1024, &hash); + persist_block_meta(kTabletId, hash, block->range().left, block->range().size()); + + _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); + _ttl_mgr->register_tablet_id(kTabletId); + + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, + std::chrono::seconds(5))); + + // Let the TTL expire. The block goes back to NORMAL while the manager keeps a non-zero TTL + // recorded for the tablet, which is the state that used to wedge the promotion path. + tablet->set_creation_time(UnixSeconds() - 120); + tablet->set_ttl_seconds(1); + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::NORMAL; }, + std::chrono::seconds(5))); + + // Extending an already expired TTL to one that has not expired has to bring the block back. + tablet->set_ttl_seconds(30758400); + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, + std::chrono::seconds(5))); +} + +TEST_F(BlockFileCacheTtlMgrTest, ExtendedTtlThatIsStillExpiredKeepsBlocksNormal) { + constexpr int64_t kTabletId = 7007; + const int64_t creation_time = UnixSeconds() - 7200; + auto tablet = std::make_shared(creation_time, 60); + fake_engine()->add_tablet(kTabletId, tablet); + + UInt128Wrapper hash; + auto block = create_block(kTabletId, "ttl-extend-still-expired", 0, 1024, &hash); + persist_block_meta(kTabletId, hash, block->range().left, block->range().size()); + + _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); + _ttl_mgr->register_tablet_id(kTabletId); + + int64_t call_count = fake_engine()->get_tablet_meta_call_count(); + ASSERT_TRUE(wait_for_condition( + [&]() { return fake_engine()->get_tablet_meta_call_count() >= call_count + 2; }, + std::chrono::seconds(5))); + ASSERT_EQ(FileCacheType::NORMAL, block->cache_type()); + + // A longer TTL that is still in the past must not promote anything. + tablet->set_ttl_seconds(120); + call_count = fake_engine()->get_tablet_meta_call_count(); + ASSERT_TRUE(wait_for_condition( + [&]() { return fake_engine()->get_tablet_meta_call_count() >= call_count + 3; }, + std::chrono::seconds(5))); + EXPECT_EQ(FileCacheType::NORMAL, block->cache_type()); +} + +TEST_F(BlockFileCacheTtlMgrTest, RewritingTtlToAnotherValidValueDoesNotRescanBlocks) { + constexpr int64_t kTabletId = 8008; + auto tablet = std::make_shared(UnixSeconds(), 3600); + fake_engine()->add_tablet(kTabletId, tablet); + + UInt128Wrapper hash; + auto block = create_block(kTabletId, "ttl-rewrite-valid", 0, 1024, &hash); + persist_block_meta(kTabletId, hash, block->range().left, block->range().size()); + + _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); + _ttl_mgr->register_tablet_id(kTabletId); + + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, + std::chrono::seconds(5))); + + std::atomic block_scan_count {0}; + auto* sync_point = SyncPoint::get_instance(); + sync_point->clear_all_call_backs(); + sync_point->clear_trace(); + SyncPoint::CallbackGuard guard; + sync_point->set_call_back( + "BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id", + [&block_scan_count](std::vector&& args) { + if (doris::try_any_cast(args[0]) == kTabletId) { + block_scan_count.fetch_add(1, std::memory_order_relaxed); + } + }, + &guard); + sync_point->enable_processing(); + + // Automated jobs rewrite this property regularly. As long as the tablet stays in the same + // state, none of those rewrites may trigger another walk of the meta store. + for (int64_t ttl : {7200, 1800, 5400}) { + tablet->set_ttl_seconds(ttl); + int64_t call_count = fake_engine()->get_tablet_meta_call_count(); + ASSERT_TRUE(wait_for_condition( + [&]() { return fake_engine()->get_tablet_meta_call_count() >= call_count + 2; }, + std::chrono::seconds(5))); + } + + sync_point->disable_processing(); + sync_point->clear_trace(); + + EXPECT_EQ(0, block_scan_count.load(std::memory_order_relaxed)); + EXPECT_EQ(FileCacheType::TTL, block->cache_type()); +} + +TEST_F(BlockFileCacheTtlMgrTest, TtlExtensionWinsOverConcurrentExpirationScan) { + constexpr int64_t kTabletId = 9009; + auto tablet = std::make_shared(UnixSeconds(), 3600); + fake_engine()->add_tablet(kTabletId, tablet); + + UInt128Wrapper hash; + auto block = create_block(kTabletId, "ttl-extend-during-demote", 0, 1024, &hash); + persist_block_meta(kTabletId, hash, block->range().left, block->range().size()); + + _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); + _ttl_mgr->register_tablet_id(kTabletId); + + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, + std::chrono::seconds(5))); + + // Stall the demotion scan midway so the TTL can be extended underneath it, reproducing the + // window where the expiration check acts on a view of the tablet that is already stale. + std::atomic demote_scan_entered {false}; + std::atomic release_demote_scan {false}; + auto* sync_point = SyncPoint::get_instance(); + sync_point->clear_all_call_backs(); + sync_point->clear_trace(); + SyncPoint::CallbackGuard guard; + sync_point->set_call_back( + "BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id", + [&](std::vector&& args) { + if (doris::try_any_cast(args[0]) != kTabletId) { + return; + } + if (demote_scan_entered.exchange(true, std::memory_order_acq_rel)) { + return; + } + while (!release_demote_scan.load(std::memory_order_acquire)) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + }, + &guard); + sync_point->enable_processing(); + + tablet->set_creation_time(UnixSeconds() - 3600); + tablet->set_ttl_seconds(1); + + bool scan_stalled = wait_for_condition( + [&]() { return demote_scan_entered.load(std::memory_order_acquire); }, + std::chrono::seconds(10)); + + // Extend the TTL while the demotion is still in flight. + tablet->set_creation_time(UnixSeconds()); + tablet->set_ttl_seconds(30758400); + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + release_demote_scan.store(true, std::memory_order_release); + + bool ends_as_ttl = + wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, + std::chrono::seconds(10)); + sync_point->disable_processing(); + sync_point->clear_trace(); + + EXPECT_TRUE(scan_stalled); + EXPECT_TRUE(ends_as_ttl); +} + } // namespace doris::io From a37af20607639c6551c7b7f4d3825a1659fe7a32 Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Tue, 15 Sep 2026 11:56:43 +0800 Subject: [PATCH 2/7] apply clang-format --- be/src/io/cache/block_file_cache_ttl_mgr.cpp | 5 ++--- be/test/io/cache/block_file_cache_ttl_mgr_test.cpp | 6 ++---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.cpp b/be/src/io/cache/block_file_cache_ttl_mgr.cpp index 67590bac074cbf..8fb76fcf0f7744 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.cpp +++ b/be/src/io/cache/block_file_cache_ttl_mgr.cpp @@ -283,9 +283,8 @@ void BlockFileCacheTtlMgr::run_backgroud_update_ttl_info_map() { auto it = _ttl_info_map.find(tablet_id); if (ttl > 0) { if (it == _ttl_info_map.end()) { - _ttl_info_map.emplace( - tablet_id, - TtlInfo {ttl, tablet_ctime, /*blocks_are_ttl=*/false}); + _ttl_info_map.emplace(tablet_id, TtlInfo {ttl, tablet_ctime, + /*blocks_are_ttl=*/false}); } else { // Keep blocks_are_ttl: it describes the blocks, not the tablet meta. it->second.ttl = ttl; diff --git a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp index 4c5627886cb92d..3959ded1d8a631 100644 --- a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp +++ b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp @@ -462,7 +462,6 @@ TEST_F(BlockFileCacheTtlMgrTest, TabletTtlRemovedMovesBlocksBackToNormal) { std::chrono::seconds(5))); } - TEST_F(BlockFileCacheTtlMgrTest, ExpiredTtlExtendedMovesBlocksBackToTtl) { constexpr int64_t kTabletId = 6006; auto tablet = std::make_shared(UnixSeconds(), 120); @@ -618,9 +617,8 @@ TEST_F(BlockFileCacheTtlMgrTest, TtlExtensionWinsOverConcurrentExpirationScan) { std::this_thread::sleep_for(std::chrono::milliseconds(200)); release_demote_scan.store(true, std::memory_order_release); - bool ends_as_ttl = - wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, - std::chrono::seconds(10)); + bool ends_as_ttl = wait_for_condition( + [&]() { return block->cache_type() == FileCacheType::TTL; }, std::chrono::seconds(10)); sync_point->disable_processing(); sync_point->clear_trace(); From d8d9ead8495113c4a62895d7b83e68199c207f34 Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Tue, 15 Sep 2026 20:57:14 +0800 Subject: [PATCH 3/7] address review: keep sweeping expired tablets, treat unrecorded state as unknown Four defects in the previous revision, found in review. **Expired tablets have to be swept on every round.** Skipping a tablet whose blocks were already demoted looked like dead work, but that sweep carries a function. The read path types a block from `ttl_seconds` alone: `beta_rowset_reader.cpp` assigns that duration straight to `io_ctx.expiration_time`, documented as an absolute time, and `CacheContext` makes any non-zero value a TTL block. An expired tablet that is still being queried therefore keeps producing TTL blocks after its existing ones were demoted, and only the per-round rescan collects them. Left alone they accumulate in a queue that nothing else reclaims -- TTL blocks are not eviction candidates for other queues, while TTL writes do evict from them -- so the tablet slowly squeezes out the data the TTL quota exists to protect. The expiration path now passes `force_scan` to walk the blocks even when the recorded state already matches. **An unrecorded state is unknown, not "normal".** `_ttl_info_map` is rebuilt in memory while block types survive on disk, so a tablet whose TTL expired while the BE was down came back with TTL blocks and a fresh entry claiming they were normal; both threads then skipped it forever. `blocks_are_ttl` is now `std::optional`, unset until a conversion actually records one. Unknown never compares equal to the wanted state, so a tablet entering the map always gets one reconciling scan -- which is also what the periodic reconcile of untracked tablets needs, so the `force_demote_scan` special case folds into the same rule. **Stop tracking a settled tablet even when nothing was converted.** Dropping the TTL of an already-expired tablet leaves no conversion to do, so the erase after the transition was never reached: the entry stayed for the life of the process and the tablet never again qualified for the periodic reconcile. The check moved ahead of the early return. **A failed conversion must not record success.** `change_cache_type()` rewrites storage metadata and can fail. Recording the new state anyway left the tablet permanently mismatched, since every later round then saw state and wants agreeing. `blocks_are_ttl` is left alone unless every block converted, so the next round retries. Also adds a `file_cache_ttl_mgr_ttl_info_map_size` bvar and an INFO line naming the tablet, direction and block count on each conversion; the original bug was hard to place precisely because a skipped conversion logged nothing. Tests: TTL blocks restored from disk for an expired tablet get demoted; blocks cached after a demotion get demoted too; clearing the TTL of an expired tablet stops tracking it. --- be/src/io/cache/block_file_cache_ttl_mgr.cpp | 101 ++++++++++++------ be/src/io/cache/block_file_cache_ttl_mgr.h | 32 ++++-- .../cache/block_file_cache_ttl_mgr_test.cpp | 94 +++++++++++++++- 3 files changed, 188 insertions(+), 39 deletions(-) diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.cpp b/be/src/io/cache/block_file_cache_ttl_mgr.cpp index 8fb76fcf0f7744..b8701b68cddc7a 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.cpp +++ b/be/src/io/cache/block_file_cache_ttl_mgr.cpp @@ -41,6 +41,8 @@ BlockFileCacheTtlMgr::BlockFileCacheTtlMgr(BlockFileCache* mgr, CacheBlockMetaSt : _mgr(mgr), _meta_store(meta_store), _stop_background(false) { _tablet_id_set_size_metrics = std::make_shared>( _mgr->get_base_path().c_str(), "file_cache_ttl_mgr_tablet_id_set_size", 0); + _ttl_info_map_size_metrics = std::make_shared>( + _mgr->get_base_path().c_str(), "file_cache_ttl_mgr_ttl_info_map_size", 0); resume(); } @@ -85,6 +87,17 @@ void BlockFileCacheTtlMgr::register_tablet_id(int64_t tablet_id) { _tablet_id_queue.enqueue(tablet_id); } +void BlockFileCacheTtlMgr::update_ttl_info_map_size_metrics() { + if (_ttl_info_map_size_metrics) { + _ttl_info_map_size_metrics->set_value(_ttl_info_map.size()); + } +} + +size_t BlockFileCacheTtlMgr::tracked_tablet_num() { + std::lock_guard lock(_ttl_info_mutex); + return _ttl_info_map.size(); +} + void BlockFileCacheTtlMgr::run_background_tablet_id_flush() { Thread::set_self_name("ttl_mgr_flush"); @@ -168,29 +181,43 @@ FileBlocks BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id(int64_t tablet_i return result; } -void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id, bool force_demote_scan) { +void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id, bool force_scan) { // Serialize all conversions of this tablet. Whichever caller takes this lock last re-reads // the current state below and has the final say, so the update and expiration threads cannot // fight over the same blocks and strand them in the loser's cache type. std::lock_guard transition_lock(transition_lock_for(tablet_id)); bool want_ttl = false; - bool blocks_are_ttl = false; + // Unknown when the tablet is not tracked at all, which is also how a tablet looks right + // after a restart: the blocks on disk keep their type, _ttl_info_map does not. + std::optional blocks_are_ttl; + bool settled = false; { // Deliberately re-read the map rather than trust what the caller saw: the expiration // thread picks its candidates up to a full gc interval before getting here. std::lock_guard lock(_ttl_info_mutex); auto it = _ttl_info_map.find(tablet_id); if (it != _ttl_info_map.end()) { - want_ttl = it->second.is_ttl_active(UnixSeconds()); - blocks_are_ttl = it->second.blocks_are_ttl; + if (it->second.ttl == 0 && it->second.blocks_are_ttl == false) { + // No TTL left and the blocks are known to be normal: nothing to track. Dropped + // here rather than after a conversion, so that a tablet which settles without + // needing one still stops being tracked and goes back to the cheap path where + // it is only visited by the periodic reconcile. + _ttl_info_map.erase(it); + update_ttl_info_map_size_metrics(); + settled = true; + } else { + want_ttl = it->second.is_ttl_active(UnixSeconds()); + blocks_are_ttl = it->second.blocks_are_ttl; + } } } // Nothing to apply. Note this is a state comparison, not a comparison of TTL values: a // tablet whose TTL is rewritten to another still-valid value keeps its blocks where they - // are, instead of rescanning the whole tablet every time the property is touched. - if (want_ttl == blocks_are_ttl && !(force_demote_scan && !want_ttl)) { + // are, instead of rescanning the whole tablet every time the property is touched. An + // unknown applied state never matches, so it always costs one scan. + if (settled || (!force_scan && blocks_are_ttl == want_ttl)) { return; } @@ -198,27 +225,34 @@ void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id, bool force // lock once per block, which is far too long to hold a mutex the other thread needs. const auto target_type = want_ttl ? FileCacheType::TTL : FileCacheType::NORMAL; FileBlocks blocks = get_file_blocks_from_tablet_id(tablet_id); + size_t converted = 0; + bool all_converted = true; for (auto& block : blocks) { if (block->cache_type() == target_type) { continue; } auto st = block->change_cache_type(target_type); - if (!st.ok()) { + if (st.ok()) { + ++converted; + } else { + all_converted = false; LOG(WARNING) << "Failed to convert block to " << cache_type_to_string(target_type) << " cache_type, tablet_id=" << tablet_id << ", err=" << st; } } + if (converted > 0) { + LOG(INFO) << "converted cached blocks to " << cache_type_to_string(target_type) + << ", tablet_id=" << tablet_id << ", block_num=" << converted + << ", scanned=" << blocks.size(); + } { std::lock_guard lock(_ttl_info_mutex); auto it = _ttl_info_map.find(tablet_id); - if (it != _ttl_info_map.end()) { + if (it != _ttl_info_map.end() && all_converted) { + // Left alone when a block failed to convert, so that the mismatch is still visible + // to the next round and gets retried instead of being recorded as done. it->second.blocks_are_ttl = want_ttl; - // A tablet with no TTL whose blocks are all back in the normal queue needs no - // further tracking, and dropping it keeps the expiration thread's scan short. - if (it->second.ttl == 0 && !want_ttl) { - _ttl_info_map.erase(it); - } } } } @@ -258,7 +292,9 @@ void BlockFileCacheTtlMgr::run_backgroud_update_ttl_info_map() { } { std::lock_guard lock(_ttl_info_mutex); - _ttl_info_map.erase(tablet_id); + if (_ttl_info_map.erase(tablet_id) > 0) { + update_ttl_info_map_size_metrics(); + } } } else { LOG(WARNING) << "Failed to get tablet meta for tablet_id: " << tablet_id @@ -283,8 +319,10 @@ void BlockFileCacheTtlMgr::run_backgroud_update_ttl_info_map() { auto it = _ttl_info_map.find(tablet_id); if (ttl > 0) { if (it == _ttl_info_map.end()) { - _ttl_info_map.emplace(tablet_id, TtlInfo {ttl, tablet_ctime, - /*blocks_are_ttl=*/false}); + // blocks_are_ttl stays unknown: what the blocks already are is + // whatever a previous run of this BE left on disk. + _ttl_info_map.emplace(tablet_id, TtlInfo {ttl, tablet_ctime}); + update_ttl_info_map_size_metrics(); } else { // Keep blocks_are_ttl: it describes the blocks, not the tablet meta. it->second.ttl = ttl; @@ -300,12 +338,11 @@ void BlockFileCacheTtlMgr::run_backgroud_update_ttl_info_map() { } } - if (tracked) { - reconcile_tablet_blocks(tablet_id, /*force_demote_scan=*/false); - } else if (need_full_reconcile) { - // Periodically reconcile blocks restored from persisted TTL metadata, - // because _ttl_info_map is rebuilt only in memory after restart. - reconcile_tablet_blocks(tablet_id, /*force_demote_scan=*/true); + // An untracked tablet has no recorded state, so reconciling it scans once and + // demotes anything restored from persisted TTL metadata. Gated on the periodic + // round so that ordinary non-TTL tablets are not walked every time. + if (tracked || need_full_reconcile) { + reconcile_tablet_blocks(tablet_id, /*force_scan=*/false); } } @@ -324,28 +361,32 @@ void BlockFileCacheTtlMgr::run_backgroud_expiration_check() { while (!_stop_background.load(std::memory_order_acquire)) { try { - // Collect tablets whose TTL has run out while their blocks still sit in the TTL - // queue. Tablets already demoted are skipped, so an expired tablet is scanned once - // instead of once per gc round for as long as it stays in the map. + // Collect tablets whose TTL has run out. std::vector expired_tablet_ids; { std::lock_guard lock(_ttl_info_mutex); uint64_t current_time = UnixSeconds(); for (const auto& [tablet_id, ttl_info] : _ttl_info_map) { - if (ttl_info.blocks_are_ttl && !ttl_info.is_ttl_active(current_time)) { + if (ttl_info.ttl > 0 && !ttl_info.is_ttl_active(current_time)) { expired_tablet_ids.push_back(tablet_id); } } } - // This is only a candidate list. reconcile_tablet_blocks() re-reads the tablet's - // state under the per-tablet lock, so a TTL extended between the two never gets - // demoted on the strength of what was observed above. + // Swept on every round rather than once, because an expired tablet keeps producing + // TTL blocks for as long as it is read: the read path derives the cache type from + // ttl_seconds alone and never looks at whether the TTL has passed. Hence force_scan + // -- the tablet-level state says the blocks were demoted already and would + // otherwise skip the walk that collects the new ones. + // + // The list above is only a candidate set. reconcile_tablet_blocks() re-reads the + // tablet's state under the per-tablet lock, so a TTL extended in between is never + // demoted on the strength of what was observed here. for (int64_t tablet_id : expired_tablet_ids) { if (_stop_background.load(std::memory_order_acquire)) { break; } - reconcile_tablet_blocks(tablet_id, /*force_demote_scan=*/false); + reconcile_tablet_blocks(tablet_id, /*force_scan=*/true); } std::this_thread::sleep_for( diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.h b/be/src/io/cache/block_file_cache_ttl_mgr.h index 2b31a474ddabfc..879e244af1c0ec 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.h +++ b/be/src/io/cache/block_file_cache_ttl_mgr.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -42,11 +43,15 @@ class CacheBlockMetaStore; struct TtlInfo { uint64_t ttl = 0; uint64_t tablet_ctime = 0; - // Cache type last applied to this tablet's blocks. The manager converts blocks only on a - // state transition, so the applied state has to be remembered explicitly: whether an entry - // exists cannot tell "this tablet never had a TTL" apart from "its TTL already expired and - // the blocks were demoted", and those two need opposite handling when a new TTL arrives. - bool blocks_are_ttl = false; + // Cache type last applied to this tablet's blocks, or nullopt while the manager has not + // converted them yet and so cannot know: _ttl_info_map is rebuilt in memory only, while the + // types themselves survive a restart on disk. Unknown never compares equal to the wanted + // state, so a tablet entering the map always gets one reconciling scan. + // + // The applied state has to be remembered explicitly rather than inferred from the presence + // of the entry: that cannot tell "this tablet never had a TTL" apart from "its TTL expired + // and the blocks were demoted", and those two need opposite handling when a TTL arrives. + std::optional blocks_are_ttl; // Whether this tablet's blocks belong in the TTL queue right now. bool is_ttl_active(uint64_t now) const { @@ -66,6 +71,9 @@ class BlockFileCacheTtlMgr { ~BlockFileCacheTtlMgr(); void register_tablet_id(int64_t tablet_id); + // Number of tablets whose TTL state is currently tracked. Mirrors the + // file_cache_ttl_mgr_ttl_info_map_size bvar; entry leaks are otherwise invisible. + size_t tracked_tablet_num(); void stop(); void resume(); @@ -82,9 +90,16 @@ class BlockFileCacheTtlMgr { // Drive this tablet's cached blocks to the cache type its current TTL state asks for. // Both background threads funnel through here and it is serialized per tablet, so they can // never scan the same tablet concurrently and leave the blocks in whatever type the scan - // that happened to finish last wrote. force_demote_scan additionally scans a tablet the map - // does not track, which is how TTL blocks restored from persisted metadata are cleaned up. - void reconcile_tablet_blocks(int64_t tablet_id, bool force_demote_scan); + // that happened to finish last wrote. + // + // force_scan walks the blocks even when the recorded state already matches. The expiration + // path needs it: a tablet whose TTL has run out keeps producing TTL blocks on every read, + // because the read path picks the cache type from ttl_seconds alone without regard to + // expiry, so the tablet-level state cannot tell whether new ones have appeared. + void reconcile_tablet_blocks(int64_t tablet_id, bool force_scan); + + // Caller must hold _ttl_info_mutex. + void update_ttl_info_map_size_metrics(); std::mutex& transition_lock_for(int64_t tablet_id) { return _transition_locks[static_cast(tablet_id) % kTransitionLockStripes]; @@ -114,6 +129,7 @@ class BlockFileCacheTtlMgr { std::array _transition_locks; std::shared_ptr> _tablet_id_set_size_metrics; + std::shared_ptr> _ttl_info_map_size_metrics; }; } // namespace doris::io diff --git a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp index 3959ded1d8a631..435e0d0507adac 100644 --- a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp +++ b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp @@ -622,8 +622,100 @@ TEST_F(BlockFileCacheTtlMgrTest, TtlExtensionWinsOverConcurrentExpirationScan) { sync_point->disable_processing(); sync_point->clear_trace(); - EXPECT_TRUE(scan_stalled); + ASSERT_TRUE(scan_stalled); EXPECT_TRUE(ends_as_ttl); } +// _ttl_info_map is rebuilt in memory only, while the cache type of each block survives on disk. +// A tablet whose TTL expired while this BE was down therefore comes back with TTL blocks and no +// recorded state, and nothing later in the tablet's life re-examines them. +TEST_F(BlockFileCacheTtlMgrTest, ExpiredTabletDemotesTtlBlocksRestoredFromDisk) { + constexpr int64_t kTabletId = 10010; + const int64_t creation_time = UnixSeconds() - 7200; + auto tablet = std::make_shared(creation_time, 60); + fake_engine()->add_tablet(kTabletId, tablet); + + const uint64_t expiration_time = static_cast(creation_time) + 60; + UInt128Wrapper hash; + auto block = create_block(kTabletId, "ttl-restored-expired", 0, 1024, &hash, FileCacheType::TTL, + expiration_time); + persist_block_meta(kTabletId, hash, block->range().left, block->range().size(), + FileCacheType::TTL, expiration_time); + ASSERT_EQ(FileCacheType::TTL, block->cache_type()); + + _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); + _ttl_mgr->register_tablet_id(kTabletId); + + EXPECT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::NORMAL; }, + std::chrono::seconds(5))); +} + +// The read path types a block from ttl_seconds alone, without regard to whether the TTL has +// passed, so an expired tablet that is still being queried keeps producing TTL blocks after its +// existing ones were demoted. Those have to be collected too. +TEST_F(BlockFileCacheTtlMgrTest, ExpiredTabletDemotesTtlBlocksCachedAfterDemotion) { + constexpr int64_t kTabletId = 11011; + auto tablet = std::make_shared(UnixSeconds(), 120); + fake_engine()->add_tablet(kTabletId, tablet); + + UInt128Wrapper hash; + auto block = create_block(kTabletId, "ttl-expire-then-cache", 0, 1024, &hash); + persist_block_meta(kTabletId, hash, block->range().left, block->range().size()); + + _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); + _ttl_mgr->register_tablet_id(kTabletId); + + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, + std::chrono::seconds(5))); + + tablet->set_creation_time(UnixSeconds() - 120); + tablet->set_ttl_seconds(1); + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::NORMAL; }, + std::chrono::seconds(5))); + + // A query landing after the demotion caches a fresh block, still as TTL. + const uint64_t expiration_time = UnixSeconds() + 3600; + UInt128Wrapper late_hash; + auto late_block = create_block(kTabletId, "ttl-expire-then-cache-late", 0, 1024, &late_hash, + FileCacheType::TTL, expiration_time); + persist_block_meta(kTabletId, late_hash, late_block->range().left, late_block->range().size(), + FileCacheType::TTL, expiration_time); + ASSERT_EQ(FileCacheType::TTL, late_block->cache_type()); + + EXPECT_TRUE( + wait_for_condition([&]() { return late_block->cache_type() == FileCacheType::NORMAL; }, + std::chrono::seconds(5))); +} + +// Dropping the TTL of a tablet that had already expired leaves nothing to convert, but the +// tablet still has to stop being tracked -- otherwise it holds a map entry for the life of the +// process and never again qualifies for the periodic reconcile. +TEST_F(BlockFileCacheTtlMgrTest, TtlClearedAfterExpiryStopsTrackingTablet) { + constexpr int64_t kTabletId = 12012; + auto tablet = std::make_shared(UnixSeconds(), 120); + fake_engine()->add_tablet(kTabletId, tablet); + + UInt128Wrapper hash; + auto block = create_block(kTabletId, "ttl-cleared-after-expiry", 0, 1024, &hash); + persist_block_meta(kTabletId, hash, block->range().left, block->range().size()); + + _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); + _ttl_mgr->register_tablet_id(kTabletId); + + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, + std::chrono::seconds(5))); + ASSERT_TRUE(wait_for_condition([&]() { return _ttl_mgr->tracked_tablet_num() == 1; }, + std::chrono::seconds(5))); + + tablet->set_creation_time(UnixSeconds() - 120); + tablet->set_ttl_seconds(1); + ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::NORMAL; }, + std::chrono::seconds(5))); + + tablet->set_ttl_seconds(0); + EXPECT_TRUE(wait_for_condition([&]() { return _ttl_mgr->tracked_tablet_num() == 0; }, + std::chrono::seconds(5))); + EXPECT_EQ(FileCacheType::NORMAL, block->cache_type()); +} + } // namespace doris::io From 8833c5ca8180c8d9099494e5e6bbbec27b6a6eab Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Tue, 15 Sep 2026 21:05:33 +0800 Subject: [PATCH 4/7] simplify: promotion is edge triggered, demotion is level triggered The previous revision expressed a rule that is asymmetric with a state comparison that is symmetric, then patched the difference back in with a force_scan flag and a three-state optional. Both disappear once the asymmetry is stated directly. Skip only when the TTL is still running and the blocks are already promoted. Everything else scans: promotion is edge triggered because nothing takes blocks out of the TTL queue behind our back, while demotion has to be level triggered because an expired tablet keeps producing TTL blocks for as long as it is read. With that, "we have not promoted these blocks" and "we do not know what these blocks are" call for the same action, so the optional collapses back to a bool. Renamed to blocks_promoted, which is what it records -- what this manager did, not what the blocks are. --- be/src/io/cache/block_file_cache_ttl_mgr.cpp | 81 +++++++++----------- be/src/io/cache/block_file_cache_ttl_mgr.h | 23 ++---- 2 files changed, 45 insertions(+), 59 deletions(-) diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.cpp b/be/src/io/cache/block_file_cache_ttl_mgr.cpp index b8701b68cddc7a..d8daee29d196e6 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.cpp +++ b/be/src/io/cache/block_file_cache_ttl_mgr.cpp @@ -181,43 +181,46 @@ FileBlocks BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id(int64_t tablet_i return result; } -void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id, bool force_scan) { +void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id) { // Serialize all conversions of this tablet. Whichever caller takes this lock last re-reads - // the current state below and has the final say, so the update and expiration threads cannot - // fight over the same blocks and strand them in the loser's cache type. + // the state below and has the final say, so the update and expiration threads cannot fight + // over the same blocks and strand them in the loser's cache type. std::lock_guard transition_lock(transition_lock_for(tablet_id)); bool want_ttl = false; - // Unknown when the tablet is not tracked at all, which is also how a tablet looks right - // after a restart: the blocks on disk keep their type, _ttl_info_map does not. - std::optional blocks_are_ttl; - bool settled = false; + bool blocks_promoted = false; { // Deliberately re-read the map rather than trust what the caller saw: the expiration // thread picks its candidates up to a full gc interval before getting here. std::lock_guard lock(_ttl_info_mutex); auto it = _ttl_info_map.find(tablet_id); if (it != _ttl_info_map.end()) { - if (it->second.ttl == 0 && it->second.blocks_are_ttl == false) { - // No TTL left and the blocks are known to be normal: nothing to track. Dropped - // here rather than after a conversion, so that a tablet which settles without - // needing one still stops being tracked and goes back to the cheap path where - // it is only visited by the periodic reconcile. + if (it->second.ttl == 0 && !it->second.blocks_promoted) { + // No TTL, and none of its blocks were put in the TTL queue by us: nothing left + // to track. Dropped here rather than after a conversion, so that a tablet which + // settles without needing one still stops being walked on every round. _ttl_info_map.erase(it); update_ttl_info_map_size_metrics(); - settled = true; - } else { - want_ttl = it->second.is_ttl_active(UnixSeconds()); - blocks_are_ttl = it->second.blocks_are_ttl; + return; } + want_ttl = it->second.is_ttl_active(UnixSeconds()); + blocks_promoted = it->second.blocks_promoted; } } - // Nothing to apply. Note this is a state comparison, not a comparison of TTL values: a - // tablet whose TTL is rewritten to another still-valid value keeps its blocks where they - // are, instead of rescanning the whole tablet every time the property is touched. An - // unknown applied state never matches, so it always costs one scan. - if (settled || (!force_scan && blocks_are_ttl == want_ttl)) { + // The two directions are not symmetric. + // + // Promotion is edge triggered: once the blocks are in the TTL queue nothing takes them out + // behind our back, so rescanning a tablet whose TTL is still running is pure waste. This is + // also what makes a TTL rewritten to another still-valid value free, which matters where + // the property is rewritten on a schedule. + // + // Demotion is level triggered, and has to be. The read path derives a block's cache type + // from ttl_seconds alone, without regard to whether the TTL has passed: beta_rowset_reader + // assigns that duration to io_ctx.expiration_time, and CacheContext turns any non-zero + // value into a TTL block. So a tablet past its TTL keeps producing TTL blocks for as long + // as it is read, and nothing but this scan collects them. + if (want_ttl && blocks_promoted) { return; } @@ -250,9 +253,9 @@ void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id, bool force std::lock_guard lock(_ttl_info_mutex); auto it = _ttl_info_map.find(tablet_id); if (it != _ttl_info_map.end() && all_converted) { - // Left alone when a block failed to convert, so that the mismatch is still visible - // to the next round and gets retried instead of being recorded as done. - it->second.blocks_are_ttl = want_ttl; + // Left alone when a block failed to convert, so the mismatch stays visible to the + // next round and gets retried instead of being recorded as done. + it->second.blocks_promoted = want_ttl; } } } @@ -319,30 +322,28 @@ void BlockFileCacheTtlMgr::run_backgroud_update_ttl_info_map() { auto it = _ttl_info_map.find(tablet_id); if (ttl > 0) { if (it == _ttl_info_map.end()) { - // blocks_are_ttl stays unknown: what the blocks already are is - // whatever a previous run of this BE left on disk. _ttl_info_map.emplace(tablet_id, TtlInfo {ttl, tablet_ctime}); update_ttl_info_map_size_metrics(); } else { - // Keep blocks_are_ttl: it describes the blocks, not the tablet meta. + // Keep blocks_promoted: it records what we did to the blocks, not + // what the tablet meta says. it->second.ttl = ttl; it->second.tablet_ctime = tablet_ctime; } tracked = true; } else if (it != _ttl_info_map.end()) { - // Hold on to the entry until the blocks are actually demoted. It carries - // the blocks_are_ttl bit reconcile_tablet_blocks() needs, and drops - // itself once the tablet has settled back to NORMAL. + // Hold on to the entry until the blocks are actually demoted; it + // drops itself once the tablet has settled back to NORMAL. it->second.ttl = 0; tracked = true; } } - // An untracked tablet has no recorded state, so reconciling it scans once and - // demotes anything restored from persisted TTL metadata. Gated on the periodic + // An untracked tablet reconciles to NORMAL, which is how TTL blocks restored + // from persisted metadata are cleaned up after a restart. Gated on the periodic // round so that ordinary non-TTL tablets are not walked every time. if (tracked || need_full_reconcile) { - reconcile_tablet_blocks(tablet_id, /*force_scan=*/false); + reconcile_tablet_blocks(tablet_id); } } @@ -373,20 +374,14 @@ void BlockFileCacheTtlMgr::run_backgroud_expiration_check() { } } - // Swept on every round rather than once, because an expired tablet keeps producing - // TTL blocks for as long as it is read: the read path derives the cache type from - // ttl_seconds alone and never looks at whether the TTL has passed. Hence force_scan - // -- the tablet-level state says the blocks were demoted already and would - // otherwise skip the walk that collects the new ones. - // - // The list above is only a candidate set. reconcile_tablet_blocks() re-reads the - // tablet's state under the per-tablet lock, so a TTL extended in between is never - // demoted on the strength of what was observed here. + // Only a candidate list: reconcile_tablet_blocks() re-reads the tablet's state + // under the per-tablet lock, so a TTL extended in between is never demoted on the + // strength of what was observed here. for (int64_t tablet_id : expired_tablet_ids) { if (_stop_background.load(std::memory_order_acquire)) { break; } - reconcile_tablet_blocks(tablet_id, /*force_scan=*/true); + reconcile_tablet_blocks(tablet_id); } std::this_thread::sleep_for( diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.h b/be/src/io/cache/block_file_cache_ttl_mgr.h index 879e244af1c0ec..59e2233f284f6a 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.h +++ b/be/src/io/cache/block_file_cache_ttl_mgr.h @@ -28,7 +28,6 @@ #include #include #include -#include #include #include @@ -43,15 +42,12 @@ class CacheBlockMetaStore; struct TtlInfo { uint64_t ttl = 0; uint64_t tablet_ctime = 0; - // Cache type last applied to this tablet's blocks, or nullopt while the manager has not - // converted them yet and so cannot know: _ttl_info_map is rebuilt in memory only, while the - // types themselves survive a restart on disk. Unknown never compares equal to the wanted - // state, so a tablet entering the map always gets one reconciling scan. - // - // The applied state has to be remembered explicitly rather than inferred from the presence - // of the entry: that cannot tell "this tablet never had a TTL" apart from "its TTL expired - // and the blocks were demoted", and those two need opposite handling when a TTL arrives. - std::optional blocks_are_ttl; + // True once this manager has put the tablet's blocks into the TTL queue and nothing has + // taken them out since. Note it records what we did, not what the blocks are: it starts + // false for a tablet seen for the first time after a restart, whose blocks on disk may + // well be TTL already, because this process has no record of putting them there and must + // scan to find out. + bool blocks_promoted = false; // Whether this tablet's blocks belong in the TTL queue right now. bool is_ttl_active(uint64_t now) const { @@ -91,12 +87,7 @@ class BlockFileCacheTtlMgr { // Both background threads funnel through here and it is serialized per tablet, so they can // never scan the same tablet concurrently and leave the blocks in whatever type the scan // that happened to finish last wrote. - // - // force_scan walks the blocks even when the recorded state already matches. The expiration - // path needs it: a tablet whose TTL has run out keeps producing TTL blocks on every read, - // because the read path picks the cache type from ttl_seconds alone without regard to - // expiry, so the tablet-level state cannot tell whether new ones have appeared. - void reconcile_tablet_blocks(int64_t tablet_id, bool force_scan); + void reconcile_tablet_blocks(int64_t tablet_id); // Caller must hold _ttl_info_mutex. void update_ttl_info_map_size_metrics(); From 86f74cd2fddeef1829e9ddcead24c22afdb7aa89 Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Tue, 15 Sep 2026 23:32:52 +0800 Subject: [PATCH 5/7] drop the read-path detail from the demotion comment Why blocks keep arriving in the TTL queue after a demotion is a defect of its own and is being fixed separately; describing it here would read as intended behaviour and go stale with that fix. What this code needs to say is only that they can arrive, and that per-tablet state cannot see them. --- be/src/io/cache/block_file_cache_ttl_mgr.cpp | 8 +++----- be/test/io/cache/block_file_cache_ttl_mgr_test.cpp | 7 +++---- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/be/src/io/cache/block_file_cache_ttl_mgr.cpp b/be/src/io/cache/block_file_cache_ttl_mgr.cpp index d8daee29d196e6..924526709b8748 100644 --- a/be/src/io/cache/block_file_cache_ttl_mgr.cpp +++ b/be/src/io/cache/block_file_cache_ttl_mgr.cpp @@ -215,11 +215,9 @@ void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id) { // also what makes a TTL rewritten to another still-valid value free, which matters where // the property is rewritten on a schedule. // - // Demotion is level triggered, and has to be. The read path derives a block's cache type - // from ttl_seconds alone, without regard to whether the TTL has passed: beta_rowset_reader - // assigns that duration to io_ctx.expiration_time, and CacheContext turns any non-zero - // value into a TTL block. So a tablet past its TTL keeps producing TTL blocks for as long - // as it is read, and nothing but this scan collects them. + // Demotion is level triggered: blocks can still land in the TTL queue after a tablet was + // demoted, and what is recorded here is per tablet, so it cannot tell whether any have. + // Rescanning is the only way to collect them. if (want_ttl && blocks_promoted) { return; } diff --git a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp index 435e0d0507adac..86c6614ef1d098 100644 --- a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp +++ b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp @@ -650,9 +650,8 @@ TEST_F(BlockFileCacheTtlMgrTest, ExpiredTabletDemotesTtlBlocksRestoredFromDisk) std::chrono::seconds(5))); } -// The read path types a block from ttl_seconds alone, without regard to whether the TTL has -// passed, so an expired tablet that is still being queried keeps producing TTL blocks after its -// existing ones were demoted. Those have to be collected too. +// Blocks can still land in the TTL queue after a tablet's existing ones were demoted, and the +// recorded state is per tablet, so it cannot tell that they have. They have to be collected too. TEST_F(BlockFileCacheTtlMgrTest, ExpiredTabletDemotesTtlBlocksCachedAfterDemotion) { constexpr int64_t kTabletId = 11011; auto tablet = std::make_shared(UnixSeconds(), 120); @@ -673,7 +672,7 @@ TEST_F(BlockFileCacheTtlMgrTest, ExpiredTabletDemotesTtlBlocksCachedAfterDemotio ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::NORMAL; }, std::chrono::seconds(5))); - // A query landing after the demotion caches a fresh block, still as TTL. + // A fresh block lands in the TTL queue after the demotion. const uint64_t expiration_time = UnixSeconds() + 3600; UInt128Wrapper late_hash; auto late_block = create_block(kTabletId, "ttl-expire-then-cache-late", 0, 1024, &late_hash, From 770f6be9f9076a107933c48f38a61fe093213a71 Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Wed, 16 Sep 2026 09:48:08 +0800 Subject: [PATCH 6/7] fix two test defects found by BE UT **stack-use-after-return in TtlExtensionWinsOverConcurrentExpirationScan.** The sync point callbacks captured stack atomics by reference, but the manager's background threads are only joined in TearDown, after the test body's frame is gone. Neither CallbackGuard nor disable_processing() synchronizes with a callback already in flight, and this test's callback parks in a loop, which makes the window wide enough for ASAN to catch it. The flags are now held by shared_ptr and captured by value, and each test using a sync point joins the threads itself before its captures die. The release has to stay ahead of that join, or stop() waits on a thread parked inside the callback. The two older tests in this file share the pattern and are fixed the same way. **RewritingTtlToAnotherValidValueDoesNotRescanBlocks counted one scan.** A block's type flips inside the conversion loop, while the promotion is recorded after it, so observing the type is not enough to know the manager is done with the tablet. The test started counting in that gap and charged the tail of the promotion to the rewrites it meant to measure. It now waits for two further update rounds before installing the counter. The window was narrow until this change added an INFO line to the conversion path, which under ASAN is wide enough to lose the race reliably. --- .../cache/block_file_cache_ttl_mgr_test.cpp | 63 +++++++++++++------ 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp index 86c6614ef1d098..fdb94b3cec0b4a 100644 --- a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp +++ b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp @@ -369,16 +369,18 @@ TEST_F(BlockFileCacheTtlMgrTest, NonTtlTabletWithoutPriorTtlInfoSkipsBlockScan) auto block = create_block(kTabletId, "non-ttl-tablet", 0, 1024, &hash); persist_block_meta(kTabletId, hash, block->range().left, block->range().size()); - std::atomic block_scan_count {0}; + // Held by value in the callback: the background threads outlive this stack frame, and + // neither the guard nor disable_processing() synchronizes with a callback in flight. + auto block_scan_count = std::make_shared>(0); auto* sync_point = SyncPoint::get_instance(); sync_point->clear_all_call_backs(); sync_point->clear_trace(); SyncPoint::CallbackGuard guard; sync_point->set_call_back( "BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id", - [&block_scan_count](std::vector&& args) { + [block_scan_count](std::vector&& args) { if (doris::try_any_cast(args[0]) == kTabletId) { - block_scan_count.fetch_add(1, std::memory_order_relaxed); + block_scan_count->fetch_add(1, std::memory_order_relaxed); } }, &guard); @@ -391,11 +393,13 @@ TEST_F(BlockFileCacheTtlMgrTest, NonTtlTabletWithoutPriorTtlInfoSkipsBlockScan) [this]() { return fake_engine()->get_tablet_meta_call_count() >= 2; }, std::chrono::seconds(5)); std::this_thread::sleep_for(std::chrono::milliseconds(100)); + // Join the background threads before the callback and its captures go away. + _ttl_mgr.reset(); sync_point->disable_processing(); sync_point->clear_trace(); EXPECT_TRUE(update_thread_observed); - EXPECT_EQ(0, block_scan_count.load(std::memory_order_relaxed)); + EXPECT_EQ(0, block_scan_count->load(std::memory_order_relaxed)); EXPECT_EQ(FileCacheType::NORMAL, block->cache_type()); } @@ -412,16 +416,18 @@ TEST_F(BlockFileCacheTtlMgrTest, PeriodicReconcileDemotesTtlBlockWithoutPriorTtl FileCacheType::TTL, expiration_time); ASSERT_EQ(FileCacheType::TTL, block->cache_type()); - std::atomic block_scan_count {0}; + // Held by value in the callback: the background threads outlive this stack frame, and + // neither the guard nor disable_processing() synchronizes with a callback in flight. + auto block_scan_count = std::make_shared>(0); auto* sync_point = SyncPoint::get_instance(); sync_point->clear_all_call_backs(); sync_point->clear_trace(); SyncPoint::CallbackGuard guard; sync_point->set_call_back( "BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id", - [&block_scan_count](std::vector&& args) { + [block_scan_count](std::vector&& args) { if (doris::try_any_cast(args[0]) == kTabletId) { - block_scan_count.fetch_add(1, std::memory_order_relaxed); + block_scan_count->fetch_add(1, std::memory_order_relaxed); } }, &guard); @@ -433,11 +439,13 @@ TEST_F(BlockFileCacheTtlMgrTest, PeriodicReconcileDemotesTtlBlockWithoutPriorTtl bool demoted = wait_for_condition([&]() { return block->cache_type() == FileCacheType::NORMAL; }, std::chrono::seconds(5)); + // Join the background threads before the callback and its captures go away. + _ttl_mgr.reset(); sync_point->disable_processing(); sync_point->clear_trace(); EXPECT_TRUE(demoted); - EXPECT_GE(block_scan_count.load(std::memory_order_relaxed), 1); + EXPECT_GE(block_scan_count->load(std::memory_order_relaxed), 1); } TEST_F(BlockFileCacheTtlMgrTest, TabletTtlRemovedMovesBlocksBackToNormal) { @@ -533,16 +541,26 @@ TEST_F(BlockFileCacheTtlMgrTest, RewritingTtlToAnotherValidValueDoesNotRescanBlo ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() == FileCacheType::TTL; }, std::chrono::seconds(5))); - std::atomic block_scan_count {0}; + // The promotion is recorded only after every block has been converted, so the manager has + // not necessarily finished with the tablet at the moment the type flips. Let a couple of + // rounds pass before counting, or the tail of the promotion is charged to the rewrites. + int64_t settled_after = fake_engine()->get_tablet_meta_call_count(); + ASSERT_TRUE(wait_for_condition( + [&]() { return fake_engine()->get_tablet_meta_call_count() >= settled_after + 2; }, + std::chrono::seconds(5))); + + // Held by value in the callback: the background threads outlive this stack frame, and + // neither the guard nor disable_processing() synchronizes with a callback in flight. + auto block_scan_count = std::make_shared>(0); auto* sync_point = SyncPoint::get_instance(); sync_point->clear_all_call_backs(); sync_point->clear_trace(); SyncPoint::CallbackGuard guard; sync_point->set_call_back( "BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id", - [&block_scan_count](std::vector&& args) { + [block_scan_count](std::vector&& args) { if (doris::try_any_cast(args[0]) == kTabletId) { - block_scan_count.fetch_add(1, std::memory_order_relaxed); + block_scan_count->fetch_add(1, std::memory_order_relaxed); } }, &guard); @@ -558,10 +576,12 @@ TEST_F(BlockFileCacheTtlMgrTest, RewritingTtlToAnotherValidValueDoesNotRescanBlo std::chrono::seconds(5))); } + // Join the background threads before the callback and its captures go away. + _ttl_mgr.reset(); sync_point->disable_processing(); sync_point->clear_trace(); - EXPECT_EQ(0, block_scan_count.load(std::memory_order_relaxed)); + EXPECT_EQ(0, block_scan_count->load(std::memory_order_relaxed)); EXPECT_EQ(FileCacheType::TTL, block->cache_type()); } @@ -582,22 +602,25 @@ TEST_F(BlockFileCacheTtlMgrTest, TtlExtensionWinsOverConcurrentExpirationScan) { // Stall the demotion scan midway so the TTL can be extended underneath it, reproducing the // window where the expiration check acts on a view of the tablet that is already stale. - std::atomic demote_scan_entered {false}; - std::atomic release_demote_scan {false}; + // Held by value in the callback rather than captured by reference: the background threads + // outlive this stack frame, and neither the guard nor disable_processing() synchronizes + // with a callback already in flight. A callback that stalls makes that window wide. + auto demote_scan_entered = std::make_shared>(false); + auto release_demote_scan = std::make_shared>(false); auto* sync_point = SyncPoint::get_instance(); sync_point->clear_all_call_backs(); sync_point->clear_trace(); SyncPoint::CallbackGuard guard; sync_point->set_call_back( "BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id", - [&](std::vector&& args) { + [demote_scan_entered, release_demote_scan](std::vector&& args) { if (doris::try_any_cast(args[0]) != kTabletId) { return; } - if (demote_scan_entered.exchange(true, std::memory_order_acq_rel)) { + if (demote_scan_entered->exchange(true, std::memory_order_acq_rel)) { return; } - while (!release_demote_scan.load(std::memory_order_acquire)) { + while (!release_demote_scan->load(std::memory_order_acquire)) { std::this_thread::sleep_for(std::chrono::milliseconds(5)); } }, @@ -608,17 +631,19 @@ TEST_F(BlockFileCacheTtlMgrTest, TtlExtensionWinsOverConcurrentExpirationScan) { tablet->set_ttl_seconds(1); bool scan_stalled = wait_for_condition( - [&]() { return demote_scan_entered.load(std::memory_order_acquire); }, + [&]() { return demote_scan_entered->load(std::memory_order_acquire); }, std::chrono::seconds(10)); // Extend the TTL while the demotion is still in flight. tablet->set_creation_time(UnixSeconds()); tablet->set_ttl_seconds(30758400); std::this_thread::sleep_for(std::chrono::milliseconds(200)); - release_demote_scan.store(true, std::memory_order_release); + // Must come before the join below, or stop() waits on a thread parked in the callback. + release_demote_scan->store(true, std::memory_order_release); bool ends_as_ttl = wait_for_condition( [&]() { return block->cache_type() == FileCacheType::TTL; }, std::chrono::seconds(10)); + _ttl_mgr.reset(); sync_point->disable_processing(); sync_point->clear_trace(); From cd5cfb7d321a87bc7d0b6c9f38e51ab981ea6310 Mon Sep 17 00:00:00 2001 From: Xin Liao Date: Wed, 16 Sep 2026 14:27:36 +0800 Subject: [PATCH 7/7] stop the cache-owned TTL manager in the test fixture BlockFileCache::initialize() starts a TTL manager of its own, and every case in this file then drives a second one against the same cache. Two managers converting the same blocks makes both the conversions and the scan counts nondeterministic: the new cases assert that a block is TTL before exercising the demotion, and the cache's manager could demote it first -- which is what BE UT hit. It also means the scan-count assertions were never only about the manager under test. The fixture now stops the cache-owned one, via a new BlockFileCache accessor, so each case drives exactly the manager it created. The two new cases also published their block to the meta store before asserting its type. A scan only reaches blocks listed there, so the assertion now comes first; in the case that caches a block after the demotion this is load bearing, because its manager is already running and sweeps the tablet every round. --- be/src/io/cache/block_file_cache.h | 4 ++++ .../io/cache/block_file_cache_ttl_mgr_test.cpp | 16 ++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/be/src/io/cache/block_file_cache.h b/be/src/io/cache/block_file_cache.h index 91c453e12aa414..99830cad839e8b 100644 --- a/be/src/io/cache/block_file_cache.h +++ b/be/src/io/cache/block_file_cache.h @@ -312,6 +312,10 @@ class BlockFileCache { [[nodiscard]] bool get_async_open_success() const { return _async_open_done; } + // The manager that keeps cached blocks in the cache type their tablet's TTL asks for. + // Exposed so that tests can drive it deterministically rather than race its threads. + BlockFileCacheTtlMgr* get_ttl_mgr() { return _ttl_mgr.get(); } + BlockFileCache& operator=(const BlockFileCache&) = delete; BlockFileCache(const BlockFileCache&) = delete; diff --git a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp index fdb94b3cec0b4a..9edd5754e3f01b 100644 --- a/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp +++ b/be/test/io/cache/block_file_cache_ttl_mgr_test.cpp @@ -224,6 +224,14 @@ class BlockFileCacheTtlMgrTest : public testing::Test { ASSERT_TRUE(_cache->initialize()); ASSERT_TRUE(wait_for_condition([this]() { return _cache->get_async_open_success(); }, std::chrono::seconds(5))); + + // initialize() starts a TTL manager of its own against this cache. Every case below + // drives one it owns, and two of them converting the same blocks makes both the + // conversions and the scan counts nondeterministic -- a block can be demoted before + // the case has finished setting up the state it means to exercise. + if (auto* cache_owned_ttl_mgr = _cache->get_ttl_mgr()) { + cache_owned_ttl_mgr->stop(); + } } void TearDown() override { @@ -664,9 +672,11 @@ TEST_F(BlockFileCacheTtlMgrTest, ExpiredTabletDemotesTtlBlocksRestoredFromDisk) UInt128Wrapper hash; auto block = create_block(kTabletId, "ttl-restored-expired", 0, 1024, &hash, FileCacheType::TTL, expiration_time); + // Asserted before the block is published to the meta store: a scan can only reach a block + // that is listed there, so until then no manager can convert it out from under us. + ASSERT_EQ(FileCacheType::TTL, block->cache_type()); persist_block_meta(kTabletId, hash, block->range().left, block->range().size(), FileCacheType::TTL, expiration_time); - ASSERT_EQ(FileCacheType::TTL, block->cache_type()); _ttl_mgr = std::make_unique(_cache.get(), _meta_store.get()); _ttl_mgr->register_tablet_id(kTabletId); @@ -702,9 +712,11 @@ TEST_F(BlockFileCacheTtlMgrTest, ExpiredTabletDemotesTtlBlocksCachedAfterDemotio UInt128Wrapper late_hash; auto late_block = create_block(kTabletId, "ttl-expire-then-cache-late", 0, 1024, &late_hash, FileCacheType::TTL, expiration_time); + // Same ordering as above, and here it matters: the manager is running by this point and + // sweeps this tablet every round, so publishing first would race the assertion. + ASSERT_EQ(FileCacheType::TTL, late_block->cache_type()); persist_block_meta(kTabletId, late_hash, late_block->range().left, late_block->range().size(), FileCacheType::TTL, expiration_time); - ASSERT_EQ(FileCacheType::TTL, late_block->cache_type()); EXPECT_TRUE( wait_for_condition([&]() { return late_block->cache_type() == FileCacheType::NORMAL; },