From 1bb8bd4ab4ce50e908dc023638aaf1432b6eb84f Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sat, 20 Apr 2024 16:01:38 +0800 Subject: [PATCH 1/4] Fix background tasks making unstable ut --- dbms/src/TiDB/Schema/SchemaSyncService.cpp | 29 +++++++++++++------ .../TiDB/Schema/tests/gtest_schema_sync.cpp | 18 ++++++++---- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaSyncService.cpp b/dbms/src/TiDB/Schema/SchemaSyncService.cpp index 2cfff9c59e3..d27b54cad73 100644 --- a/dbms/src/TiDB/Schema/SchemaSyncService.cpp +++ b/dbms/src/TiDB/Schema/SchemaSyncService.cpp @@ -42,15 +42,26 @@ SchemaSyncService::SchemaSyncService(DB::Context & context_) , log(Logger::get()) { // Add task for adding and removing keyspace sync schema tasks. - handle = background_pool.addTask( - [&, this] { - addKeyspaceGCTasks(); - removeKeyspaceGCTasks(); - - return false; - }, - false, - context.getSettingsRef().ddl_sync_interval_seconds * 1000); + auto interval_ms = context.getSettingsRef().ddl_sync_interval_seconds * 1000; + if (interval_ms == 0) + { + LOG_WARNING( + log, + "The background task of SchemaSyncService is disabled, please check the ddl_sync_interval_seconds " + "settings"); + } + else + { + handle = background_pool.addTask( + [&, this] { + addKeyspaceGCTasks(); + removeKeyspaceGCTasks(); + + return false; + }, + false, + interval_ms); + } } void SchemaSyncService::addKeyspaceGCTasks() diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp index ba24eab316f..1636134b948 100644 --- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -300,7 +300,12 @@ try refreshTableSchema(table_id); } - auto sync_service = std::make_shared(global_ctx); + // Create a temporary context with ddl sync task disabled + auto ctx = DB::tests::TiFlashTestEnv::getContext(); + ctx->getSettingsRef().ddl_sync_interval_seconds = 0; + auto sync_service = std::make_shared(*ctx); + sync_service->shutdown(); // shutdown the background tasks + // run gc with safepoint == 0, will be skip ASSERT_FALSE(sync_service->gc(0, NullspaceID)); ASSERT_TRUE(sync_service->gc(10000000, NullspaceID)); @@ -312,8 +317,6 @@ try ASSERT_TRUE(sync_service->gc(20000000, 1024)); // run gc with the same safepoint ASSERT_FALSE(sync_service->gc(20000000, 1024)); - - sync_service->shutdown(); } CATCH @@ -357,7 +360,12 @@ try std::vector{1001, 1002, 1003}); SCOPE_EXIT({ FailPointHelper::disableFailPoint(FailPoints::force_set_num_regions_for_table); }); - auto sync_service = std::make_shared(global_ctx); + // Create a temporary context with ddl sync task disabled + auto ctx = DB::tests::TiFlashTestEnv::getContext(); + ctx->getSettingsRef().ddl_sync_interval_seconds = 0; + auto sync_service = std::make_shared(*ctx); + sync_service->shutdown(); // shutdown the background tasks + { // ensure gc_safe_point cache is empty auto last_gc_safe_point = lastGcSafePoint(sync_service, NullspaceID); @@ -381,8 +389,6 @@ try ++num_remain_tables; } ASSERT_EQ(num_remain_tables, 1); - - sync_service->shutdown(); } CATCH From 10441cc6891b05782b8adde9be08020c8379b01d Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sat, 20 Apr 2024 16:14:37 +0800 Subject: [PATCH 2/4] Use temp TiDBSchemaSyncerManager in SchemaSyncTest --- dbms/src/TiDB/Schema/TiDBSchemaManager.h | 98 ++++++++++--------- .../TiDB/Schema/tests/gtest_schema_sync.cpp | 24 ++--- 2 files changed, 66 insertions(+), 56 deletions(-) diff --git a/dbms/src/TiDB/Schema/TiDBSchemaManager.h b/dbms/src/TiDB/Schema/TiDBSchemaManager.h index 8e9d4289936..dd70e4dd9d0 100644 --- a/dbms/src/TiDB/Schema/TiDBSchemaManager.h +++ b/dbms/src/TiDB/Schema/TiDBSchemaManager.h @@ -29,31 +29,6 @@ class TiDBSchemaSyncerManager , log(Logger::get("TiDBSchemaSyncerManager")) {} - SchemaSyncerPtr createSchemaSyncer(KeyspaceID keyspace_id) - { - if (!mock_getter and !mock_mapper) - { - auto schema_syncer = std::static_pointer_cast( - std::make_shared>(cluster, keyspace_id)); - schema_syncers[keyspace_id] = schema_syncer; - return schema_syncer; - } - else if (mock_getter and mock_mapper) - { - // for mock test - auto schema_syncer = std::static_pointer_cast( - std::make_shared>(cluster, keyspace_id)); - schema_syncers[keyspace_id] = schema_syncer; - return schema_syncer; - } - - // for unit test - auto schema_syncer = std::static_pointer_cast( - std::make_shared>(cluster, keyspace_id)); - schema_syncers[keyspace_id] = schema_syncer; - return schema_syncer; - } - bool syncSchemas(Context & context, KeyspaceID keyspace_id) { auto schema_syncer = getOrCreateSchemaSyncer(keyspace_id); @@ -68,8 +43,8 @@ class TiDBSchemaSyncerManager void reset(KeyspaceID keyspace_id) { - std::shared_lock read_lock(schema_syncers_mutex); - auto schema_syncer = getSchemaSyncer(keyspace_id); + std::shared_lock read_lock(schema_syncers_mutex); + auto schema_syncer = getSchemaSyncer(keyspace_id, read_lock); if (schema_syncer == nullptr) { LOG_ERROR(log, "SchemaSyncer not found, keyspace={}", keyspace_id); @@ -80,8 +55,8 @@ class TiDBSchemaSyncerManager TiDB::DBInfoPtr getDBInfoByName(KeyspaceID keyspace_id, const String & database_name) { - std::shared_lock read_lock(schema_syncers_mutex); - auto schema_syncer = getSchemaSyncer(keyspace_id); + std::shared_lock read_lock(schema_syncers_mutex); + auto schema_syncer = getSchemaSyncer(keyspace_id, read_lock); if (schema_syncer == nullptr) { LOG_ERROR(log, "SchemaSyncer not found, keyspace={}", keyspace_id); @@ -92,8 +67,8 @@ class TiDBSchemaSyncerManager bool removeSchemaSyncer(KeyspaceID keyspace_id) { - std::unique_lock lock(schema_syncers_mutex); - auto schema_syncer = getSchemaSyncer(keyspace_id); + std::unique_lock lock(schema_syncers_mutex); + auto schema_syncer = getSchemaSyncer(keyspace_id, lock); if (schema_syncer == nullptr) { LOG_ERROR(log, "SchemaSyncer not found, keyspace={}", keyspace_id); @@ -105,8 +80,8 @@ class TiDBSchemaSyncerManager void removeTableID(KeyspaceID keyspace_id, TableID table_id) { - std::shared_lock read_lock(schema_syncers_mutex); - auto schema_syncer = getSchemaSyncer(keyspace_id); + std::shared_lock read_lock(schema_syncers_mutex); + auto schema_syncer = getSchemaSyncer(keyspace_id, read_lock); if (schema_syncer == nullptr) { LOG_ERROR(log, "SchemaSyncer not found, keyspace={}", keyspace_id); @@ -126,30 +101,63 @@ class TiDBSchemaSyncerManager std::unordered_map schema_syncers; - /// the function is not thread safe, should be called with a lock - SchemaSyncerPtr getSchemaSyncer(KeyspaceID keyspace_id) +private: + /// Try to get the SchemaSyncer for the `keyspace_id`. Returns nullptr + /// if there is not exist. + /// Note: the function is not thread safe, should be called with a lock + template + SchemaSyncerPtr getSchemaSyncer(KeyspaceID keyspace_id, Lock & /*lock*/) { auto syncer = schema_syncers.find(keyspace_id); return syncer == schema_syncers.end() ? nullptr : syncer->second; } + /// Try to get the SchemaSyncer for the `keyspace_id`. Create a SchemaSyncer + /// if there is not exist. SchemaSyncerPtr getOrCreateSchemaSyncer(KeyspaceID keyspace_id) { - std::shared_lock read_lock(schema_syncers_mutex); - auto syncer = schema_syncers.find(keyspace_id); - if (syncer == schema_syncers.end()) { - read_lock.unlock(); - std::unique_lock write_lock(schema_syncers_mutex); - - syncer = schema_syncers.find(keyspace_id); - if (syncer == schema_syncers.end()) + std::shared_lock read_lock(schema_syncers_mutex); + if (auto syncer = schema_syncers.find(keyspace_id); syncer != schema_syncers.end()) { - return createSchemaSyncer(keyspace_id); + return syncer->second; } + } + + // release the read_lock and acquire a write_lock + std::unique_lock write_lock(schema_syncers_mutex); + // check again whether other thread has created for the keyspace_id + // after `write_lock` acquired + if (auto syncer = schema_syncers.find(keyspace_id); syncer != schema_syncers.end()) + { return syncer->second; } - return syncer->second; + return createSchemaSyncer(keyspace_id, write_lock); + } + + SchemaSyncerPtr createSchemaSyncer(KeyspaceID keyspace_id, std::unique_lock &) + { + if (!mock_getter && !mock_mapper) + { + auto schema_syncer = std::static_pointer_cast( + std::make_shared>(cluster, keyspace_id)); + schema_syncers[keyspace_id] = schema_syncer; + return schema_syncer; + } + else if (mock_getter && mock_mapper) + { + // for mock test + auto schema_syncer = std::static_pointer_cast( + std::make_shared>(cluster, keyspace_id)); + schema_syncers[keyspace_id] = schema_syncer; + return schema_syncer; + } + + // for unit test + auto schema_syncer = std::static_pointer_cast( + std::make_shared>(cluster, keyspace_id)); + schema_syncers[keyspace_id] = schema_syncer; + return schema_syncer; } }; } // namespace DB diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp index 1636134b948..5771ad6c778 100644 --- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -72,6 +72,14 @@ class SchemaSyncTest : public ::testing::Test void SetUp() override { + // unit test. + // Get DBInfo/TableInfo from MockTiDB, but create table with names `t_${table_id}` + auto cluster = std::make_shared(); + schema_sync_manager = std::make_unique( + cluster, + /*mock_getter*/ true, + /*mock_mapper*/ false); + // disable schema sync timer global_ctx.getSchemaSyncService().reset(); recreateMetadataPath(); @@ -97,11 +105,9 @@ class SchemaSyncTest : public ::testing::Test // Sync schema info from TiDB/MockTiDB to TiFlash void refreshSchema() { - auto & flash_ctx = global_ctx.getTMTContext(); - auto schema_syncer = flash_ctx.getSchemaSyncerManager(); try { - schema_syncer->syncSchemas(global_ctx, NullspaceID); + schema_sync_manager->syncSchemas(global_ctx, NullspaceID); } catch (Exception & e) { @@ -118,11 +124,9 @@ class SchemaSyncTest : public ::testing::Test void refreshTableSchema(TableID table_id) { - auto & flash_ctx = global_ctx.getTMTContext(); - auto schema_syncer = flash_ctx.getSchemaSyncerManager(); try { - schema_syncer->syncTableSchema(global_ctx, NullspaceID, table_id); + schema_sync_manager->syncTableSchema(global_ctx, NullspaceID, table_id); } catch (Exception & e) { @@ -138,11 +142,7 @@ class SchemaSyncTest : public ::testing::Test } // Reset the schema syncer to mock TiFlash shutdown - void resetSchemas() - { - auto & flash_ctx = global_ctx.getTMTContext(); - flash_ctx.getSchemaSyncerManager()->reset(NullspaceID); - } + void resetSchemas() { schema_sync_manager->reset(NullspaceID); } // Get the TiFlash synced table ManageableStoragePtr mustGetSyncedTable(TableID table_id) @@ -207,6 +207,8 @@ class SchemaSyncTest : public ::testing::Test protected: Context & global_ctx; + + std::unique_ptr schema_sync_manager; }; TEST_F(SchemaSyncTest, SchemaDiff) From 983c954cdf17c5b2e43ae69487477440afbeea50 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sat, 20 Apr 2024 16:25:52 +0800 Subject: [PATCH 3/4] Simplify code --- dbms/src/TiDB/Schema/TiDBSchemaManager.h | 26 ++++++++++-------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/dbms/src/TiDB/Schema/TiDBSchemaManager.h b/dbms/src/TiDB/Schema/TiDBSchemaManager.h index dd70e4dd9d0..a0c2451c5ae 100644 --- a/dbms/src/TiDB/Schema/TiDBSchemaManager.h +++ b/dbms/src/TiDB/Schema/TiDBSchemaManager.h @@ -118,9 +118,9 @@ class TiDBSchemaSyncerManager { { std::shared_lock read_lock(schema_syncers_mutex); - if (auto syncer = schema_syncers.find(keyspace_id); syncer != schema_syncers.end()) + if (auto iter = schema_syncers.find(keyspace_id); iter != schema_syncers.end()) { - return syncer->second; + return iter->second; } } @@ -128,36 +128,32 @@ class TiDBSchemaSyncerManager std::unique_lock write_lock(schema_syncers_mutex); // check again whether other thread has created for the keyspace_id // after `write_lock` acquired - if (auto syncer = schema_syncers.find(keyspace_id); syncer != schema_syncers.end()) + if (auto iter = schema_syncers.find(keyspace_id); iter != schema_syncers.end()) { - return syncer->second; + return iter->second; } - return createSchemaSyncer(keyspace_id, write_lock); + auto syncer = createSchemaSyncer(keyspace_id); + schema_syncers[keyspace_id] = syncer; + return syncer; } - SchemaSyncerPtr createSchemaSyncer(KeyspaceID keyspace_id, std::unique_lock &) + SchemaSyncerPtr createSchemaSyncer(KeyspaceID keyspace_id) { if (!mock_getter && !mock_mapper) { - auto schema_syncer = std::static_pointer_cast( + return std::static_pointer_cast( std::make_shared>(cluster, keyspace_id)); - schema_syncers[keyspace_id] = schema_syncer; - return schema_syncer; } else if (mock_getter && mock_mapper) { // for mock test - auto schema_syncer = std::static_pointer_cast( + return std::static_pointer_cast( std::make_shared>(cluster, keyspace_id)); - schema_syncers[keyspace_id] = schema_syncer; - return schema_syncer; } // for unit test - auto schema_syncer = std::static_pointer_cast( + return std::static_pointer_cast( std::make_shared>(cluster, keyspace_id)); - schema_syncers[keyspace_id] = schema_syncer; - return schema_syncer; } }; } // namespace DB From 08de4fef8413d6cea1934af0d5c289a9eff599e1 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sat, 20 Apr 2024 16:26:30 +0800 Subject: [PATCH 4/4] Simplify code --- dbms/src/TiDB/Schema/TiDBSchemaManager.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/TiDB/Schema/TiDBSchemaManager.h b/dbms/src/TiDB/Schema/TiDBSchemaManager.h index a0c2451c5ae..6d4bac44e7a 100644 --- a/dbms/src/TiDB/Schema/TiDBSchemaManager.h +++ b/dbms/src/TiDB/Schema/TiDBSchemaManager.h @@ -133,7 +133,7 @@ class TiDBSchemaSyncerManager return iter->second; } auto syncer = createSchemaSyncer(keyspace_id); - schema_syncers[keyspace_id] = syncer; + schema_syncers[keyspace_id] = syncer; // register to the syncers return syncer; }