From fb3c483ffadd749cec6ffc1a0cb4a5b029029818 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 22 Aug 2022 18:31:39 +0800 Subject: [PATCH 1/6] Create RegionPersister with explicit PathPool instance Signed-off-by: JaySon-Huang --- dbms/src/Server/DTTool/DTTool.h | 3 +- dbms/src/Server/Server.cpp | 5 +- dbms/src/Server/tests/gtest_server_config.cpp | 3 +- dbms/src/Storages/Transaction/KVStore.cpp | 4 +- dbms/src/Storages/Transaction/KVStore.h | 3 +- .../Storages/Transaction/RegionPersister.cpp | 20 +- .../Storages/Transaction/RegionPersister.h | 3 +- dbms/src/Storages/Transaction/TMTContext.cpp | 4 +- dbms/src/Storages/Transaction/TMTContext.h | 4 +- .../Transaction/tests/gtest_kvstore.cpp | 46 ++-- .../tests/gtest_region_persister.cpp | 220 ++++++++++-------- dbms/src/TestUtils/TiFlashTestEnv.cpp | 5 +- 12 files changed, 176 insertions(+), 144 deletions(-) diff --git a/dbms/src/Server/DTTool/DTTool.h b/dbms/src/Server/DTTool/DTTool.h index 6236bd6cdb9..657399ed965 100644 --- a/dbms/src/Server/DTTool/DTTool.h +++ b/dbms/src/Server/DTTool/DTTool.h @@ -114,7 +114,8 @@ class ImitativeEnv global_context->setDeltaIndexManager(1024 * 1024 * 100 /*100MB*/); - global_context->getTMTContext().restore(); + auto & path_pool = global_context->getPathPool(); + global_context->getTMTContext().restore(path_pool); return global_context; } diff --git a/dbms/src/Server/Server.cpp b/dbms/src/Server/Server.cpp index 70bc1d3f26d..63bea66e55d 100644 --- a/dbms/src/Server/Server.cpp +++ b/dbms/src/Server/Server.cpp @@ -1229,8 +1229,9 @@ int Server::main(const std::vector & /*args*/) { if (proxy_conf.is_proxy_runnable && !tiflash_instance_wrap.proxy_helper) throw Exception("Raft Proxy Helper is not set, should not happen"); + auto & path_pool = global_context->getPathPool(); /// initialize TMTContext - global_context->getTMTContext().restore(tiflash_instance_wrap.proxy_helper); + global_context->getTMTContext().restore(path_pool, tiflash_instance_wrap.proxy_helper); } /// setting up elastic thread pool @@ -1391,4 +1392,4 @@ int mainEntryClickHouseServer(int argc, char ** argv) auto code = DB::getCurrentExceptionCode(); return code ? code : 1; } -} \ No newline at end of file +} diff --git a/dbms/src/Server/tests/gtest_server_config.cpp b/dbms/src/Server/tests/gtest_server_config.cpp index 3d0c5f7122a..b069c981bf9 100644 --- a/dbms/src/Server/tests/gtest_server_config.cpp +++ b/dbms/src/Server/tests/gtest_server_config.cpp @@ -286,9 +286,10 @@ dt_open_file_max_idle_seconds = 20 dt_page_gc_low_write_prob = 0.2 )"}; auto & global_ctx = TiFlashTestEnv::getGlobalContext(); + auto & global_path_pool = global_ctx.getPathPool(); RegionManager region_manager; RegionPersister persister(global_ctx, region_manager); - persister.restore(nullptr, PageStorage::Config{}); + persister.restore(global_path_pool, nullptr, PageStorage::Config{}); auto verify_persister_reload_config = [&global_ctx](RegionPersister & persister) { DB::Settings & settings = global_ctx.getSettingsRef(); diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index f484b3d6644..efa7c5766ab 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -50,13 +50,13 @@ KVStore::KVStore(Context & context, TiDB::SnapshotApplyMethod snapshot_apply_met // default config about compact-log: period 120s, rows 40k, bytes 32MB. } -void KVStore::restore(const TiFlashRaftProxyHelper * proxy_helper) +void KVStore::restore(PathPool & path_pool, const TiFlashRaftProxyHelper * proxy_helper) { auto task_lock = genTaskLock(); auto manage_lock = genRegionWriteLock(task_lock); this->proxy_helper = proxy_helper; - manage_lock.regions = region_persister->restore(proxy_helper); + manage_lock.regions = region_persister->restore(path_pool, proxy_helper); LOG_FMT_INFO(log, "Restored {} regions", manage_lock.regions.size()); diff --git a/dbms/src/Storages/Transaction/KVStore.h b/dbms/src/Storages/Transaction/KVStore.h index 4cce3b80f5b..1a45eab11e8 100644 --- a/dbms/src/Storages/Transaction/KVStore.h +++ b/dbms/src/Storages/Transaction/KVStore.h @@ -70,6 +70,7 @@ class ReadIndexWorkerManager; using BatchReadIndexRes = std::vector>; class ReadIndexStressTest; struct FileUsageStatistics; +class PathPool; class RegionPersister; /// TODO: brief design document. @@ -77,7 +78,7 @@ class KVStore final : private boost::noncopyable { public: KVStore(Context & context, TiDB::SnapshotApplyMethod snapshot_apply_method_); - void restore(const TiFlashRaftProxyHelper *); + void restore(PathPool & path_pool, const TiFlashRaftProxyHelper *); RegionPtr getRegion(RegionID region_id) const; diff --git a/dbms/src/Storages/Transaction/RegionPersister.cpp b/dbms/src/Storages/Transaction/RegionPersister.cpp index 8aae893efdc..bbd9bcd9687 100644 --- a/dbms/src/Storages/Transaction/RegionPersister.cpp +++ b/dbms/src/Storages/Transaction/RegionPersister.cpp @@ -215,15 +215,15 @@ void RegionPersister::forceTransformKVStoreV2toV3() page_writer->writeIntoV2(std::move(write_batch_del_v2), nullptr); } -RegionMap RegionPersister::restore(const TiFlashRaftProxyHelper * proxy_helper, PageStorage::Config config) +RegionMap RegionPersister::restore(PathPool & path_pool, const TiFlashRaftProxyHelper * proxy_helper, PageStorage::Config config) { { - auto & path_pool = global_context.getPathPool(); auto delegator = path_pool.getPSDiskDelegatorRaft(); auto provider = global_context.getFileProvider(); - auto run_mode = global_context.getPageStorageRunMode(); + const auto global_run_mode = global_context.getPageStorageRunMode(); + auto run_mode = global_run_mode; - switch (run_mode) + switch (global_run_mode) { case PageStorageRunMode::ONLY_V2: { @@ -245,8 +245,8 @@ RegionMap RegionPersister::restore(const TiFlashRaftProxyHelper * proxy_helper, config, provider); page_storage_v2->restore(); - page_writer = std::make_shared(run_mode, page_storage_v2, /*storage_v3_*/ nullptr); - page_reader = std::make_shared(run_mode, ns_id, page_storage_v2, /*storage_v3_*/ nullptr, /*readlimiter*/ global_context.getReadLimiter()); + page_writer = std::make_shared(global_run_mode, page_storage_v2, /*storage_v3_*/ nullptr); + page_reader = std::make_shared(global_run_mode, ns_id, page_storage_v2, /*storage_v3_*/ nullptr, /*readlimiter*/ global_context.getReadLimiter()); } else { @@ -270,8 +270,8 @@ RegionMap RegionPersister::restore(const TiFlashRaftProxyHelper * proxy_helper, config, provider); page_storage_v3->restore(); - page_writer = std::make_shared(run_mode, /*storage_v2_*/ nullptr, page_storage_v3); - page_reader = std::make_shared(run_mode, ns_id, /*storage_v2_*/ nullptr, page_storage_v3, global_context.getReadLimiter()); + page_writer = std::make_shared(global_run_mode, /*storage_v2_*/ nullptr, page_storage_v3); + page_reader = std::make_shared(global_run_mode, ns_id, /*storage_v2_*/ nullptr, page_storage_v3, global_context.getReadLimiter()); break; } case PageStorageRunMode::MIX_MODE: @@ -296,8 +296,8 @@ RegionMap RegionPersister::restore(const TiFlashRaftProxyHelper * proxy_helper, if (const auto & kvstore_remain_pages = page_storage_v2->getNumberOfPages(); kvstore_remain_pages != 0) { - page_writer = std::make_shared(run_mode, page_storage_v2, page_storage_v3); - page_reader = std::make_shared(run_mode, ns_id, page_storage_v2, page_storage_v3, global_context.getReadLimiter()); + page_writer = std::make_shared(global_run_mode, page_storage_v2, page_storage_v3); + page_reader = std::make_shared(global_run_mode, ns_id, page_storage_v2, page_storage_v3, global_context.getReadLimiter()); LOG_FMT_INFO(log, "Current kvstore transform to V3 begin [pages_before_transform={}]", kvstore_remain_pages); forceTransformKVStoreV2toV3(); diff --git a/dbms/src/Storages/Transaction/RegionPersister.h b/dbms/src/Storages/Transaction/RegionPersister.h index a6b400345f8..5f179488684 100644 --- a/dbms/src/Storages/Transaction/RegionPersister.h +++ b/dbms/src/Storages/Transaction/RegionPersister.h @@ -25,6 +25,7 @@ namespace DB { class Context; +class PathPool; class Region; using RegionPtr = std::shared_ptr; using RegionMap = std::unordered_map; @@ -50,7 +51,7 @@ class RegionPersister final : private boost::noncopyable void drop(RegionID region_id, const RegionTaskLock &); void persist(const Region & region); void persist(const Region & region, const RegionTaskLock & lock); - RegionMap restore(const TiFlashRaftProxyHelper * proxy_helper = nullptr, PageStorage::Config config = PageStorage::Config{}); + RegionMap restore(PathPool & path_pool, const TiFlashRaftProxyHelper * proxy_helper = nullptr, PageStorage::Config config = PageStorage::Config{}); bool gc(); using RegionCacheWriteElement = std::tuple; diff --git a/dbms/src/Storages/Transaction/TMTContext.cpp b/dbms/src/Storages/Transaction/TMTContext.cpp index 3c7468cbd64..c67e3b7755e 100644 --- a/dbms/src/Storages/Transaction/TMTContext.cpp +++ b/dbms/src/Storages/Transaction/TMTContext.cpp @@ -62,9 +62,9 @@ TMTContext::TMTContext(Context & context_, const TiFlashRaftConfig & raft_config , wait_region_ready_timeout_sec(DEFAULT_WAIT_REGION_READY_TIMEOUT_SEC) {} -void TMTContext::restore(const TiFlashRaftProxyHelper * proxy_helper) +void TMTContext::restore(PathPool & path_pool, const TiFlashRaftProxyHelper * proxy_helper) { - kvstore->restore(proxy_helper); + kvstore->restore(path_pool, proxy_helper); region_table.restore(); store_status = StoreStatus::Ready; diff --git a/dbms/src/Storages/Transaction/TMTContext.h b/dbms/src/Storages/Transaction/TMTContext.h index 8e26c0da88c..41cc3b6557c 100644 --- a/dbms/src/Storages/Transaction/TMTContext.h +++ b/dbms/src/Storages/Transaction/TMTContext.h @@ -25,6 +25,8 @@ namespace DB { class Context; +class PathPool; + class KVStore; using KVStorePtr = std::shared_ptr; @@ -85,7 +87,7 @@ class TMTContext : private boost::noncopyable MPPTaskManagerPtr getMPPTaskManager(); - void restore(const TiFlashRaftProxyHelper * proxy_helper = nullptr); + void restore(PathPool & path_pool, const TiFlashRaftProxyHelper * proxy_helper = nullptr); const std::unordered_set & getIgnoreDatabases() const; diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index 864de5b380e..3c0a18fac72 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -53,16 +54,29 @@ class RegionKVStoreTest : public ::testing::Test static void testRaftMerge(KVStore & kvs, TMTContext & tmt); static void testRaftChangePeer(KVStore & kvs, TMTContext & tmt); static void testRaftMergeRollback(KVStore & kvs, TMTContext & tmt); + + static std::unique_ptr createCleanPathPool(const String & path) + { + // Drop files on disk + Poco::File file(path); + if (file.exists()) + file.remove(true); + file.createDirectories(); + + auto & global_ctx = TiFlashTestEnv::getGlobalContext(); + auto path_capacity = global_ctx.getPathCapacity(); + auto provider = global_ctx.getFileProvider(); + // Create a PathPool instance on the clean directory + Strings main_data_paths{path}; + return std::make_unique(main_data_paths, main_data_paths, Strings{}, path_capacity, provider); + }; }; void RegionKVStoreTest::testNewProxy() { std::string path = TiFlashTestEnv::getTemporaryPath("/region_kvs_tmp") + "/basic"; - Poco::File file(path); - if (file.exists()) - file.remove(true); - file.createDirectories(); + auto path_pool = createCleanPathPool(path); auto ctx = TiFlashTestEnv::getContext( DB::Settings(), @@ -76,7 +90,7 @@ void RegionKVStoreTest::testNewProxy() proxy_helper = MockRaftStoreProxy::SetRaftStoreProxyFFIHelper(RaftStoreProxyPtr{&proxy_instance}); proxy_instance.init(100); } - kvs.restore(&proxy_helper); + kvs.restore(*path_pool, &proxy_helper); { auto store = metapb::Store{}; store.set_id(1234); @@ -119,10 +133,7 @@ void RegionKVStoreTest::testReadIndex() { std::string path = TiFlashTestEnv::getTemporaryPath("/region_kvs_tmp") + "/basic"; - Poco::File file(path); - if (file.exists()) - file.remove(true); - file.createDirectories(); + auto path_pool = createCleanPathPool(path); auto ctx = TiFlashTestEnv::getContext( DB::Settings(), @@ -142,7 +153,7 @@ void RegionKVStoreTest::testReadIndex() proxy_instance.testRunNormal(over); }); KVStore & kvs = *ctx.getTMTContext().getKVStore(); - kvs.restore(&proxy_helper); + kvs.restore(*path_pool, &proxy_helper); ASSERT_EQ(kvs.getProxyHelper(), &proxy_helper); { ASSERT_EQ(kvs.getRegion(0), nullptr); @@ -828,16 +839,11 @@ void RegionKVStoreTest::testKVStore() { std::string path = TiFlashTestEnv::getTemporaryPath("/region_kvs_tmp") + "/basic"; - Poco::File file(path); - if (file.exists()) - file.remove(true); - file.createDirectories(); + auto path_pool = createCleanPathPool(path); - auto ctx = TiFlashTestEnv::getContext( - DB::Settings(), - Strings{ - path, - }); + auto ctx = TiFlashTestEnv::getContext(DB::Settings(), Strings{ + path, + }); KVStore & kvs = *ctx.getTMTContext().getKVStore(); MockRaftStoreProxy proxy_instance; TiFlashRaftProxyHelper proxy_helper; @@ -845,7 +851,7 @@ void RegionKVStoreTest::testKVStore() proxy_helper = MockRaftStoreProxy::SetRaftStoreProxyFFIHelper(RaftStoreProxyPtr{&proxy_instance}); proxy_instance.init(100); } - kvs.restore(&proxy_helper); + kvs.restore(*path_pool, &proxy_helper); { // Run without read-index workers diff --git a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp index 963e3a3571d..dae6cc2c6c3 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp @@ -17,16 +17,19 @@ #include #include #include +#include #include #include #include #include +#include #include +#include #include #include -#include "region_helper.h" +#include "common/logger_useful.h" namespace DB { @@ -38,39 +41,6 @@ extern const char force_disable_region_persister_compatible_mode[]; namespace tests { -class RegionPersister_test : public ::testing::Test -{ -public: - RegionPersister_test() - : dir_path(TiFlashTestEnv::getTemporaryPath("/region_persister_tmp")) - {} - - static void SetUpTestCase() {} - - void SetUp() override { dropFiles(); } - - void dropFiles() - { - // cleanup - Poco::File file(dir_path); - if (file.exists()) - file.remove(true); - file.createDirectories(); - } - - void runTest(const String & path, bool sync_on_write); - void testFunc(const String & path, const PageStorage::Config & config, int region_num, bool is_gc, bool clean_up); - -protected: - String dir_path; - - DB::Timestamp tso = 0; - - String getPageStorageV3MetaPath(String & path) - { - return path + "/page/kvstore/wal/log_1_0"; - } -}; static ::testing::AssertionResult PeerCompare( const char * lhs_expr, @@ -98,16 +68,16 @@ static ::testing::AssertionResult RegionCompare( } #define ASSERT_REGION_EQ(val1, val2) ASSERT_PRED_FORMAT2(::DB::tests::RegionCompare, val1, val2) -TEST_F(RegionPersister_test, peer) +TEST(RegionSeriTest, peer) try { auto peer = createPeer(100, true); - auto path = dir_path + "/peer.test"; + const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/peer.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); auto size = writeBinary2(peer, write_buf); write_buf.next(); write_buf.sync(); - ASSERT_EQ(size, (size_t)Poco::File(path).getSize()); + ASSERT_EQ(size, Poco::File(path).getSize()); ReadBufferFromFile read_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_RDONLY); auto new_peer = readPeer(read_buf); @@ -115,11 +85,11 @@ try } CATCH -TEST_F(RegionPersister_test, region_info) +TEST(RegionSeriTest, RegionInfo) try { auto region_info = createRegionInfo(233, "", ""); - auto path = dir_path + "/region_info.test"; + const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/region_info.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); auto size = writeBinary2(region_info, write_buf); write_buf.next(); @@ -137,11 +107,11 @@ try } CATCH -TEST_F(RegionPersister_test, region_meta) +TEST(RegionSeriTest, RegionMeta) try { RegionMeta meta = createRegionMeta(888, 66); - auto path = dir_path + "/meta.test"; + const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/meta.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); auto size = std::get<0>(meta.serialize(write_buf)); write_buf.next(); @@ -154,7 +124,7 @@ try } CATCH -TEST_F(RegionPersister_test, region) +TEST(RegionSeriTest, Region) try { TableID table_id = 100; @@ -164,7 +134,7 @@ try region->insert("write", TiKVKey::copyFrom(key), RecordKVFormat::encodeWriteCfValue('P', 0)); region->insert("lock", TiKVKey::copyFrom(key), RecordKVFormat::encodeLockCfValue('P', "", 0, 0)); - auto path = dir_path + "/region.test"; + const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/region.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); size_t region_ser_size = std::get<0>(region->serialize(write_buf)); write_buf.next(); @@ -177,7 +147,7 @@ try } CATCH -TEST_F(RegionPersister_test, region_stat) +TEST(RegionSeriTest, RegionStat) try { RegionPtr region = nullptr; @@ -207,7 +177,7 @@ try region->insert("write", TiKVKey::copyFrom(key), RecordKVFormat::encodeWriteCfValue('P', 0)); region->insert("lock", TiKVKey::copyFrom(key), RecordKVFormat::encodeLockCfValue('P', "", 0, 0)); - auto path = dir_path + "/region_state.test"; + const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/region_state.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); size_t region_ser_size = std::get<0>(region->serialize(write_buf)); write_buf.next(); @@ -219,29 +189,79 @@ try } CATCH -TEST_F(RegionPersister_test, persister) + +class RegionPersisterTest : public ::testing::Test +{ +public: + RegionPersisterTest() + : dir_path(TiFlashTestEnv::getTemporaryPath("/region_persister_test")) + { + } + + static void SetUpTestCase() {} + + void SetUp() override + { + dropFiles(); + + auto & global_ctx = TiFlashTestEnv::getGlobalContext(); + auto path_capacity = global_ctx.getPathCapacity(); + auto provider = global_ctx.getFileProvider(); + + Strings main_data_paths{dir_path}; + mocked_path_pool = std::make_unique( + main_data_paths, + main_data_paths, + /*kvstore_paths=*/Strings{}, + path_capacity, + provider, + /*enable_raft_compatible_mode_=*/true); + } + + void dropFiles() + { + // cleanup + Poco::File file(dir_path); + if (file.exists()) + file.remove(true); + file.createDirectories(); + } + + void runTest(const String & path, bool sync_on_write); + void testFunc(const String & path, const PageStorage::Config & config, int region_num, bool is_gc, bool clean_up); + +protected: + String dir_path; + + DB::Timestamp tso = 0; + + static String getPageStorageV3MetaPath(String & path) + { + return path + "/page/kvstore/wal/log_1_0"; + } + + std::unique_ptr mocked_path_pool; +}; + +TEST_F(RegionPersisterTest, persister) try { RegionManager region_manager; - std::string path = dir_path + "/broken_file"; - - auto ctx = TiFlashTestEnv::getContext(DB::Settings(), - Strings{ - path, - }); + auto ctx = TiFlashTestEnv::getGlobalContext(); size_t region_num = 100; RegionMap regions; - TableID table_id = 100; + const TableID table_id = 100; PageStorage::Config config; config.file_roll_size = 128 * MB; { UInt64 diff = 0; RegionPersister persister(ctx, region_manager); - persister.restore(nullptr, config); + persister.restore(*mocked_path_pool, nullptr, config); + // Persist region by region for (size_t i = 0; i < region_num; ++i) { auto region = std::make_shared(createRegionMeta(i, table_id)); @@ -254,26 +274,30 @@ try regions.emplace(region->id(), region); } + } - // If we truncate page data file, exception will throw instead of droping last region. - auto meta_path = getPageStorageV3MetaPath(path); // First page + { + // Truncate the last byte of the meta to mock that the last region persist is not completed + auto meta_path = dir_path + "/page/kvstore/wal/log_1_0"; // First page Poco::File meta_file(meta_path); size_t size = meta_file.getSize(); - int rt = ::truncate(meta_path.c_str(), size - 1); // Remove last one byte - ASSERT_EQ(rt, 0); + int ret = ::truncate(meta_path.c_str(), size - 1); // Remove last one byte + ASSERT_EQ(ret, 0); } RegionMap new_regions; { RegionPersister persister(ctx, region_manager); - new_regions = persister.restore(nullptr, config); + new_regions = persister.restore(*mocked_path_pool, nullptr, config); + + // check that only the last region (which write is not completed) is thrown away size_t num_regions_missed = 0; for (size_t i = 0; i < region_num; ++i) { auto new_iter = new_regions.find(i); if (new_iter == new_regions.end()) { - LOG_FMT_ERROR(&Poco::Logger::get("RegionPersister_test"), "Region missed, id={}", i); + LOG_FMT_ERROR(&Poco::Logger::get("RegionPersisterTest"), "Region missed, id={}", i); ++num_regions_missed; } else @@ -283,27 +307,22 @@ try ASSERT_EQ(*new_region, *old_region); } } - ASSERT_EQ(num_regions_missed, 1UL); + ASSERT_EQ(num_regions_missed, 1); } } CATCH -TEST_F(RegionPersister_test, persister_compatible_mode) +TEST_F(RegionPersisterTest, persisterPSVersionUpgrade) try { - std::string path = dir_path + "/compatible_mode"; - - auto current_storage_run_mode = TiFlashTestEnv::getGlobalContext().getPageStorageRunMode(); - // Force to run in compatible mode for the default region persister - FailPointHelper::enableFailPoint(FailPoints::force_enable_region_persister_compatible_mode); - SCOPE_EXIT( - { FailPointHelper::disableFailPoint(FailPoints::force_enable_region_persister_compatible_mode); - TiFlashTestEnv::getGlobalContext().setPageStorageRunMode(current_storage_run_mode); }); - TiFlashTestEnv::getGlobalContext().setPageStorageRunMode(PageStorageRunMode::ONLY_V2); - auto ctx = TiFlashTestEnv::getContext(DB::Settings(), - Strings{ - path, - }); + auto & global_ctx = TiFlashTestEnv::getGlobalContext(); + auto saved_storage_run_mode = global_ctx.getPageStorageRunMode(); + global_ctx.setPageStorageRunMode(PageStorageRunMode::ONLY_V2); + // Force to run in ps v1 mode for the default region persister + SCOPE_EXIT({ + FailPointHelper::disableFailPoint(FailPoints::force_enable_region_persister_compatible_mode); + global_ctx.setPageStorageRunMode(saved_storage_run_mode); + }); size_t region_num = 500; RegionMap regions; @@ -314,13 +333,13 @@ try RegionManager region_manager; DB::Timestamp tso = 0; { - RegionPersister persister(ctx, region_manager); - // Force to run in compatible mode + RegionPersister persister(global_ctx, region_manager); + // Force to run in ps v1 mode FailPointHelper::enableFailPoint(FailPoints::force_enable_region_persister_compatible_mode); - persister.restore(nullptr, config); + persister.restore(*mocked_path_pool, nullptr, config); ASSERT_EQ(persister.page_writer, nullptr); ASSERT_EQ(persister.page_reader, nullptr); - ASSERT_NE(persister.stable_page_storage, nullptr); + ASSERT_NE(persister.stable_page_storage, nullptr); // ps v1 for (size_t i = 0; i < region_num; ++i) { @@ -334,15 +353,16 @@ try regions.emplace(region->id(), region); } + LOG_DEBUG(&Poco::Logger::get("fff"), "v1 write done"); } { - RegionPersister persister(ctx, region_manager); - // restore normally, should run in compatible mode. - RegionMap new_regions = persister.restore(nullptr, config); + RegionPersister persister(global_ctx, region_manager); + // restore normally, should run in ps v1 mode. + RegionMap new_regions = persister.restore(*mocked_path_pool, nullptr, config); ASSERT_EQ(persister.page_writer, nullptr); ASSERT_EQ(persister.page_reader, nullptr); - ASSERT_NE(persister.stable_page_storage, nullptr); + ASSERT_NE(persister.stable_page_storage, nullptr); // ps v1 // Try to read for (size_t i = 0; i < region_num; ++i) { @@ -356,10 +376,10 @@ try size_t region_num_under_nromal_mode = 200; { - RegionPersister persister(ctx, region_manager); - // Force to run in normal mode + RegionPersister persister(global_ctx, region_manager); + // Force to run in ps v2 mode FailPointHelper::enableFailPoint(FailPoints::force_disable_region_persister_compatible_mode); - RegionMap new_regions = persister.restore(nullptr, config); + RegionMap new_regions = persister.restore(*mocked_path_pool, nullptr, config); ASSERT_NE(persister.page_writer, nullptr); ASSERT_NE(persister.page_reader, nullptr); ASSERT_EQ(persister.stable_page_storage, nullptr); @@ -372,7 +392,7 @@ try auto new_region = new_regions[i]; ASSERT_EQ(*new_region, *old_region); } - // Try to write more regions under normal mode + // Try to write more regions under ps v2 mode for (size_t i = region_num; i < region_num + region_num_under_nromal_mode; ++i) { auto region = std::make_shared(createRegionMeta(i, table_id)); @@ -388,9 +408,9 @@ try } { - RegionPersister persister(ctx, region_manager); - // Restore normally, should run in normal mode. - RegionMap new_regions = persister.restore(nullptr, config); + RegionPersister persister(global_ctx, region_manager); + // Restore normally, should run in ps v2 mode. + RegionMap new_regions = persister.restore(*mocked_path_pool, nullptr, config); ASSERT_NE(persister.page_writer, nullptr); ASSERT_NE(persister.page_reader, nullptr); ASSERT_EQ(persister.stable_page_storage, nullptr); @@ -408,19 +428,16 @@ try CATCH -void RegionPersister_test::testFunc(const String & path, const PageStorage::Config & config, int region_num, bool is_gc, bool clean_up) +void RegionPersisterTest::testFunc(const String & /*path*/, const PageStorage::Config & config, int region_num, bool is_gc, bool clean_up) { if (clean_up) dropFiles(); - auto ctx = TiFlashTestEnv::getContext(DB::Settings(), - Strings{ - path, - }); + auto & global_ctx = TiFlashTestEnv::getGlobalContext(); RegionManager region_manager; - RegionPersister persister(ctx, region_manager); - persister.restore(nullptr, config); + RegionPersister persister(global_ctx, region_manager); + persister.restore(*mocked_path_pool, nullptr, config); TableID table_id = 100; RegionMap regions; @@ -441,7 +458,7 @@ void RegionPersister_test::testFunc(const String & path, const PageStorage::Conf persister.gc(); RegionMap new_regions; - new_regions = persister.restore(nullptr, config); + new_regions = persister.restore(*mocked_path_pool, nullptr, config); for (int i = 0; i < region_num; ++i) { @@ -454,7 +471,7 @@ void RegionPersister_test::testFunc(const String & path, const PageStorage::Conf dropFiles(); } -void RegionPersister_test::runTest(const String & path, bool sync_on_write) +void RegionPersisterTest::runTest(const String & path, bool sync_on_write) { Stopwatch watch; @@ -536,11 +553,12 @@ void RegionPersister_test::runTest(const String & path, bool sync_on_write) } auto seconds = watch.elapsedSeconds(); - LOG_FMT_INFO(&Poco::Logger::get("RegionPersister_test"), "[sync_on_write={}], [time={:.4f}s]", sync_on_write, seconds); + LOG_FMT_INFO(&Poco::Logger::get("RegionPersisterTest"), "[sync_on_write={}], [time={:.4f}s]", sync_on_write, seconds); } +// TODO: check whether we still need this test case // This test takes about 10 minutes. Disable by default -TEST_F(RegionPersister_test, DISABLED_persister_sync_on_write) +TEST_F(RegionPersisterTest, DISABLED_persister_sync_on_write) { runTest(dir_path + "region_persist_storage_sow_false", false); runTest(dir_path + "region_persist_storage_sow_true", true); diff --git a/dbms/src/TestUtils/TiFlashTestEnv.cpp b/dbms/src/TestUtils/TiFlashTestEnv.cpp index 0d10e1c20d9..5ef80f48872 100644 --- a/dbms/src/TestUtils/TiFlashTestEnv.cpp +++ b/dbms/src/TestUtils/TiFlashTestEnv.cpp @@ -69,7 +69,7 @@ void TiFlashTestEnv::initializeGlobalContext(Strings testdata_path, PageStorageR paths.first, paths.second, Strings{}, - true, + /*enable_raft_compatible_mode=*/true, global_context->getPathCapacity(), global_context->getFileProvider()); @@ -85,7 +85,8 @@ void TiFlashTestEnv::initializeGlobalContext(Strings testdata_path, PageStorageR global_context->setDeltaIndexManager(1024 * 1024 * 100 /*100MB*/); - global_context->getTMTContext().restore(); + auto & path_pool = global_context->getPathPool(); + global_context->getTMTContext().restore(path_pool); } Context TiFlashTestEnv::getContext(const DB::Settings & settings, Strings testdata_path) From 02b3138e68bd22a66cdc76ddd8630d06f8a81f6d Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 22 Aug 2022 18:33:39 +0800 Subject: [PATCH 2/6] Remove useless test case Signed-off-by: JaySon-Huang --- .../tests/gtest_region_persister.cpp | 137 ------------------ 1 file changed, 137 deletions(-) diff --git a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp index dae6cc2c6c3..13cb7e9162c 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp @@ -427,142 +427,5 @@ try } CATCH - -void RegionPersisterTest::testFunc(const String & /*path*/, const PageStorage::Config & config, int region_num, bool is_gc, bool clean_up) -{ - if (clean_up) - dropFiles(); - - auto & global_ctx = TiFlashTestEnv::getGlobalContext(); - - RegionManager region_manager; - RegionPersister persister(global_ctx, region_manager); - persister.restore(*mocked_path_pool, nullptr, config); - - TableID table_id = 100; - RegionMap regions; - for (int i = 0; i < region_num; ++i) - { - auto region = std::make_shared(createRegionMeta(i, table_id)); - TiKVKey key = RecordKVFormat::genKey(table_id, i, tso++); - region->insert("default", TiKVKey::copyFrom(key), TiKVValue("value1")); - region->insert("write", TiKVKey::copyFrom(key), RecordKVFormat::encodeWriteCfValue('P', 0)); - region->insert("lock", TiKVKey::copyFrom(key), RecordKVFormat::encodeLockCfValue('P', "", 0, 0)); - - persister.persist(*region); - - regions.emplace(region->id(), region); - } - - if (is_gc) - persister.gc(); - - RegionMap new_regions; - new_regions = persister.restore(*mocked_path_pool, nullptr, config); - - for (int i = 0; i < region_num; ++i) - { - auto old_region = regions[i]; - auto new_region = new_regions[i]; - ASSERT_EQ(*new_region, *old_region); - } - - if (clean_up) - dropFiles(); -} - -void RegionPersisterTest::runTest(const String & path, bool sync_on_write) -{ - Stopwatch watch; - - dropFiles(); - SCOPE_EXIT({ dropFiles(); }); - - { - PageStorage::Config conf; - conf.sync_on_write = sync_on_write; - conf.file_roll_size = 1; - conf.gc_min_bytes = 1; - conf.num_write_slots = 4; - - testFunc(path, conf, 10, false, false); - testFunc(path, conf, 10, true, false); - - testFunc(path, conf, 10, false, true); - testFunc(path, conf, 10, true, true); - } - { - PageStorage::Config conf; - conf.sync_on_write = sync_on_write; - conf.file_roll_size = 500; - conf.gc_min_bytes = 1; - conf.num_write_slots = 4; - - testFunc(path, conf, 100, false, false); - testFunc(path, conf, 100, false, false); - testFunc(path, conf, 100, false, false); - testFunc(path, conf, 100, false, true); - - testFunc(path, conf, 100, true, false); - testFunc(path, conf, 100, true, false); - testFunc(path, conf, 100, true, false); - testFunc(path, conf, 100, true, false); - } - { - PageStorage::Config conf; - conf.sync_on_write = sync_on_write; - conf.file_roll_size = 500; - conf.gc_min_bytes = 1; - conf.num_write_slots = 4; - - testFunc(path, conf, 100, false, false); - testFunc(path, conf, 100, false, false); - testFunc(path, conf, 100, false, false); - testFunc(path, conf, 100, false, true); - - testFunc(path, conf, 100, true, false); - testFunc(path, conf, 100, true, false); - testFunc(path, conf, 100, true, false); - testFunc(path, conf, 100, true, false); - } - { - PageStorage::Config conf; - conf.sync_on_write = sync_on_write; - conf.num_write_slots = 4; - - testFunc(path, conf, 10000, false, false); - testFunc(path, conf, 10000, false, false); - testFunc(path, conf, 10000, false, false); - testFunc(path, conf, 10000, false, false); - testFunc(path, conf, 10000, false, false); - testFunc(path, conf, 10000, false, false); - testFunc(path, conf, 10000, false, false); - } - { - PageStorage::Config conf; - conf.sync_on_write = sync_on_write; - conf.num_write_slots = 4; - - testFunc(path, conf, 10000, true, false); - testFunc(path, conf, 10000, true, false); - testFunc(path, conf, 10000, true, false); - testFunc(path, conf, 10000, true, false); - testFunc(path, conf, 10000, true, false); - testFunc(path, conf, 10000, true, false); - testFunc(path, conf, 10000, true, false); - } - - auto seconds = watch.elapsedSeconds(); - LOG_FMT_INFO(&Poco::Logger::get("RegionPersisterTest"), "[sync_on_write={}], [time={:.4f}s]", sync_on_write, seconds); -} - -// TODO: check whether we still need this test case -// This test takes about 10 minutes. Disable by default -TEST_F(RegionPersisterTest, DISABLED_persister_sync_on_write) -{ - runTest(dir_path + "region_persist_storage_sow_false", false); - runTest(dir_path + "region_persist_storage_sow_true", true); -} - } // namespace tests } // namespace DB From 540c06ad742b5d8ab2189e409123e3d88c6cf33b Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 22 Aug 2022 20:51:28 +0800 Subject: [PATCH 3/6] Refactor kvstore test Signed-off-by: JaySon-Huang --- dbms/src/Debug/MockSSTReader.h | 10 +- dbms/src/Debug/dbgFuncMockRaftSnapshot.cpp | 8 +- dbms/src/Server/Server.cpp | 5 +- dbms/src/Storages/Transaction/KVStore.cpp | 15 +- dbms/src/Storages/Transaction/KVStore.h | 4 +- .../Transaction/tests/gtest_kvstore.cpp | 206 ++++++++++++------ .../tests/gtest_region_persister.cpp | 3 +- 7 files changed, 162 insertions(+), 89 deletions(-) diff --git a/dbms/src/Debug/MockSSTReader.h b/dbms/src/Debug/MockSSTReader.h index 99e166dc9ce..50abdbab635 100644 --- a/dbms/src/Debug/MockSSTReader.h +++ b/dbms/src/Debug/MockSSTReader.h @@ -43,7 +43,7 @@ struct MockSSTReader Data() = default; }; - MockSSTReader(const Data & data_) + explicit MockSSTReader(const Data & data_) : iter(data_.begin()) , end(data_.end()) , remained(iter != end) @@ -70,16 +70,18 @@ struct MockSSTReader }; -class RegionMockTest +class RegionMockTest final { public: - RegionMockTest(KVStorePtr kvstore_, RegionPtr region_); + RegionMockTest(KVStore * kvstore_, RegionPtr region_); ~RegionMockTest(); + DISALLOW_COPY_AND_MOVE(RegionMockTest); + private: TiFlashRaftProxyHelper mock_proxy_helper{}; const TiFlashRaftProxyHelper * ori_proxy_helper{}; - KVStorePtr kvstore; + KVStore * kvstore; RegionPtr region; }; } // namespace DB diff --git a/dbms/src/Debug/dbgFuncMockRaftSnapshot.cpp b/dbms/src/Debug/dbgFuncMockRaftSnapshot.cpp index b5d3f252d0a..b4c83e33127 100644 --- a/dbms/src/Debug/dbgFuncMockRaftSnapshot.cpp +++ b/dbms/src/Debug/dbgFuncMockRaftSnapshot.cpp @@ -290,7 +290,7 @@ void fn_gc(SSTReaderPtr ptr, ColumnFamilyType) delete reader; } -RegionMockTest::RegionMockTest(KVStorePtr kvstore_, RegionPtr region_) +RegionMockTest::RegionMockTest(KVStore * kvstore_, RegionPtr region_) : kvstore(kvstore_) , region(region_) { @@ -465,7 +465,7 @@ void MockRaftCommand::dbgFuncIngestSST(Context & context, const ASTs & args, DBG FailPointHelper::enableFailPoint(FailPoints::force_set_sst_decode_rand); // Register some mock SST reading methods so that we can decode data in `MockSSTReader::MockSSTData` - RegionMockTest mock_test(kvstore, region); + RegionMockTest mock_test(kvstore.get(), region); { // Mocking ingest a SST for column family "Write" @@ -646,7 +646,7 @@ void MockRaftCommand::dbgFuncRegionSnapshotPreHandleDTFiles(Context & context, c RegionPtr new_region = RegionBench::createRegion(table->id(), region_id, start_handle, end_handle + 10000, index); // Register some mock SST reading methods so that we can decode data in `MockSSTReader::MockSSTData` - RegionMockTest mock_test(kvstore, new_region); + RegionMockTest mock_test(kvstore.get(), new_region); std::vector sst_views; { @@ -743,7 +743,7 @@ void MockRaftCommand::dbgFuncRegionSnapshotPreHandleDTFilesWithHandles(Context & RegionPtr new_region = RegionBench::createRegion(table->id(), region_id, region_start_handle, region_end_handle, index); // Register some mock SST reading methods so that we can decode data in `MockSSTReader::MockSSTData` - RegionMockTest mock_test(kvstore, new_region); + RegionMockTest mock_test(kvstore.get(), new_region); std::vector sst_views; { diff --git a/dbms/src/Server/Server.cpp b/dbms/src/Server/Server.cpp index 63bea66e55d..cb3d3d9e55e 100644 --- a/dbms/src/Server/Server.cpp +++ b/dbms/src/Server/Server.cpp @@ -1308,14 +1308,15 @@ int Server::main(const std::vector & /*args*/) assert(tiflash_instance_wrap.proxy_helper->getProxyStatus() == RaftProxyStatus::Running); LOG_FMT_INFO(log, "store {}, tiflash proxy is ready to serve, try to wake up all regions' leader", tmt_context.getKVStore()->getStoreID(std::memory_order_seq_cst)); size_t runner_cnt = config().getUInt("flash.read_index_runner_count", 1); // if set 0, DO NOT enable read-index worker - tmt_context.getKVStore()->initReadIndexWorkers( + auto & kvstore_ptr = tmt_context.getKVStore(); + kvstore_ptr->initReadIndexWorkers( [&]() { // get from tmt context return std::chrono::milliseconds(tmt_context.readIndexWorkerTick()); }, /*running thread count*/ runner_cnt); tmt_context.getKVStore()->asyncRunReadIndexWorkers(); - WaitCheckRegionReady(tmt_context, terminate_signals_counter); + WaitCheckRegionReady(tmt_context, *kvstore_ptr, terminate_signals_counter); } SCOPE_EXIT({ if (proxy_conf.is_proxy_runnable && tiflash_instance_wrap.status != EngineStoreServerStatus::Running) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index efa7c5766ab..8ef436b0739 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -625,6 +625,7 @@ EngineStoreApplyRes KVStore::handleAdminRaftCmd(raft_cmdpb::AdminRequest && requ void WaitCheckRegionReady( const TMTContext & tmt, + KVStore & kvstore, const std::atomic_size_t & terminate_signals_counter, double wait_tick_time, double max_wait_tick_time, @@ -644,7 +645,7 @@ void WaitCheckRegionReady( Stopwatch region_check_watch; size_t total_regions_cnt = 0; { - tmt.getKVStore()->traverseRegions([&remain_regions](RegionID region_id, const RegionPtr &) { remain_regions.emplace(region_id); }); + kvstore.traverseRegions([&remain_regions](RegionID region_id, const RegionPtr &) { remain_regions.emplace(region_id); }); total_regions_cnt = remain_regions.size(); } while (region_check_watch.elapsedSeconds() < get_wait_region_ready_timeout_sec * batch_read_index_time_rate @@ -654,7 +655,7 @@ void WaitCheckRegionReady( for (auto it = remain_regions.begin(); it != remain_regions.end();) { auto region_id = *it; - if (auto region = tmt.getKVStore()->getRegion(region_id); region) + if (auto region = kvstore.getRegion(region_id); region) { batch_read_index_req.emplace_back(GenRegionReadIndexReq(*region)); it++; @@ -664,7 +665,7 @@ void WaitCheckRegionReady( it = remain_regions.erase(it); } } - auto read_index_res = tmt.getKVStore()->batchReadIndex(batch_read_index_req, tmt.batchReadIndexTimeout()); + auto read_index_res = kvstore.batchReadIndex(batch_read_index_req, tmt.batchReadIndexTimeout()); for (auto && [resp, region_id] : read_index_res) { bool need_retry = resp.read_index() == 0; @@ -716,7 +717,7 @@ void WaitCheckRegionReady( for (auto it = regions_to_check.begin(); it != regions_to_check.end();) { auto [region_id, latest_index] = *it; - if (auto region = tmt.getKVStore()->getRegion(region_id); region) + if (auto region = kvstore.getRegion(region_id); region) { if (region->appliedIndex() >= latest_index) { @@ -752,7 +753,7 @@ void WaitCheckRegionReady( regions_to_check.begin(), regions_to_check.end(), [&](const auto & e, FmtBuffer & b) { - if (auto r = tmt.getKVStore()->getRegion(e.first); r) + if (auto r = kvstore.getRegion(e.first); r) { b.fmtAppend("{},{},{}", e.first, e.second, r->appliedIndex()); } @@ -771,14 +772,14 @@ void WaitCheckRegionReady( region_check_watch.elapsedSeconds()); } -void WaitCheckRegionReady(const TMTContext & tmt, const std::atomic_size_t & terminate_signals_counter) +void WaitCheckRegionReady(const TMTContext & tmt, KVStore & kvstore, const std::atomic_size_t & terminate_signals_counter) { // wait interval to check region ready, not recommended to modify only if for tesing auto wait_region_ready_tick = tmt.getContext().getConfigRef().getUInt64("flash.wait_region_ready_tick", 0); auto wait_region_ready_timeout_sec = static_cast(tmt.waitRegionReadyTimeout()); const double max_wait_tick_time = 0 == wait_region_ready_tick ? 20.0 : wait_region_ready_timeout_sec; double min_wait_tick_time = 0 == wait_region_ready_tick ? 2.5 : static_cast(wait_region_ready_tick); // default tick in TiKV is about 2s (without hibernate-region) - return WaitCheckRegionReady(tmt, terminate_signals_counter, min_wait_tick_time, max_wait_tick_time, wait_region_ready_timeout_sec); + return WaitCheckRegionReady(tmt, kvstore, terminate_signals_counter, min_wait_tick_time, max_wait_tick_time, wait_region_ready_timeout_sec); } void KVStore::setStore(metapb::Store store_) diff --git a/dbms/src/Storages/Transaction/KVStore.h b/dbms/src/Storages/Transaction/KVStore.h index 1a45eab11e8..9c1cea1f0ba 100644 --- a/dbms/src/Storages/Transaction/KVStore.h +++ b/dbms/src/Storages/Transaction/KVStore.h @@ -276,7 +276,7 @@ class KVStoreTaskLock : private boost::noncopyable std::lock_guard lock; }; -void WaitCheckRegionReady(const TMTContext &, const std::atomic_size_t & terminate_signals_counter); -void WaitCheckRegionReady(const TMTContext &, const std::atomic_size_t &, double, double, double); +void WaitCheckRegionReady(const TMTContext &, KVStore & kvstore, const std::atomic_size_t & terminate_signals_counter); +void WaitCheckRegionReady(const TMTContext &, KVStore & kvstore, const std::atomic_size_t &, double, double, double); } // namespace DB diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index 3c0a18fac72..6b063e0014a 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -16,11 +16,16 @@ #include #include #include +#include #include #include +#include #include #include #include +#include + +#include namespace DB { @@ -37,17 +42,59 @@ extern void ChangeRegionStateRange(RegionState & region_state, bool source_at_le namespace tests { + +// TODO: Use another way to workaround calling the private methods on KVStore class RegionKVStoreTest : public ::testing::Test { public: RegionKVStoreTest() = default; - static void SetUpTestCase() {} + static void SetUpTestCase() + { + test_path = TiFlashTestEnv::getTemporaryPath("/region_kvs_test"); + } + + void SetUp() override + { + // clean data and create path pool instance + path_pool = createCleanPathPool(test_path); + + reloadKVSFromDisk(); + + proxy_instance = std::make_unique(); + proxy_helper = std::make_unique(MockRaftStoreProxy::SetRaftStoreProxyFFIHelper( + RaftStoreProxyPtr{proxy_instance.get()})); + proxy_instance->init(100); + + kvstore->restore(*path_pool, proxy_helper.get()); + } + + void TearDown() override + { + proxy_helper.reset(); + proxy_instance.reset(); + kvstore.reset(); + path_pool.reset(); + } + static void testBasic(); static void testKVStore(); static void testRegion(); static void testReadIndex(); static void testNewProxy(); + static void testKVStoreRestore(); + +protected: + static KVStore & getKVS() { return *kvstore; } + static KVStore & reloadKVSFromDisk() + { + kvstore.reset(); + auto & global_ctx = TiFlashTestEnv::getGlobalContext(); + kvstore = std::make_unique(global_ctx, TiDB::SnapshotApplyMethod::DTFile_Directory); + // TODO: Do we need to recreate proxy_instance and proxy_helper here? + kvstore->restore(*path_pool, proxy_helper.get()); + return *kvstore; + } private: static void testRaftSplit(KVStore & kvs, TMTContext & tmt); @@ -69,28 +116,28 @@ class RegionKVStoreTest : public ::testing::Test // Create a PathPool instance on the clean directory Strings main_data_paths{path}; return std::make_unique(main_data_paths, main_data_paths, Strings{}, path_capacity, provider); - }; + } + + static std::string test_path; + + static std::unique_ptr path_pool; + static std::unique_ptr kvstore; + + static std::unique_ptr proxy_instance; + static std::unique_ptr proxy_helper; }; +std::string RegionKVStoreTest::test_path; +std::unique_ptr RegionKVStoreTest::path_pool; +std::unique_ptr RegionKVStoreTest::kvstore; +std::unique_ptr RegionKVStoreTest::proxy_instance; +std::unique_ptr RegionKVStoreTest::proxy_helper; + void RegionKVStoreTest::testNewProxy() { - std::string path = TiFlashTestEnv::getTemporaryPath("/region_kvs_tmp") + "/basic"; + auto ctx = TiFlashTestEnv::getGlobalContext(); - auto path_pool = createCleanPathPool(path); - - auto ctx = TiFlashTestEnv::getContext( - DB::Settings(), - Strings{ - path, - }); - KVStore & kvs = *ctx.getTMTContext().getKVStore(); - MockRaftStoreProxy proxy_instance; - TiFlashRaftProxyHelper proxy_helper; - { - proxy_helper = MockRaftStoreProxy::SetRaftStoreProxyFFIHelper(RaftStoreProxyPtr{&proxy_instance}); - proxy_instance.init(100); - } - kvs.restore(*path_pool, &proxy_helper); + KVStore & kvs = getKVS(); { auto store = metapb::Store{}; store.set_id(1234); @@ -131,30 +178,16 @@ void RegionKVStoreTest::testNewProxy() void RegionKVStoreTest::testReadIndex() { - std::string path = TiFlashTestEnv::getTemporaryPath("/region_kvs_tmp") + "/basic"; - - auto path_pool = createCleanPathPool(path); - - auto ctx = TiFlashTestEnv::getContext( - DB::Settings(), - Strings{ - path, - }); - MockRaftStoreProxy proxy_instance; - TiFlashRaftProxyHelper proxy_helper; - { - proxy_helper = MockRaftStoreProxy::SetRaftStoreProxyFFIHelper(RaftStoreProxyPtr{&proxy_instance}); - proxy_instance.init(10); - } - std::atomic_bool over{false}; + auto ctx = TiFlashTestEnv::getGlobalContext(); // start mock proxy in other thread + std::atomic_bool over{false}; auto proxy_runner = std::thread([&]() { - proxy_instance.testRunNormal(over); + proxy_instance->testRunNormal(over); }); - KVStore & kvs = *ctx.getTMTContext().getKVStore(); - kvs.restore(*path_pool, &proxy_helper); - ASSERT_EQ(kvs.getProxyHelper(), &proxy_helper); + KVStore & kvs = getKVS(); + ASSERT_EQ(kvs.getProxyHelper(), proxy_helper.get()); + { ASSERT_EQ(kvs.getRegion(0), nullptr); auto task_lock = kvs.genTaskLock(); @@ -213,15 +246,15 @@ void RegionKVStoreTest::testReadIndex() lock.index.add(region); } { - ASSERT_EQ(proxy_instance.regions.at(tar_region_id)->getLatestCommitIndex(), 5); - proxy_instance.regions.at(tar_region_id)->updateCommitIndex(66); + ASSERT_EQ(proxy_instance->regions.at(tar_region_id)->getLatestCommitIndex(), 5); + proxy_instance->regions.at(tar_region_id)->updateCommitIndex(66); } AsyncWaker::Notifier notifier; const std::atomic_size_t terminate_signals_counter{}; std::thread t([&]() { notifier.wake(); - WaitCheckRegionReady(ctx.getTMTContext(), terminate_signals_counter, 1 / 1000.0, 20, 20 * 60); + WaitCheckRegionReady(ctx.getTMTContext(), kvs, terminate_signals_counter, 1 / 1000.0, 20, 20 * 60); }); SCOPE_EXIT({ t.join(); @@ -242,8 +275,8 @@ void RegionKVStoreTest::testReadIndex() auto tar_region_id = 9; { - ASSERT_EQ(proxy_instance.regions.at(tar_region_id)->getLatestCommitIndex(), 66); - proxy_instance.unsafeInvokeForTest([&](MockRaftStoreProxy & p) { + ASSERT_EQ(proxy_instance->regions.at(tar_region_id)->getLatestCommitIndex(), 66); + proxy_instance->unsafeInvokeForTest([&](MockRaftStoreProxy & p) { p.region_id_to_error.emplace(tar_region_id); p.regions.at(2)->updateCommitIndex(6); }); @@ -253,7 +286,7 @@ void RegionKVStoreTest::testReadIndex() const std::atomic_size_t terminate_signals_counter{}; std::thread t([&]() { notifier.wake(); - WaitCheckRegionReady(ctx.getTMTContext(), terminate_signals_counter, 1 / 1000.0, 2 / 1000.0, 5 / 1000.0); + WaitCheckRegionReady(ctx.getTMTContext(), kvs, terminate_signals_counter, 1 / 1000.0, 2 / 1000.0, 5 / 1000.0); }); SCOPE_EXIT({ t.join(); @@ -281,7 +314,7 @@ void RegionKVStoreTest::testReadIndex() ASSERT_EQ(std::get<0>(r), WaitIndexResult::Terminated); } } - for (auto & r : proxy_instance.regions) + for (auto & r : proxy_instance->regions) { r.second->updateCommitIndex(667); } @@ -300,7 +333,7 @@ void RegionKVStoreTest::testReadIndex() { auto region = kvs.getRegion(2); auto req = GenRegionReadIndexReq(*region, 5); - auto resp = proxy_helper.batchReadIndex({req}, 100); // v2 + auto resp = proxy_helper->batchReadIndex({req}, 100); // v2 ASSERT_EQ(resp[0].first.read_index(), 667); // got latest { auto r = region->waitIndex(667 + 1, 2, []() { return true; }); @@ -325,7 +358,7 @@ void RegionKVStoreTest::testReadIndex() kvs.stopReadIndexWorkers(); kvs.releaseReadIndexWorkers(); over = true; - proxy_instance.wake(); + proxy_instance->wake(); proxy_runner.join(); ASSERT(GCMonitor::instance().checkClean()); ASSERT(!GCMonitor::instance().empty()); @@ -837,21 +870,9 @@ void RegionKVStoreTest::testRegion() void RegionKVStoreTest::testKVStore() { - std::string path = TiFlashTestEnv::getTemporaryPath("/region_kvs_tmp") + "/basic"; - - auto path_pool = createCleanPathPool(path); + auto ctx = TiFlashTestEnv::getGlobalContext(); - auto ctx = TiFlashTestEnv::getContext(DB::Settings(), Strings{ - path, - }); - KVStore & kvs = *ctx.getTMTContext().getKVStore(); - MockRaftStoreProxy proxy_instance; - TiFlashRaftProxyHelper proxy_helper; - { - proxy_helper = MockRaftStoreProxy::SetRaftStoreProxyFFIHelper(RaftStoreProxyPtr{&proxy_instance}); - proxy_instance.init(100); - } - kvs.restore(*path_pool, &proxy_helper); + KVStore & kvs = getKVS(); { // Run without read-index workers @@ -1089,7 +1110,7 @@ void RegionKVStoreTest::testKVStore() BaseBuffView{region_id_str.data(), region_id_str.length()}, }); { - RegionMockTest mock_test(ctx.getTMTContext().getKVStore(), region); + RegionMockTest mock_test(kvstore.get(), region); kvs.handleApplySnapshot( region->getMetaRegion(), @@ -1145,10 +1166,10 @@ void RegionKVStoreTest::testKVStore() } { - const auto * ori_ptr = proxy_helper.proxy_ptr.inner; - proxy_helper.proxy_ptr.inner = nullptr; + const auto * ori_ptr = proxy_helper->proxy_ptr.inner; + proxy_helper->proxy_ptr.inner = nullptr; SCOPE_EXIT({ - proxy_helper.proxy_ptr.inner = ori_ptr; + proxy_helper->proxy_ptr.inner = ori_ptr; }); try @@ -1170,7 +1191,7 @@ void RegionKVStoreTest::testKVStore() } { - proxy_instance.getRegion(22)->setSate(({ + proxy_instance->getRegion(22)->setSate(({ raft_serverpb::RegionLocalState s; s.set_state(::raft_serverpb::PeerState::Tombstone); s; @@ -1184,7 +1205,7 @@ void RegionKVStoreTest::testKVStore() ctx.getTMTContext()); kvs.checkAndApplySnapshot(RegionPtrWithSnapshotFiles{region, std::move(ingest_ids)}, ctx.getTMTContext()); // overlap, tombstone, remove previous one - auto state = proxy_helper.getRegionLocalState(8192); + auto state = proxy_helper->getRegionLocalState(8192); ASSERT_EQ(state.state(), raft_serverpb::PeerState::Tombstone); } @@ -1207,7 +1228,7 @@ void RegionKVStoreTest::testKVStore() // Mock SST data for handle [star, end) auto region = kvs.getRegion(region_id); - RegionMockTest mock_test(ctx.getTMTContext().getKVStore(), region); + RegionMockTest mock_test(kvstore.get(), region); { // Mocking ingest a SST for column family "Write" @@ -1259,6 +1280,48 @@ void RegionKVStoreTest::testKVStore() } } +void RegionKVStoreTest::testKVStoreRestore() +{ + { + KVStore & kvs = getKVS(); + { + auto store = metapb::Store{}; + store.set_id(1234); + kvs.setStore(store); + ASSERT_EQ(kvs.getStoreID(), store.id()); + } + { + ASSERT_EQ(kvs.getRegion(0), nullptr); + auto task_lock = kvs.genTaskLock(); + auto lock = kvs.genRegionWriteLock(task_lock); + { + auto region = makeRegion(1, RecordKVFormat::genKey(1, 0), RecordKVFormat::genKey(1, 10)); + lock.regions.emplace(1, region); + lock.index.add(region); + } + { + auto region = makeRegion(2, RecordKVFormat::genKey(1, 10), RecordKVFormat::genKey(1, 20)); + lock.regions.emplace(2, region); + lock.index.add(region); + } + { + auto region = makeRegion(3, RecordKVFormat::genKey(1, 30), RecordKVFormat::genKey(1, 40)); + lock.regions.emplace(3, region); + lock.index.add(region); + } + } + kvs.tryPersist(1); + kvs.tryPersist(2); + kvs.tryPersist(3); + } + { + KVStore & kvs = reloadKVSFromDisk(); + kvs.getRegion(1); + kvs.getRegion(2); + kvs.getRegion(3); + } +} + void test_mergeresult() { ASSERT_EQ(MetaRaftCommandDelegate::computeRegionMergeResult(createRegionInfo(1, "x", ""), createRegionInfo(1000, "", "x")).source_at_left, false); @@ -1480,6 +1543,13 @@ try } CATCH +TEST_F(RegionKVStoreTest, KVStoreRestore) +try +{ + testKVStoreRestore(); +} +CATCH + TEST_F(RegionKVStoreTest, Region) try { diff --git a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp index 13cb7e9162c..3ada4856e23 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp @@ -25,12 +25,11 @@ #include #include #include +#include #include #include -#include "common/logger_useful.h" - namespace DB { namespace FailPoints From 9044d781c9eb0f0367e6c4d6cd6f346fd4b76ca7 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 23 Aug 2022 14:02:26 +0800 Subject: [PATCH 4/6] Use DBMS_PUBLIC_GTEST instead of friend class Signed-off-by: JaySon-Huang --- dbms/src/Storages/Transaction/KVStore.h | 4 + dbms/src/Storages/Transaction/Region.h | 8 +- dbms/src/Storages/Transaction/RegionMeta.h | 7 +- .../Transaction/tests/gtest_kvstore.cpp | 99 ++++--------------- .../tests/gtest_region_persister.cpp | 1 - 5 files changed, 31 insertions(+), 88 deletions(-) diff --git a/dbms/src/Storages/Transaction/KVStore.h b/dbms/src/Storages/Transaction/KVStore.h index 9c1cea1f0ba..22a6dc3ace7 100644 --- a/dbms/src/Storages/Transaction/KVStore.h +++ b/dbms/src/Storages/Transaction/KVStore.h @@ -163,7 +163,9 @@ class KVStore final : private boost::noncopyable FileUsageStatistics getFileUsageStatistics() const; +#ifndef DBMS_PUBLIC_GTEST private: +#endif friend class MockTiDB; friend struct MockTiDBTable; friend struct MockRaftCommand; @@ -232,7 +234,9 @@ class KVStore final : private boost::noncopyable void releaseReadIndexWorkers(); void handleDestroy(UInt64 region_id, TMTContext & tmt, const KVStoreTaskLock &); +#ifndef DBMS_PUBLIC_GTEST private: +#endif RegionManager region_manager; std::unique_ptr region_persister; diff --git a/dbms/src/Storages/Transaction/Region.h b/dbms/src/Storages/Transaction/Region.h index 06b18de379a..91d0f3c8b81 100644 --- a/dbms/src/Storages/Transaction/Region.h +++ b/dbms/src/Storages/Transaction/Region.h @@ -64,7 +64,7 @@ class Region : public std::enable_shared_from_this class CommittedScanner : private boost::noncopyable { public: - CommittedScanner(const RegionPtr & store_, bool use_lock = true) + explicit CommittedScanner(const RegionPtr & store_, bool use_lock = true) : store(store_) { if (use_lock) @@ -97,7 +97,7 @@ class Region : public std::enable_shared_from_this class CommittedRemover : private boost::noncopyable { public: - CommittedRemover(const RegionPtr & store_, bool use_lock = true) + explicit CommittedRemover(const RegionPtr & store_, bool use_lock = true) : store(store_) { if (use_lock) @@ -245,11 +245,11 @@ class RegionRaftCommandDelegate : public Region const RegionRangeKeys & getRange(); UInt64 appliedIndex(); + RegionRaftCommandDelegate() = delete; + private: friend class tests::RegionKVStoreTest; - RegionRaftCommandDelegate() = delete; - Regions execBatchSplit( const raft_cmdpb::AdminRequest & request, const raft_cmdpb::AdminResponse & response, diff --git a/dbms/src/Storages/Transaction/RegionMeta.h b/dbms/src/Storages/Transaction/RegionMeta.h index ca284238c12..ee64ab3c958 100644 --- a/dbms/src/Storages/Transaction/RegionMeta.h +++ b/dbms/src/Storages/Transaction/RegionMeta.h @@ -110,8 +110,9 @@ class RegionMeta metapb::Region getMetaRegion() const; raft_serverpb::MergeState getMergeState() const; -private: RegionMeta() = delete; + +private: friend class MetaRaftCommandDelegate; friend class tests::RegionKVStoreTest; @@ -157,8 +158,6 @@ class MetaRaftCommandDelegate friend class RegionRaftCommandDelegate; friend class tests::RegionKVStoreTest; - MetaRaftCommandDelegate() = delete; - const metapb::Peer & getPeer() const; const raft_serverpb::RaftApplyState & applyState() const; const RegionState & regionState() const; @@ -192,6 +191,8 @@ class MetaRaftCommandDelegate static RegionMergeResult computeRegionMergeResult( const metapb::Region & source_region, const metapb::Region & target_region); + + MetaRaftCommandDelegate() = delete; }; } // namespace DB diff --git a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp index 6b063e0014a..6ba67bd3668 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_kvstore.cpp @@ -47,13 +47,13 @@ namespace tests class RegionKVStoreTest : public ::testing::Test { public: - RegionKVStoreTest() = default; - - static void SetUpTestCase() + RegionKVStoreTest() { test_path = TiFlashTestEnv::getTemporaryPath("/region_kvs_test"); } + static void SetUpTestCase() {} + void SetUp() override { // clean data and create path pool instance @@ -69,34 +69,21 @@ class RegionKVStoreTest : public ::testing::Test kvstore->restore(*path_pool, proxy_helper.get()); } - void TearDown() override - { - proxy_helper.reset(); - proxy_instance.reset(); - kvstore.reset(); - path_pool.reset(); - } - - static void testBasic(); - static void testKVStore(); - static void testRegion(); - static void testReadIndex(); - static void testNewProxy(); - static void testKVStoreRestore(); + void TearDown() override {} protected: - static KVStore & getKVS() { return *kvstore; } - static KVStore & reloadKVSFromDisk() + KVStore & getKVS() { return *kvstore; } + KVStore & reloadKVSFromDisk() { kvstore.reset(); auto & global_ctx = TiFlashTestEnv::getGlobalContext(); kvstore = std::make_unique(global_ctx, TiDB::SnapshotApplyMethod::DTFile_Directory); - // TODO: Do we need to recreate proxy_instance and proxy_helper here? + // only recreate kvstore and restore data from disk, don't recreate proxy instance kvstore->restore(*path_pool, proxy_helper.get()); return *kvstore; } -private: +protected: static void testRaftSplit(KVStore & kvs, TMTContext & tmt); static void testRaftMerge(KVStore & kvs, TMTContext & tmt); static void testRaftChangePeer(KVStore & kvs, TMTContext & tmt); @@ -118,22 +105,16 @@ class RegionKVStoreTest : public ::testing::Test return std::make_unique(main_data_paths, main_data_paths, Strings{}, path_capacity, provider); } - static std::string test_path; + std::string test_path; - static std::unique_ptr path_pool; - static std::unique_ptr kvstore; + std::unique_ptr path_pool; + std::unique_ptr kvstore; - static std::unique_ptr proxy_instance; - static std::unique_ptr proxy_helper; + std::unique_ptr proxy_instance; + std::unique_ptr proxy_helper; }; -std::string RegionKVStoreTest::test_path; -std::unique_ptr RegionKVStoreTest::path_pool; -std::unique_ptr RegionKVStoreTest::kvstore; -std::unique_ptr RegionKVStoreTest::proxy_instance; -std::unique_ptr RegionKVStoreTest::proxy_helper; - -void RegionKVStoreTest::testNewProxy() +TEST_F(RegionKVStoreTest, NewProxy) { auto ctx = TiFlashTestEnv::getGlobalContext(); @@ -176,7 +157,7 @@ void RegionKVStoreTest::testNewProxy() } } -void RegionKVStoreTest::testReadIndex() +TEST_F(RegionKVStoreTest, ReadIndex) { auto ctx = TiFlashTestEnv::getGlobalContext(); @@ -771,7 +752,7 @@ void RegionKVStoreTest::testRaftMerge(KVStore & kvs, TMTContext & tmt) } } -void RegionKVStoreTest::testRegion() +TEST_F(RegionKVStoreTest, Region) { TableID table_id = 100; { @@ -868,7 +849,7 @@ void RegionKVStoreTest::testRegion() } } -void RegionKVStoreTest::testKVStore() +TEST_F(RegionKVStoreTest, KVStore) { auto ctx = TiFlashTestEnv::getGlobalContext(); @@ -1280,7 +1261,7 @@ void RegionKVStoreTest::testKVStore() } } -void RegionKVStoreTest::testKVStoreRestore() +TEST_F(RegionKVStoreTest, KVStoreRestore) { { KVStore & kvs = getKVS(); @@ -1367,7 +1348,7 @@ void test_mergeresult() } } -void RegionKVStoreTest::testBasic() +TEST_F(RegionKVStoreTest, Basic) { { RegionsRangeIndex region_index; @@ -1529,47 +1510,5 @@ void RegionKVStoreTest::testBasic() } } -TEST_F(RegionKVStoreTest, Basic) -try -{ - testBasic(); -} -CATCH - -TEST_F(RegionKVStoreTest, KVStore) -try -{ - testKVStore(); -} -CATCH - -TEST_F(RegionKVStoreTest, KVStoreRestore) -try -{ - testKVStoreRestore(); -} -CATCH - -TEST_F(RegionKVStoreTest, Region) -try -{ - testRegion(); -} -CATCH - -TEST_F(RegionKVStoreTest, ReadIndex) -try -{ - testReadIndex(); -} -CATCH - -TEST_F(RegionKVStoreTest, NewProxy) -try -{ - testNewProxy(); -} -CATCH - } // namespace tests } // namespace DB diff --git a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp index 3ada4856e23..41d30fd23d6 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp @@ -28,7 +28,6 @@ #include #include -#include namespace DB { From 29e4368127b37f26049d0fab4a882b3e9329b9a9 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 23 Aug 2022 14:46:47 +0800 Subject: [PATCH 5/6] Fix ut fail Signed-off-by: JaySon-Huang --- .../tests/gtest_region_persister.cpp | 53 ++++++++++--------- dbms/src/TestUtils/TiFlashTestEnv.h | 12 ++++- 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp index 41d30fd23d6..b102cc4087f 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp @@ -48,8 +48,7 @@ static ::testing::AssertionResult PeerCompare( { if (lhs.id() == rhs.id() && lhs.role() == rhs.role()) return ::testing::AssertionSuccess(); - else - return ::testing::internal::EqFailure(lhs_expr, rhs_expr, lhs.ShortDebugString(), rhs.ShortDebugString(), false); + return ::testing::internal::EqFailure(lhs_expr, rhs_expr, lhs.ShortDebugString(), rhs.ShortDebugString(), false); } #define ASSERT_PEER_EQ(val1, val2) ASSERT_PRED_FORMAT2(::DB::tests::PeerCompare, val1, val2) @@ -61,16 +60,31 @@ static ::testing::AssertionResult RegionCompare( { if (lhs == rhs) return ::testing::AssertionSuccess(); - else - return ::testing::internal::EqFailure(lhs_expr, rhs_expr, lhs.toString(), rhs.toString(), false); + return ::testing::internal::EqFailure(lhs_expr, rhs_expr, lhs.toString(), rhs.toString(), false); } #define ASSERT_REGION_EQ(val1, val2) ASSERT_PRED_FORMAT2(::DB::tests::RegionCompare, val1, val2) -TEST(RegionSeriTest, peer) +class RegionSeriTest : public ::testing::Test +{ +public: + RegionSeriTest() + : dir_path(TiFlashTestEnv::getTemporaryPath("RegionSeriTest")) + { + } + + void SetUp() override + { + TiFlashTestEnv::tryRemovePath(dir_path, /*recreate=*/true); + } + + std::string dir_path; +}; + +TEST_F(RegionSeriTest, peer) try { auto peer = createPeer(100, true); - const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/peer.test"; + const auto path = dir_path + "/peer.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); auto size = writeBinary2(peer, write_buf); write_buf.next(); @@ -83,11 +97,11 @@ try } CATCH -TEST(RegionSeriTest, RegionInfo) +TEST_F(RegionSeriTest, RegionInfo) try { auto region_info = createRegionInfo(233, "", ""); - const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/region_info.test"; + const auto path = dir_path + "/region_info.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); auto size = writeBinary2(region_info, write_buf); write_buf.next(); @@ -105,11 +119,11 @@ try } CATCH -TEST(RegionSeriTest, RegionMeta) +TEST_F(RegionSeriTest, RegionMeta) try { RegionMeta meta = createRegionMeta(888, 66); - const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/meta.test"; + const auto path = dir_path + "/meta.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); auto size = std::get<0>(meta.serialize(write_buf)); write_buf.next(); @@ -122,7 +136,7 @@ try } CATCH -TEST(RegionSeriTest, Region) +TEST_F(RegionSeriTest, Region) try { TableID table_id = 100; @@ -132,7 +146,7 @@ try region->insert("write", TiKVKey::copyFrom(key), RecordKVFormat::encodeWriteCfValue('P', 0)); region->insert("lock", TiKVKey::copyFrom(key), RecordKVFormat::encodeLockCfValue('P', "", 0, 0)); - const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/region.test"; + const auto path = dir_path + "/region.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); size_t region_ser_size = std::get<0>(region->serialize(write_buf)); write_buf.next(); @@ -145,7 +159,7 @@ try } CATCH -TEST(RegionSeriTest, RegionStat) +TEST_F(RegionSeriTest, RegionStat) try { RegionPtr region = nullptr; @@ -175,7 +189,7 @@ try region->insert("write", TiKVKey::copyFrom(key), RecordKVFormat::encodeWriteCfValue('P', 0)); region->insert("lock", TiKVKey::copyFrom(key), RecordKVFormat::encodeLockCfValue('P', "", 0, 0)); - const auto path = TiFlashTestEnv::getTemporaryPath("RegionSeriTest") + "/region_state.test"; + const auto path = dir_path + "/region_state.test"; WriteBufferFromFile write_buf(path, DBMS_DEFAULT_BUFFER_SIZE, O_WRONLY | O_CREAT); size_t region_ser_size = std::get<0>(region->serialize(write_buf)); write_buf.next(); @@ -200,7 +214,7 @@ class RegionPersisterTest : public ::testing::Test void SetUp() override { - dropFiles(); + TiFlashTestEnv::tryRemovePath(dir_path); auto & global_ctx = TiFlashTestEnv::getGlobalContext(); auto path_capacity = global_ctx.getPathCapacity(); @@ -216,15 +230,6 @@ class RegionPersisterTest : public ::testing::Test /*enable_raft_compatible_mode_=*/true); } - void dropFiles() - { - // cleanup - Poco::File file(dir_path); - if (file.exists()) - file.remove(true); - file.createDirectories(); - } - void runTest(const String & path, bool sync_on_write); void testFunc(const String & path, const PageStorage::Config & config, int region_num, bool is_gc, bool clean_up); diff --git a/dbms/src/TestUtils/TiFlashTestEnv.h b/dbms/src/TestUtils/TiFlashTestEnv.h index 6b650665646..636ffa06f95 100644 --- a/dbms/src/TestUtils/TiFlashTestEnv.h +++ b/dbms/src/TestUtils/TiFlashTestEnv.h @@ -36,14 +36,22 @@ class TiFlashTestEnv return Poco::Path(path).absolute().toString(); } - static void tryRemovePath(const std::string & path) + static void tryRemovePath(const std::string & path, bool recreate = false) { try { - if (Poco::File p(path); p.exists()) + // drop the data on disk + Poco::File p(path); + if (p.exists()) { p.remove(true); } + + // re-create empty directory for testing + if (recreate) + { + p.createDirectories(); + } } catch (...) { From ddf492aa1f280d8be62080d7edd8b089dd221da0 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 23 Aug 2022 14:54:18 +0800 Subject: [PATCH 6/6] remove useless var Signed-off-by: JaySon-Huang --- .../Transaction/tests/gtest_region_persister.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp index b102cc4087f..2f3ed021d79 100644 --- a/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp +++ b/dbms/src/Storages/Transaction/tests/gtest_region_persister.cpp @@ -230,19 +230,9 @@ class RegionPersisterTest : public ::testing::Test /*enable_raft_compatible_mode_=*/true); } - void runTest(const String & path, bool sync_on_write); - void testFunc(const String & path, const PageStorage::Config & config, int region_num, bool is_gc, bool clean_up); - protected: String dir_path; - DB::Timestamp tso = 0; - - static String getPageStorageV3MetaPath(String & path) - { - return path + "/page/kvstore/wal/log_1_0"; - } - std::unique_ptr mocked_path_pool; };