From acd747f28bb8f48e00844427147e06a33cc2668a Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Thu, 11 Aug 2022 19:24:48 +0800 Subject: [PATCH] Fix panic when region is not found on TiFlash's side in tryFlushData to release-6.2 (#5606) close pingcap/tiflash#5602 --- dbms/src/Storages/Transaction/KVStore.cpp | 12 +++++++++++- .../src/Storages/Transaction/tests/gtest_kvstore.cpp | 8 ++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index a4d9fab1fd0..f484b3d6644 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); } @@ -338,6 +339,15 @@ 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) + { + /// 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 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; + } return canFlushRegionDataImpl(curr_region_ptr, true, try_until_succeed, tmt, region_task_lock, index, term); } @@ -345,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; diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index 180be7e65b7..864de5b380e 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -1242,6 +1242,14 @@ 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. + // 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); } }