github-actions[bot] commented on code in PR #67971:
URL: https://github.com/apache/doris/pull/67971#discussion_r4026001367
##########
be/src/io/cache/block_file_cache_ttl_mgr.cpp:
##########
@@ -168,6 +181,83 @@ FileBlocks
BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id(int64_t tablet_i
return result;
}
+void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id) {
+ // Serialize all conversions of this tablet. Whichever caller takes this
lock last re-reads
+ // 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<std::mutex>
transition_lock(transition_lock_for(tablet_id));
+
+ bool want_ttl = 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<std::mutex> 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_promoted) {
Review Comment:
[P1] `blocks_promoted=false` does not prove that there are no TTL blocks to
demote. After an expired tablet is successfully demoted, a cache fill that
captured the old TTL context can still publish a late TTL block (the comment
below explicitly acknowledges this). If the property is then cleared before the
next expiry sweep, this branch erases the entry and returns without scanning,
leaving that block in the TTL queue until the 20-round fallback—about an hour
at the defaults. Please require one successful NORMAL reconciliation on the
TTL-to-zero transition before erasing, coordinated with late block publication.
##########
be/src/io/cache/block_file_cache_ttl_mgr.cpp:
##########
@@ -220,47 +312,36 @@ 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<std::mutex> 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});
+ update_ttl_info_map_size_metrics();
+ } else {
+ // 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;
}
- } 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
+ // 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";
- }
- }
- }
+ // 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) {
Review Comment:
[P1] Seed the reconcile set from restored metadata. `need_full_reconcile`
still iterates only `tablet_ids_to_process`, copied from `_tablet_id_set`, but
the async RocksDB restore paths add restored cells directly with `add_cell()`
and never call `register_tablet_id()`; a fully restored cache hit also only
calls `use_cell()`. Thus after a real restart the set can remain empty and the
expired persisted TTL block this branch is meant to demote is never visited.
The new test masks this by manually registering the ID instead of recreating
the cache. Please register/seed restored tablet IDs in the production lifecycle
and cover that lifecycle without the manual call.
##########
be/test/io/cache/block_file_cache_ttl_mgr_test.cpp:
##########
@@ -462,4 +478,280 @@ TEST_F(BlockFileCacheTtlMgrTest,
TabletTtlRemovedMovesBlocksBackToNormal) {
std::chrono::seconds(5)));
}
+TEST_F(BlockFileCacheTtlMgrTest, ExpiredTtlExtendedMovesBlocksBackToTtl) {
+ constexpr int64_t kTabletId = 6006;
+ auto tablet = std::make_shared<FakeTablet>(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<BlockFileCacheTtlMgr>(_cache.get(),
_meta_store.get());
+ _ttl_mgr->register_tablet_id(kTabletId);
+
+ ASSERT_TRUE(wait_for_condition([&]() { return block->cache_type() ==
FileCacheType::TTL; },
Review Comment:
[P1] Use a synchronized cache-type observation in these waits.
`FileBlock::cache_type()` is a plain read of `_key.meta.type`, while the TTL
manager writes that field under the cache and block mutexes. Polling it
concurrently here is a C++ data race, and the same unlocked predicate appears
throughout the new transition tests. Please observe through a lock-safe path
such as `BlockFileCache::dump_single_cache_type()`, or add a helper that takes
the required lock.
##########
be/src/io/cache/block_file_cache_ttl_mgr.cpp:
##########
@@ -279,29 +360,26 @@ void
BlockFileCacheTtlMgr::run_backgroud_expiration_check() {
while (!_stop_background.load(std::memory_order_acquire)) {
try {
- std::map<int64_t, TtlInfo> ttl_info_copy;
-
- // Copy TTL info for processing
+ // Collect tablets whose TTL has run out.
+ std::vector<int64_t> expired_tablet_ids;
{
std::lock_guard<std::mutex> 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.ttl > 0 &&
!ttl_info.is_ttl_active(current_time)) {
Review Comment:
[P2] Avoid giving unchanged expired tablets to both sweepers. An expired
entry keeps a nonzero TTL, so the updater treats it as `tracked` and performs
the level-triggered scan every update round; this expiration loop then selects
the same entry and performs the same scan again. With both defaults at 180
seconds, every expired tablet is prefix-scanned twice every three minutes
indefinitely. Each prefix row also calls `get_blocks_by_key()`, which copies
every offset for that hash under the global cache mutex, making multi-block
files quadratic per scan. Please assign expiry sweeping to one owner and
coalesce/index the lookup.
##########
be/src/io/cache/block_file_cache_ttl_mgr.cpp:
##########
@@ -168,6 +181,83 @@ FileBlocks
BlockFileCacheTtlMgr::get_file_blocks_from_tablet_id(int64_t tablet_i
return result;
}
+void BlockFileCacheTtlMgr::reconcile_tablet_blocks(int64_t tablet_id) {
+ // Serialize all conversions of this tablet. Whichever caller takes this
lock last re-reads
+ // 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<std::mutex>
transition_lock(transition_lock_for(tablet_id));
+
+ bool want_ttl = 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<std::mutex> 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_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();
+ return;
+ }
+ want_ttl = it->second.is_ttl_active(UnixSeconds());
+ blocks_promoted = it->second.blocks_promoted;
+ }
+ }
+
+ // 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: 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;
+ }
+
+ // 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);
+ 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()) {
+ ++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<std::mutex> lock(_ttl_info_mutex);
+ auto it = _ttl_info_map.find(tablet_id);
+ if (it != _ttl_info_map.end() && all_converted) {
Review Comment:
[P1] Do not latch promotion from this incomplete scan. A NORMAL cell is
registered before it finishes downloading, but `range_get()` cannot see its
pending metadata PUT and `get_blocks_by_key()` filters out non-DOWNLOADED
cells. The updater can therefore scan zero blocks, leave `all_converted=true`,
and set `blocks_promoted=true`; the block then finalizes with its original
NORMAL key, and every later active-TTL round returns at line 221. Please make
the state generation/dirty-aware (or cover in-memory and pending blocks) so
creation/finalization invalidates promotion completion, and add a deterministic
in-flight-block test.
##########
be/test/io/cache/block_file_cache_ttl_mgr_test.cpp:
##########
@@ -462,4 +478,280 @@ TEST_F(BlockFileCacheTtlMgrTest,
TabletTtlRemovedMovesBlocksBackToNormal) {
std::chrono::seconds(5)));
}
+TEST_F(BlockFileCacheTtlMgrTest, ExpiredTtlExtendedMovesBlocksBackToTtl) {
+ constexpr int64_t kTabletId = 6006;
+ auto tablet = std::make_shared<FakeTablet>(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<BlockFileCacheTtlMgr>(_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<FakeTablet>(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<BlockFileCacheTtlMgr>(_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<FakeTablet>(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<BlockFileCacheTtlMgr>(_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)));
+
+ // 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<std::atomic<int64_t>>(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<std::any>&& args) {
+ if (doris::try_any_cast<int64_t>(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)));
+ }
+
+ // 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(FileCacheType::TTL, block->cache_type());
+}
+
+TEST_F(BlockFileCacheTtlMgrTest, TtlExtensionWinsOverConcurrentExpirationScan)
{
+ constexpr int64_t kTabletId = 9009;
+ auto tablet = std::make_shared<FakeTablet>(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<BlockFileCacheTtlMgr>(_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.
+ // 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<std::atomic<bool>>(false);
+ auto release_demote_scan = std::make_shared<std::atomic<bool>>(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",
+ [demote_scan_entered, release_demote_scan](std::vector<std::any>&&
args) {
+ if (doris::try_any_cast<int64_t>(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));
+ // 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(
Review Comment:
[P1] Wait for the conflicting scan to finish before accepting TTL. At this
point the block is still TTL from setup, and the stalled callback sleeps in 5
ms increments. The first predicate evaluation after `release_demote_scan=true`
can therefore succeed before the demotion runs; `_ttl_mgr.reset()` may then
join a scan that leaves the block NORMAL, while the test still asserts the
previously captured `true`. Please add an explicit completion barrier for the
released reconciliation (and proof that the updater observed the extension),
then assert `block->cache_type()` after the final transition rather than
relying on this pre-existing state.
##########
be/test/io/cache/block_file_cache_ttl_mgr_test.cpp:
##########
@@ -462,4 +478,280 @@ TEST_F(BlockFileCacheTtlMgrTest,
TabletTtlRemovedMovesBlocksBackToNormal) {
std::chrono::seconds(5)));
}
+TEST_F(BlockFileCacheTtlMgrTest, ExpiredTtlExtendedMovesBlocksBackToTtl) {
+ constexpr int64_t kTabletId = 6006;
+ auto tablet = std::make_shared<FakeTablet>(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<BlockFileCacheTtlMgr>(_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<FakeTablet>(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<BlockFileCacheTtlMgr>(_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<FakeTablet>(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<BlockFileCacheTtlMgr>(_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)));
+
+ // 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<std::atomic<int64_t>>(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<std::any>&& args) {
+ if (doris::try_any_cast<int64_t>(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)));
+ }
+
+ // 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(FileCacheType::TTL, block->cache_type());
+}
+
+TEST_F(BlockFileCacheTtlMgrTest, TtlExtensionWinsOverConcurrentExpirationScan)
{
+ constexpr int64_t kTabletId = 9009;
+ auto tablet = std::make_shared<FakeTablet>(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<BlockFileCacheTtlMgr>(_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.
+ // 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<std::atomic<bool>>(false);
+ auto release_demote_scan = std::make_shared<std::atomic<bool>>(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",
+ [demote_scan_entered, release_demote_scan](std::vector<std::any>&&
args) {
+ if (doris::try_any_cast<int64_t>(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);
Review Comment:
[P1] Do not mutate `creation_time` while the manager thread is reading it.
`TabletMeta::creation_time()` and `set_creation_time()` are plain, unlocked
accesses to `_creation_time`, so this write races the update thread at
`tablet_meta->creation_time()` and makes the test undefined/flaky (the same
pattern appears in the other new expiry tests). Use a fixed past creation time
before starting the manager and drive active/expired/extended states only
through the locked TTL setter.
--
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]
