From ccf5959b63b90ffd9650cf2571b22992fc249fee Mon Sep 17 00:00:00 2001 From: CalvinNeo Date: Thu, 11 Aug 2022 11:16:13 +0800 Subject: [PATCH 1/6] fix Signed-off-by: CalvinNeo --- dbms/src/Storages/Transaction/KVStore.cpp | 5 +++++ dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp | 2 ++ 2 files changed, 7 insertions(+) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index a4d9fab1fd0..14167627c3e 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -338,6 +338,11 @@ bool KVStore::tryFlushRegionData(UInt64 region_id, bool try_until_succeed, TMTCo { auto region_task_lock = region_manager.genRegionTaskLock(region_id); const RegionPtr curr_region_ptr = getRegion(region_id); + if (curr_region_ptr == nullptr) + { + LOG_FMT_WARNING(log, "region {} [index: {}, term {}], not exist when flushing, maybe have exec `RemoveNode` first", region_id, index, term); + return true; + } return canFlushRegionDataImpl(curr_region_ptr, true, try_until_succeed, tmt, region_task_lock, index, term); } diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index 180be7e65b7..6a6198b6c72 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -1242,6 +1242,8 @@ void RegionKVStoreTest::testKVStore() ASSERT_EQ(kvs.needFlushRegionData(19, ctx.getTMTContext()), true); // Force flush until succeed only for testing. ASSERT_EQ(kvs.tryFlushRegionData(19, true, ctx.getTMTContext(), 0, 0), true); + // Non existing region. + ASSERT_EQ(kvs.tryFlushRegionData(1999, true, ctx.getTMTContext(), 0, 0), true); } } From 3af34398b58bca308cd604f8fbc820eca5cde2d4 Mon Sep 17 00:00:00 2001 From: CalvinNeo Date: Thu, 11 Aug 2022 11:21:52 +0800 Subject: [PATCH 2/6] f Signed-off-by: CalvinNeo --- dbms/src/Storages/Transaction/KVStore.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index 14167627c3e..a22e50ee508 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -340,6 +340,10 @@ bool KVStore::tryFlushRegionData(UInt64 region_id, bool try_until_succeed, TMTCo const RegionPtr curr_region_ptr = getRegion(region_id); if (curr_region_ptr == nullptr) { + /// If we can't find region here, we return true so proxy can trigger a CompactLog. + /// The triggered CompactLog will be handled by `handleUselessAdminRaftCmd`, + /// and result in a `EngineStoreApplyRes::NotFound`. + /// Proxy will print `region not found in engine-store, maybe have exec `RemoveNode` first`. LOG_FMT_WARNING(log, "region {} [index: {}, term {}], not exist when flushing, maybe have exec `RemoveNode` first", region_id, index, term); return true; } From 15a440b283c4c38ee51c09d6c2b86d5c9483292b Mon Sep 17 00:00:00 2001 From: CalvinNeo Date: Thu, 11 Aug 2022 14:19:42 +0800 Subject: [PATCH 3/6] ut Signed-off-by: CalvinNeo --- dbms/src/Storages/Transaction/KVStore.cpp | 1 + dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index a22e50ee508..364681fffea 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -331,6 +331,7 @@ bool KVStore::needFlushRegionData(UInt64 region_id, TMTContext & tmt) { auto region_task_lock = region_manager.genRegionTaskLock(region_id); const RegionPtr curr_region_ptr = getRegion(region_id); + // TODO Should handle when curr_region_ptr is null. return canFlushRegionDataImpl(curr_region_ptr, false, false, tmt, region_task_lock, 0, 0); } diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index 6a6198b6c72..e7e84a94081 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -1244,6 +1244,14 @@ void RegionKVStoreTest::testKVStore() ASSERT_EQ(kvs.tryFlushRegionData(19, true, ctx.getTMTContext(), 0, 0), true); // Non existing region. ASSERT_EQ(kvs.tryFlushRegionData(1999, true, ctx.getTMTContext(), 0, 0), true); + + raft_cmdpb::AdminRequest request; + raft_cmdpb::AdminResponse response; + + request.mutable_compact_log(); + request.set_cmd_type(::raft_cmdpb::AdminCmdType::CompactLog); + ASSERT_EQ(kvs.handleAdminRaftCmd(raft_cmdpb::AdminRequest{request}, std::move(response), 1999, 22, 6, ctx.getTMTContext()), EngineStoreApplyRes::NotFound); + } } From 6aa79febc0e61c93c69c850e506d0cb8dba9a962 Mon Sep 17 00:00:00 2001 From: CalvinNeo Date: Thu, 11 Aug 2022 15:38:48 +0800 Subject: [PATCH 4/6] fmt Signed-off-by: CalvinNeo --- dbms/src/Storages/Transaction/KVStore.cpp | 2 +- dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index 364681fffea..5c0f9602320 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -344,7 +344,7 @@ bool KVStore::tryFlushRegionData(UInt64 region_id, bool try_until_succeed, TMTCo /// If we can't find region here, we return true so proxy can trigger a CompactLog. /// The triggered CompactLog will be handled by `handleUselessAdminRaftCmd`, /// and result in a `EngineStoreApplyRes::NotFound`. - /// Proxy will print `region not found in engine-store, maybe have exec `RemoveNode` first`. + /// Proxy will print this message and continue: `region not found in engine-store, maybe have exec `RemoveNode` first`. LOG_FMT_WARNING(log, "region {} [index: {}, term {}], not exist when flushing, maybe have exec `RemoveNode` first", region_id, index, term); return true; } diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index e7e84a94081..503c7a21864 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -1251,7 +1251,6 @@ void RegionKVStoreTest::testKVStore() request.mutable_compact_log(); request.set_cmd_type(::raft_cmdpb::AdminCmdType::CompactLog); ASSERT_EQ(kvs.handleAdminRaftCmd(raft_cmdpb::AdminRequest{request}, std::move(response), 1999, 22, 6, ctx.getTMTContext()), EngineStoreApplyRes::NotFound); - } } From e1aae49074d967bd9e113d72c1cfd0a090a5c4a8 Mon Sep 17 00:00:00 2001 From: CalvinNeo Date: Thu, 11 Aug 2022 15:41:58 +0800 Subject: [PATCH 5/6] f Signed-off-by: CalvinNeo --- dbms/src/Storages/Transaction/KVStore.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index 5c0f9602320..f484b3d6644 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -355,7 +355,7 @@ bool KVStore::canFlushRegionDataImpl(const RegionPtr & curr_region_ptr, UInt8 fl { if (curr_region_ptr == nullptr) { - throw Exception(fmt::format("region not found when trying flush", ErrorCodes::LOGICAL_ERROR)); + throw Exception("region not found when trying flush", ErrorCodes::LOGICAL_ERROR); } auto & curr_region = *curr_region_ptr; From a54fe0c24100f131db92619dd2a02332ee979776 Mon Sep 17 00:00:00 2001 From: CalvinNeo Date: Thu, 11 Aug 2022 16:12:44 +0800 Subject: [PATCH 6/6] f Signed-off-by: CalvinNeo --- dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index 503c7a21864..864de5b380e 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -1243,11 +1243,10 @@ void RegionKVStoreTest::testKVStore() // Force flush until succeed only for testing. ASSERT_EQ(kvs.tryFlushRegionData(19, true, ctx.getTMTContext(), 0, 0), true); // Non existing region. + // Flush and CompactLog will not panic. ASSERT_EQ(kvs.tryFlushRegionData(1999, true, ctx.getTMTContext(), 0, 0), true); - raft_cmdpb::AdminRequest request; raft_cmdpb::AdminResponse response; - request.mutable_compact_log(); request.set_cmd_type(::raft_cmdpb::AdminCmdType::CompactLog); ASSERT_EQ(kvs.handleAdminRaftCmd(raft_cmdpb::AdminRequest{request}, std::move(response), 1999, 22, 6, ctx.getTMTContext()), EngineStoreApplyRes::NotFound);