remove legacy code in RegionPersister (pingcap#6781)

ref pingcap#6728

Signed-off-by: ywqzzy <[email protected]>

dbms/src/Common/FailPoint.cpp

@@ -43,8 +43,6 @@ std::unordered_map<String, std::shared_ptr<FailPointChannel>> FailPointHelper::f
     M(region_exception_after_read_from_storage_all_error)         \
     M(exception_before_dmfile_remove_encryption)                  \
     M(exception_before_dmfile_remove_from_disk)                   \
-    M(force_enable_region_persister_compatible_mode)              \
-    M(force_disable_region_persister_compatible_mode)             \
     M(force_triggle_background_merge_delta)                       \
     M(force_triggle_foreground_flush)                             \
     M(exception_before_mpp_register_non_root_mpp_task)            \

dbms/src/Interpreters/Context.cpp

@@ -554,7 +554,6 @@ void Context::setPathPool(
     const Strings & main_data_paths,
     const Strings & latest_data_paths,
     const Strings & kvstore_paths,
-    bool enable_raft_compatible_mode,
     PathCapacityMetricsPtr global_capacity_,
     FileProviderPtr file_provider_)
 {
@@ -564,8 +563,7 @@ void Context::setPathPool(
         latest_data_paths,
         kvstore_paths,
         global_capacity_,
-        file_provider_,
-        enable_raft_compatible_mode);
+        file_provider_);
 }
 
 void Context::setConfig(const ConfigurationPtr & config)

dbms/src/Interpreters/Context.h

@@ -203,7 +203,6 @@ class Context
     void setPathPool(const Strings & main_data_paths,
                      const Strings & latest_data_paths,
                      const Strings & kvstore_paths,
-                     bool enable_raft_compatible_mode,
                      PathCapacityMetricsPtr global_capacity_,
                      FileProviderPtr file_provider);
 

dbms/src/Server/DTTool/DTTool.h

@@ -108,7 +108,6 @@ class ImitativeEnv
             /*main_data_paths*/ {path},
             /*latest_data_paths*/ {path},
             /*kvstore_paths*/ Strings{},
-            /*enable_raft_compatible_mode*/ true,
             global_context->getPathCapacity(),
             global_context->getFileProvider());
         TiFlashRaftConfig raft_config;

dbms/src/Server/RaftConfigParser.cpp

@@ -82,12 +82,6 @@ TiFlashRaftConfig TiFlashRaftConfig::parseSettings(Poco::Util::AbstractConfigura
             res.engine = DEFAULT_ENGINE;
     }
 
-    // just for test
-    if (config.has("raft.enable_compatible_mode"))
-    {
-        res.enable_compatible_mode = config.getBool("raft.enable_compatible_mode");
-    }
-
     LOG_INFO(log, "Default storage engine [type={}]", static_cast<Int64>(res.engine));
 
     return res;

dbms/src/Server/RaftConfigParser.h

@@ -39,10 +39,6 @@ struct TiFlashRaftConfig
     // Actually it is "flash.service_addr"
     std::string flash_server_addr;
 
-    // Use PageStorage V1 for kvstore or not.
-    // TODO: remove this config
-    bool enable_compatible_mode = true;
-
     bool for_unit_test = false;
 
     static constexpr TiDB::StorageEngine DEFAULT_ENGINE = TiDB::StorageEngine::DT;

dbms/src/Server/Server.cpp

@@ -967,7 +967,6 @@ int Server::main(const std::vector<std::string> & /*args*/)
         storage_config.main_data_paths, //
         storage_config.latest_data_paths, //
         storage_config.kvstore_data_path, //
-        raft_config.enable_compatible_mode, //
         global_context->getPathCapacity(),
         global_context->getFileProvider());
 

dbms/src/Storages/Page/V3/tests/gtest_page_storage_mix_mode.cpp

@@ -43,7 +43,7 @@ class PageStorageMixedTest : public DB::base::TiFlashStorageTestBasic
 
         auto & global_context = TiFlashTestEnv::getGlobalContext();
 
-        storage_path_pool_v3 = std::make_unique<PathPool>(Strings{path}, Strings{path}, Strings{}, std::make_shared<PathCapacityMetrics>(0, paths, caps, Strings{}, caps), global_context.getFileProvider(), true);
+        storage_path_pool_v3 = std::make_unique<PathPool>(Strings{path}, Strings{path}, Strings{}, std::make_shared<PathCapacityMetrics>(0, paths, caps, Strings{}, caps), global_context.getFileProvider());
 
         global_context.setPageStorageRunMode(PageStorageRunMode::MIX_MODE);
     }

dbms/src/Storages/PathPool.cpp

@@ -58,12 +58,10 @@ PathPool::PathPool(
     const Strings & latest_data_paths_,
     const Strings & kvstore_paths_, //
     PathCapacityMetricsPtr global_capacity_,
-    FileProviderPtr file_provider_,
-    bool enable_raft_compatible_mode_)
+    FileProviderPtr file_provider_)
     : main_data_paths(main_data_paths_)
     , latest_data_paths(latest_data_paths_)
     , kvstore_paths(kvstore_paths_)
-    , enable_raft_compatible_mode(enable_raft_compatible_mode_)
     , global_capacity(global_capacity_)
     , file_provider(file_provider_)
     , log(Logger::get("PathPool"))

dbms/src/Storages/PathPool.h

@@ -56,15 +56,11 @@ class PathPool
         const Strings & latest_data_paths,
         const Strings & kvstore_paths,
         PathCapacityMetricsPtr global_capacity_,
-        FileProviderPtr file_provider_,
-        bool enable_raft_compatible_mode_ = false);
+        FileProviderPtr file_provider_);
 
     // Constructor to create PathPool for one Storage
     StoragePathPool withTable(const String & database_, const String & table_, bool path_need_database_name_) const;
 
-    // TODO: remove this outdated code
-    bool isRaftCompatibleModeEnabled() const { return enable_raft_compatible_mode; }
-
     // Generate a delegator for managing the paths of `RegionPersister`.
     // Those paths are generated from `kvstore_paths`.
     // User should keep the pointer to track the PageFileID -> path index mapping.
@@ -133,8 +129,6 @@ class PathPool
     Strings kvstore_paths;
     Strings global_page_paths;
 
-    bool enable_raft_compatible_mode;
-
     PathCapacityMetricsPtr global_capacity;
 
     FileProviderPtr file_provider;

dbms/src/Storages/Transaction/RegionPersister.cpp

@@ -41,26 +41,11 @@ namespace ErrorCodes
 extern const int LOGICAL_ERROR;
 } // namespace ErrorCodes
 
-namespace FailPoints
-{
-extern const char force_enable_region_persister_compatible_mode[];
-extern const char force_disable_region_persister_compatible_mode[];
-} // namespace FailPoints
-
 void RegionPersister::drop(RegionID region_id, const RegionTaskLock &)
 {
-    if (page_writer)
-    {
-        DB::WriteBatch wb_v2{ns_id};
-        wb_v2.delPage(region_id);
-        page_writer->write(std::move(wb_v2), global_context.getWriteLimiter());
-    }
-    else
-    {
-        PS::V1::WriteBatch wb_v1;
-        wb_v1.delPage(region_id);
-        stable_page_storage->write(std::move(wb_v1));
-    }
+    DB::WriteBatch wb_v2{ns_id};
+    wb_v2.delPage(region_id);
+    page_writer->write(std::move(wb_v2), global_context.getWriteLimiter());
 }
 
 void RegionPersister::computeRegionWriteBuffer(const Region & region, RegionCacheWriteElement & region_write_buffer)
@@ -108,18 +93,9 @@ void RegionPersister::doPersist(RegionCacheWriteElement & region_write_buffer, c
 
     std::lock_guard lock(mutex);
 
-    if (page_reader)
-    {
-        auto entry = page_reader->getPageEntry(region_id);
-        if (entry.isValid() && entry.tag > applied_index)
-            return;
-    }
-    else
-    {
-        auto entry = stable_page_storage->getEntry(region_id, nullptr);
-        if (entry.isValid() && entry.tag > applied_index)
-            return;
-    }
+    auto entry = page_reader->getPageEntry(region_id);
+    if (entry.isValid() && entry.tag > applied_index)
+        return;
 
     if (region.isPendingRemove())
     {
@@ -128,18 +104,9 @@ void RegionPersister::doPersist(RegionCacheWriteElement & region_write_buffer, c
     }
 
     auto read_buf = buffer.tryGetReadBuffer();
-    if (page_writer)
-    {
-        DB::WriteBatch wb{ns_id};
-        wb.putPage(region_id, applied_index, read_buf, region_size);
-        page_writer->write(std::move(wb), global_context.getWriteLimiter());
-    }
-    else
-    {
-        PS::V1::WriteBatch wb;
-        wb.putPage(region_id, applied_index, read_buf, region_size);
-        stable_page_storage->write(std::move(wb));
-    }
+    DB::WriteBatch wb{ns_id};
+    wb.putPage(region_id, applied_index, read_buf, region_size);
+    page_writer->write(std::move(wb), global_context.getWriteLimiter());
 }
 
 RegionPersister::RegionPersister(Context & global_context_, const RegionManager & region_manager_)
@@ -150,32 +117,9 @@ RegionPersister::RegionPersister(Context & global_context_, const RegionManager
 
 PageStorageConfig RegionPersister::getPageStorageSettings() const
 {
-    if (!page_writer)
-    {
-        throw Exception("Not support for PS v1", ErrorCodes::LOGICAL_ERROR);
-    }
-
     return page_writer->getSettings();
 }
 
-PS::V1::PageStorage::Config getV1PSConfig(const PageStorageConfig & config)
-{
-    PS::V1::PageStorage::Config c;
-    c.sync_on_write = config.sync_on_write;
-    c.file_roll_size = config.file_roll_size;
-    c.file_max_size = config.file_max_size;
-    c.file_small_size = config.file_max_size;
-
-    c.merge_hint_low_used_rate = config.gc_max_valid_rate;
-    c.merge_hint_low_used_file_total_size = config.gc_min_bytes;
-    c.merge_hint_low_used_file_num = config.gc_min_files;
-    c.gc_compact_legacy_min_num = config.gc_min_legacy_num;
-
-    c.version_set_config.compact_hint_delta_deletions = config.version_set_config.compact_hint_delta_deletions;
-    c.version_set_config.compact_hint_delta_entries = config.version_set_config.compact_hint_delta_entries;
-    return c;
-}
-
 void RegionPersister::forceTransformKVStoreV2toV3()
 {
     assert(page_reader != nullptr);
@@ -229,36 +173,23 @@ RegionMap RegionPersister::restore(PathPool & path_pool, const TiFlashRaftProxyH
         {
             // If there is no PageFile with basic version binary format, use version 2 of PageStorage.
             auto detect_binary_version = DB::PS::V2::PageStorage::getMaxDataVersion(provider, delegator);
-            bool use_v1_format = path_pool.isRaftCompatibleModeEnabled() && (detect_binary_version == PageFormat::V1);
-
-            fiu_do_on(FailPoints::force_enable_region_persister_compatible_mode, { use_v1_format = true; });
-            fiu_do_on(FailPoints::force_disable_region_persister_compatible_mode, { use_v1_format = false; });
-
-            if (!use_v1_format)
-            {
-                mergeConfigFromSettings(global_context.getSettingsRef(), config);
-                config.num_write_slots = 4; // extend write slots to 4 at least
-
-                auto page_storage_v2 = std::make_shared<PS::V2::PageStorage>(
-                    "RegionPersister",
-                    delegator,
-                    config,
-                    provider,
-                    global_context.getPSBackgroundPool());
-                page_storage_v2->restore();
-                page_writer = std::make_shared<PageWriter>(global_run_mode, page_storage_v2, /*storage_v3_*/ nullptr);
-                page_reader = std::make_shared<PageReader>(global_run_mode, ns_id, page_storage_v2, /*storage_v3_*/ nullptr, /*readlimiter*/ global_context.getReadLimiter());
-            }
-            else
+            if (detect_binary_version == PageFormat::V1)
             {
-                LOG_INFO(log, "RegionPersister running in v1 mode");
-                auto c = getV1PSConfig(config);
-                stable_page_storage = std::make_unique<PS::V1::PageStorage>(
-                    "RegionPersister",
-                    delegator->defaultPath(),
-                    c,
-                    provider);
+                LOG_WARNING(log, "Detect V1 format data, and we will read it using V2 format code.");
             }
+
+            mergeConfigFromSettings(global_context.getSettingsRef(), config);
+            config.num_write_slots = 4; // extend write slots to 4 at least
+
+            auto page_storage_v2 = std::make_shared<PS::V2::PageStorage>(
+                "RegionPersister",
+                delegator,
+                config,
+                provider,
+                global_context.getPSBackgroundPool());
+            page_storage_v2->restore();
+            page_writer = std::make_shared<PageWriter>(global_run_mode, page_storage_v2, /*storage_v3_*/ nullptr);
+            page_reader = std::make_shared<PageReader>(global_run_mode, ns_id, page_storage_v2, /*storage_v3_*/ nullptr, /*readlimiter*/ global_context.getReadLimiter());
             break;
         }
         case PageStorageRunMode::ONLY_V3:
@@ -339,51 +270,32 @@ RegionMap RegionPersister::restore(PathPool & path_pool, const TiFlashRaftProxyH
     }
 
     RegionMap regions;
-    if (page_reader)
-    {
-        auto acceptor = [&](const DB::Page & page) {
-            // We will traverse the pages in V3 before traverse the pages in V2 When we used MIX MODE
-            // If we found the page_id has been restored, just skip it.
-            if (const auto it = regions.find(page.page_id); it != regions.end())
-            {
-                LOG_INFO(log, "Already exist [page_id={}], skip it.", page.page_id);
-                return;
-            }
+    auto acceptor = [&](const DB::Page & page) {
+        // We will traverse the pages in V3 before traverse the pages in V2 When we used MIX MODE
+        // If we found the page_id has been restored, just skip it.
+        if (const auto it = regions.find(page.page_id); it != regions.end())
+        {
+            LOG_INFO(log, "Already exist [page_id={}], skip it.", page.page_id);
+            return;
+        }
 
-            ReadBufferFromMemory buf(page.data.begin(), page.data.size());
-            auto region = Region::deserialize(buf, proxy_helper);
-            if (page.page_id != region->id())
-                throw Exception("region id and page id not match!", ErrorCodes::LOGICAL_ERROR);
+        ReadBufferFromMemory buf(page.data.begin(), page.data.size());
+        auto region = Region::deserialize(buf, proxy_helper);
+        if (page.page_id != region->id())
+            throw Exception("region id and page id not match!", ErrorCodes::LOGICAL_ERROR);
 
-            regions.emplace(page.page_id, region);
-        };
-        page_reader->traverse(acceptor);
-    }
-    else
-    {
-        auto acceptor = [&](const PS::V1::Page & page) {
-            ReadBufferFromMemory buf(page.data.begin(), page.data.size());
-            auto region = Region::deserialize(buf, proxy_helper);
-            if (page.page_id != region->id())
-                throw Exception("region id and page id not match!", ErrorCodes::LOGICAL_ERROR);
-            regions.emplace(page.page_id, region);
-        };
-        stable_page_storage->traverse(acceptor, nullptr);
-    }
+        regions.emplace(page.page_id, region);
+    };
+    page_reader->traverse(acceptor);
 
     return regions;
 }
 
 bool RegionPersister::gc()
 {
-    if (page_writer)
-    {
-        PageStorageConfig config = getConfigFromSettings(global_context.getSettingsRef());
-        page_writer->reloadSettings(config);
-        return page_writer->gc(false, nullptr, nullptr);
-    }
-    else
-        return stable_page_storage->gc();
+    PageStorageConfig config = getConfigFromSettings(global_context.getSettingsRef());
+    page_writer->reloadSettings(config);
+    return page_writer->gc(false, nullptr, nullptr);
 }
 
 FileUsageStatistics RegionPersister::getFileUsageStatistics() const

dbms/src/Storages/Transaction/RegionPersister.h

@@ -78,8 +78,6 @@ class RegionPersister final : private boost::noncopyable
     PageWriterPtr page_writer;
     PageReaderPtr page_reader;
 
-    std::shared_ptr<PS::V1::PageStorage> stable_page_storage;
-
     NamespaceId ns_id = KVSTORE_NAMESPACE_ID;
     const RegionManager & region_manager;
     std::mutex mutex;