From 39c64c9c67ae4528de667dfca2aa4775f5d92b79 Mon Sep 17 00:00:00 2001 From: Konstantinos Kanellis Date: Mon, 29 Sep 2025 16:18:47 -0500 Subject: [PATCH 1/2] Handle corner case in hlog lookup compaction --- cc/src/core/f2.h | 17 ++++++++++++++--- cc/src/core/faster.h | 6 ++++-- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/cc/src/core/f2.h b/cc/src/core/f2.h index 518b1325b..1b53e268e 100644 --- a/cc/src/core/f2.h +++ b/cc/src/core/f2.h @@ -741,15 +741,26 @@ inline bool F2Kv::CompactLog(S& store, StoreType store_type, bool shift_begin_address, int n_threads, bool checkpoint) { const bool is_hot_store = (store_type == StoreType::HOT); - uint64_t tail_address = store.hlog.GetTailAddress().control(); + uint64_t begin_address{ store.hlog.begin_address.control() }; + uint64_t tail_address{ store.hlog.GetTailAddress().control() }; + uint64_t safe_read_only_address{ store.hlog.safe_read_only_address.control() }; + log_debug("Compact %s: {%.2lf GB} {Goal %.2lf GB} [%lu %lu] -> [%lu %lu]", is_hot_store ? "HOT" : "COLD", static_cast(store.Size()) / (1 << 30), static_cast(tail_address - until_address) / (1 << 30), store.hlog.begin_address.control(), tail_address, until_address, tail_address); - if (until_address > store.hlog.safe_read_only_address.control()) { - throw std::invalid_argument{ "Can only compact until safe read-only region" }; + + if (until_address <= begin_address) { + log_warn("Skipping log compaction due to: until_address <= begin_address. " + "until_address should be larger than hlog.begin_address"); + return false; + } + if (until_address > safe_read_only_address) { + log_warn("Skipping log compaction due to: until_address > safe_read_only_address. " + "Can only compact until safe read-only region"); + return false; } StoreCheckpointStatus status; diff --git a/cc/src/core/faster.h b/cc/src/core/faster.h index 6564c7f3e..eb89ee14f 100644 --- a/cc/src/core/faster.h +++ b/cc/src/core/faster.h @@ -3591,7 +3591,9 @@ inline bool FasterKv::CompactWithLookup(uint64_t until_address, template bool FasterKv::InternalCompactWithLookup(uint64_t until_address, bool shift_begin_address, int n_threads, bool to_other_store, bool checkpoint, Guid& checkpoint_token) { - if (hlog.begin_address.load() > until_address) { + Address begin_address = hlog.begin_address.load(); + + if (begin_address >= until_address) { throw std::invalid_argument {"Invalid until address; should be larger than hlog.begin_address"}; } if (until_address > hlog.safe_read_only_address.control()) { @@ -3610,7 +3612,7 @@ bool FasterKv::InternalCompactWithLookup(uint64_t until_address, std::deque threads; - ConcurrentLogPageIterator iter(&hlog, &disk, &epoch_, hlog.begin_address.load(), Address(until_address)); + ConcurrentLogPageIterator iter(&hlog, &disk, &epoch_, begin_address, Address(until_address)); compaction_context_.Initialize(&iter, n_threads-1, to_other_store); // Spawn the threads first From f750b46d475fe083c38275d67ba3247ab3bdcc12 Mon Sep 17 00:00:00 2001 From: Konstantinos Kanellis Date: Mon, 29 Sep 2025 16:20:40 -0500 Subject: [PATCH 2/2] Minor fixes in F2 tests --- cc/test/f2_test.cc | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/cc/test/f2_test.cc b/cc/test/f2_test.cc index 97201120f..3f2dddfea 100644 --- a/cc/test/f2_test.cc +++ b/cc/test/f2_test.cc @@ -292,8 +292,9 @@ TEST_P(HotColdParameterizedTestParam, UpsertRead) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Read. @@ -332,8 +333,9 @@ TEST_P(HotColdParameterizedTestParam, UpsertRead) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true, 4); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true, 4)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Read existing again (in random order), plus non-existing ones @@ -433,8 +435,9 @@ TEST_P(HotColdParameterizedTestParam, HotColdCompaction) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Read existing again (in random order), plus non-existing ones @@ -530,8 +533,9 @@ TEST_P(HotColdParameterizedTestParam, UpsertDelete) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true, 1); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true, 1)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Read both existent and non-existent keys for(size_t idx = 1; idx <= num_records; idx++) { @@ -574,8 +578,9 @@ TEST_P(HotColdParameterizedTestParam, UpsertDelete) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true, 1); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true, 1)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Read all keys -- all should return NOT_FOUND @@ -709,8 +714,9 @@ TEST_P(HotColdParameterizedTestParam, Rmw) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Rmw, decrement by 1, 8 times -- random order @@ -895,8 +901,9 @@ TEST_P(HotColdParameterizedTestParam, ConcurrentOps) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Perform reads for all keys (and more non-existent ones) in random order @@ -1281,8 +1288,9 @@ TEST_P(HotColdParameterizedTestParam, VariableLengthKey) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Read again. @@ -1609,8 +1617,9 @@ TEST_P(HotColdParameterizedTestParam, VariableLengthValue) { if (!auto_compaction) { // perform hot-cold compaction uint64_t hot_size = store.hot_store.Size(), cold_size = store.cold_store.Size(); - store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true); - ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + if (store.CompactHotLog(store.hot_store.hlog.safe_read_only_address.control(), true)) { + ASSERT_TRUE(store.hot_store.Size() < hot_size && store.cold_store.Size() > cold_size); + } } // Read again.