Check with owner manager

Signed-off-by: JaySon-Huang <[email protected]>
pingcap · Feb 28, 2023 · 337bf4f · 337bf4f
1 parent 2288bdd
commit 337bf4f
Show file tree

Hide file tree

Showing 9 changed files with 108 additions and 77 deletions.
diff --git a/contrib/client-c b/contrib/client-c
diff --git a/dbms/src/Storages/Page/V3/Remote/RemoteDataLocation.h b/dbms/src/Storages/Page/V3/Remote/RemoteDataLocation.h
@@ -1,3 +1,17 @@
+// Copyright 2023 PingCAP, Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 #include <common/types.h>
 
 namespace DB::Remote

diff --git a/dbms/src/Storages/S3/S3GCManager.cpp b/dbms/src/Storages/S3/S3GCManager.cpp
@@ -23,6 +23,7 @@
 #include <Storages/S3/S3Filename.h>
 #include <Storages/S3/S3GCManager.h>
 #include <Storages/Transaction/Types.h>
+#include <TiDB/OwnerManager.h>
 #include <aws/core/utils/DateTime.h>
 #include <aws/s3/S3Client.h>
 #include <aws/s3/model/CommonPrefix.h>
@@ -49,10 +50,12 @@ namespace DB::S3
 S3GCManager::S3GCManager(
     pingcap::pd::ClientPtr pd_client_,
     std::shared_ptr<TiFlashS3Client> client_,
+    OwnerManagerPtr gc_owner_manager_,
     S3LockClientPtr lock_client_,
     S3GCConfig config_)
     : pd_client(std::move(pd_client_))
     , client(std::move(client_))
+    , gc_owner_manager(std::move(gc_owner_manager_))
     , lock_client(std::move(lock_client_))
     , shutdown_called(false)
     , config(config_)
@@ -77,8 +80,7 @@ bool S3GCManager::runOnAllStores()
 {
     // Only the GC Manager node run the GC logic
     // TODO: keep a pointer of OwnerManager and check it here
-    bool is_gc_owner = true;
-    if (!is_gc_owner)
+    if (bool is_gc_owner = gc_owner_manager->isOwner(); !is_gc_owner)
     {
         return false;
     }
@@ -437,12 +439,13 @@ String S3GCManager::getTemporaryDownloadFile(String s3_key)
 S3GCManagerService::S3GCManagerService(
     Context & context,
     pingcap::pd::ClientPtr pd_client,
+    OwnerManagerPtr gc_owner_manager_,
     S3LockClientPtr lock_client,
     const S3GCConfig & config)
     : global_ctx(context.getGlobalContext())
 {
     auto s3_client = S3::ClientFactory::instance().createWithBucket();
-    manager = std::make_unique<S3GCManager>(std::move(pd_client), std::move(s3_client), std::move(lock_client), config);
+    manager = std::make_unique<S3GCManager>(std::move(pd_client), std::move(s3_client), std::move(gc_owner_manager_), std::move(lock_client), config);
 
     timer = global_ctx.getBackgroundPool().addTask(
         [this]() {

diff --git a/dbms/src/Storages/S3/S3GCManager.h b/dbms/src/Storages/S3/S3GCManager.h
@@ -45,6 +45,8 @@ namespace DB
 class Context;
 class Logger;
 using LoggerPtr = std::shared_ptr<Logger>;
+class OwnerManager;
+using OwnerManagerPtr = std::shared_ptr<OwnerManager>;
 } // namespace DB
 
 namespace DB::S3
@@ -76,6 +78,7 @@ class S3GCManager
     explicit S3GCManager(
         pingcap::pd::ClientPtr pd_client_,
         std::shared_ptr<TiFlashS3Client> client_,
+        OwnerManagerPtr gc_owner_manager_,
         S3LockClientPtr lock_client_,
         S3GCConfig config_);
 
@@ -122,6 +125,7 @@ class S3GCManager
 
     const std::shared_ptr<TiFlashS3Client> client;
 
+    const OwnerManagerPtr gc_owner_manager;
     const S3LockClientPtr lock_client;
 
     std::atomic<bool> shutdown_called;
@@ -137,6 +141,7 @@ class S3GCManagerService
     explicit S3GCManagerService(
         Context & context,
         pingcap::pd::ClientPtr pd_client,
+        OwnerManagerPtr gc_owner_manager_,
         S3LockClientPtr lock_client,
         const S3GCConfig & config);
 

diff --git a/dbms/src/Storages/S3/tests/gtest_s3gcmanager.cpp b/dbms/src/Storages/S3/tests/gtest_s3gcmanager.cpp
@@ -21,6 +21,7 @@
 #include <TestUtils/MockS3Client.h>
 #include <TestUtils/TiFlashTestBasic.h>
 #include <TestUtils/TiFlashTestEnv.h>
+#include <TiDB/OwnerManager.h>
 #include <aws/core/utils/DateTime.h>
 #include <gtest/gtest.h>
 #include <pingcap/pd/MockPDClient.h>
@@ -42,6 +43,22 @@ class S3GCManagerTest : public ::testing::Test
     {
     }
 
+    void SetUp() override
+    {
+        S3GCConfig config{
+            .manifest_expired_hour = 1,
+            .delmark_expired_hour = 1,
+            .temp_path = tests::TiFlashTestEnv::getTemporaryPath(),
+        };
+        mock_s3_client = std::make_shared<MockS3Client>();
+        auto mock_gc_owner = OwnerManager::createMockOwner("owner_0");
+        auto mock_lock_client = std::make_shared<MockS3LockClient>(mock_s3_client);
+        auto mock_pd_client = std::make_shared<pingcap::pd::MockPDClient>();
+        gc_mgr = std::make_unique<S3GCManager>(mock_pd_client, mock_s3_client, mock_gc_owner, mock_lock_client, config);
+    }
+
+    std::shared_ptr<MockS3Client> mock_s3_client;
+    std::unique_ptr<S3GCManager> gc_mgr;
     LoggerPtr log;
 };
 
@@ -74,18 +91,9 @@ try
     ASSERT_EQ(set.latestUploadSequence(), 81);
     ASSERT_EQ(set.latestManifestKey(), S3Filename::newCheckpointManifest(store_id, 81).toFullKey());
 
-    S3GCConfig config{
-        .manifest_expired_hour = 1,
-        .delmark_expired_hour = 1,
-        .temp_path = tests::TiFlashTestEnv::getTemporaryPath(),
-    };
-    auto mock_client = std::make_shared<MockS3Client>();
-    auto mock_lock_client = std::make_shared<MockS3LockClient>(mock_client);
-    auto mock_pd_client = std::make_shared<pingcap::pd::MockPDClient>();
-    S3GCManager gc_mgr(mock_pd_client, mock_client, mock_lock_client, config);
-    gc_mgr.removeOutdatedManifest(set, &timepoint);
+    gc_mgr->removeOutdatedManifest(set, &timepoint);
 
-    auto delete_keys = mock_client->delete_keys;
+    auto delete_keys = mock_s3_client->delete_keys;
     ASSERT_EQ(delete_keys.size(), 2);
     ASSERT_EQ(delete_keys[0], S3Filename::newCheckpointManifest(store_id, 4).toFullKey());
     ASSERT_EQ(delete_keys[1], S3Filename::newCheckpointManifest(store_id, 5).toFullKey());
@@ -96,33 +104,23 @@ CATCH
 TEST_F(S3GCManagerTest, RemoveDataFile)
 try
 {
-    S3GCConfig config{
-        .manifest_expired_hour = 1,
-        .delmark_expired_hour = 1,
-        .temp_path = tests::TiFlashTestEnv::getTemporaryPath(),
-    };
-    auto mock_client = std::make_shared<MockS3Client>();
-    auto mock_lock_client = std::make_shared<MockS3LockClient>(mock_client);
-    auto mock_pd_client = std::make_shared<pingcap::pd::MockPDClient>();
-    S3GCManager gc_mgr(mock_pd_client, mock_client, mock_lock_client, config);
-
     auto timepoint = Aws::Utils::DateTime("2023-02-01T08:00:00Z", Aws::Utils::DateFormat::ISO_8601);
     {
         // delmark expired
         auto delmark_mtime = timepoint - std::chrono::milliseconds(3601 * 1000);
-        gc_mgr.removeDataFileIfDelmarkExpired("datafile_key", "datafile_key.del", timepoint, delmark_mtime);
+        gc_mgr->removeDataFileIfDelmarkExpired("datafile_key", "datafile_key.del", timepoint, delmark_mtime);
 
-        auto delete_keys = mock_client->delete_keys;
+        auto delete_keys = mock_s3_client->delete_keys;
         ASSERT_EQ(delete_keys.size(), 2);
         ASSERT_EQ(delete_keys[0], "datafile_key");
         ASSERT_EQ(delete_keys[1], "datafile_key.del");
     }
     {
         // delmark not expired
         auto delmark_mtime = timepoint - std::chrono::milliseconds(3599 * 1000);
-        gc_mgr.removeDataFileIfDelmarkExpired("datafile_key", "datafile_key.del", timepoint, delmark_mtime);
+        gc_mgr->removeDataFileIfDelmarkExpired("datafile_key", "datafile_key.del", timepoint, delmark_mtime);
 
-        auto delete_keys = mock_client->delete_keys;
+        auto delete_keys = mock_s3_client->delete_keys;
         ASSERT_EQ(delete_keys.size(), 2);
         ASSERT_EQ(delete_keys[0], "datafile_key");
         ASSERT_EQ(delete_keys[1], "datafile_key.del");
@@ -134,16 +132,6 @@ CATCH
 TEST_F(S3GCManagerTest, RemoveLock)
 try
 {
-    S3GCConfig config{
-        .manifest_expired_hour = 1,
-        .delmark_expired_hour = 1,
-        .temp_path = tests::TiFlashTestEnv::getTemporaryPath(),
-    };
-    auto mock_client = std::make_shared<MockS3Client>();
-    auto mock_lock_client = std::make_shared<MockS3LockClient>(mock_client);
-    auto mock_pd_client = std::make_shared<pingcap::pd::MockPDClient>();
-    S3GCManager gc_mgr(mock_pd_client, mock_client, mock_lock_client, config);
-
     StoreID store_id = 20;
     auto df = S3Filename::newCheckpointData(store_id, 300, 1);
 
@@ -153,59 +141,59 @@ try
     auto timepoint = Aws::Utils::DateTime("2023-02-01T08:00:00Z", Aws::Utils::DateFormat::ISO_8601);
     {
         // delmark not exist, and no more lockfile
-        mock_client->clear();
-        gc_mgr.cleanOneLock(lock_key, lock_view, timepoint);
+        mock_s3_client->clear();
+        gc_mgr->cleanOneLock(lock_key, lock_view, timepoint);
 
         // lock is deleted and delmark is created
-        auto delete_keys = mock_client->delete_keys;
+        auto delete_keys = mock_s3_client->delete_keys;
         ASSERT_EQ(delete_keys.size(), 1);
         ASSERT_EQ(delete_keys[0], lock_key);
-        auto put_keys = mock_client->put_keys;
+        auto put_keys = mock_s3_client->put_keys;
         ASSERT_EQ(put_keys.size(), 1);
         ASSERT_EQ(put_keys[0], df.toView().getDelMarkKey());
     }
     {
         // delmark not exist, but still locked by another lockfile
-        mock_client->clear();
+        mock_s3_client->clear();
         auto another_lock_key = df.toView().getLockKey(store_id + 1, 450);
-        mock_client->list_result = {another_lock_key};
-        gc_mgr.cleanOneLock(lock_key, lock_view, timepoint);
+        mock_s3_client->list_result = {another_lock_key};
+        gc_mgr->cleanOneLock(lock_key, lock_view, timepoint);
 
         // lock is deleted and delmark is created
-        auto delete_keys = mock_client->delete_keys;
+        auto delete_keys = mock_s3_client->delete_keys;
         ASSERT_EQ(delete_keys.size(), 1);
         ASSERT_EQ(delete_keys[0], lock_key);
-        auto put_keys = mock_client->put_keys;
+        auto put_keys = mock_s3_client->put_keys;
         ASSERT_EQ(put_keys.size(), 0);
     }
     {
         // delmark exist, not expired
-        mock_client->clear();
+        mock_s3_client->clear();
         auto delmark_mtime = timepoint - std::chrono::milliseconds(3599 * 1000);
-        mock_client->head_result_mtime = delmark_mtime;
-        gc_mgr.cleanOneLock(lock_key, lock_view, timepoint);
+        mock_s3_client->head_result_mtime = delmark_mtime;
+        gc_mgr->cleanOneLock(lock_key, lock_view, timepoint);
 
         // lock is deleted, datafile and delmark remain
-        auto delete_keys = mock_client->delete_keys;
+        auto delete_keys = mock_s3_client->delete_keys;
         ASSERT_EQ(delete_keys.size(), 1);
         ASSERT_EQ(delete_keys[0], lock_key);
-        auto put_keys = mock_client->put_keys;
+        auto put_keys = mock_s3_client->put_keys;
         ASSERT_EQ(put_keys.size(), 0);
     }
     {
         // delmark exist, expired
-        mock_client->clear();
+        mock_s3_client->clear();
         auto delmark_mtime = timepoint - std::chrono::milliseconds(3601 * 1000);
-        mock_client->head_result_mtime = delmark_mtime;
-        gc_mgr.cleanOneLock(lock_key, lock_view, timepoint);
+        mock_s3_client->head_result_mtime = delmark_mtime;
+        gc_mgr->cleanOneLock(lock_key, lock_view, timepoint);
 
         // lock datafile and delmark are deleted
-        auto delete_keys = mock_client->delete_keys;
+        auto delete_keys = mock_s3_client->delete_keys;
         ASSERT_EQ(delete_keys.size(), 3);
         ASSERT_EQ(delete_keys[0], lock_key);
         ASSERT_EQ(delete_keys[1], df.toFullKey());
         ASSERT_EQ(delete_keys[2], df.toView().getDelMarkKey());
-        auto put_keys = mock_client->put_keys;
+        auto put_keys = mock_s3_client->put_keys;
         ASSERT_EQ(put_keys.size(), 0);
     }
 }
@@ -214,16 +202,6 @@ CATCH
 TEST_F(S3GCManagerTest, ScanLocks)
 try
 {
-    S3GCConfig config{
-        .manifest_expired_hour = 1,
-        .delmark_expired_hour = 1,
-        .temp_path = tests::TiFlashTestEnv::getTemporaryPath(),
-    };
-    auto mock_client = std::make_shared<MockS3Client>();
-    auto mock_lock_client = std::make_shared<MockS3LockClient>(mock_client);
-    auto mock_pd_client = std::make_shared<pingcap::pd::MockPDClient>();
-    S3GCManager gc_mgr(mock_pd_client, mock_client, mock_lock_client, config);
-
     StoreID store_id = 20;
     StoreID lock_store_id = 21;
     UInt64 safe_sequence = 100;
@@ -258,19 +236,19 @@ try
             expected_created_delmark = df.toView().getDelMarkKey();
             keys.emplace_back(lock_key);
         }
-        mock_client->clear();
-        mock_client->list_result = keys; // set for `LIST`
+        mock_s3_client->clear();
+        mock_s3_client->list_result = keys; // set for `LIST`
     }
 
     {
         auto timepoint = Aws::Utils::DateTime("2023-02-01T08:00:00Z", Aws::Utils::DateFormat::ISO_8601);
-        gc_mgr.cleanUnusedLocks(lock_store_id, S3Filename::getLockPrefix(), safe_sequence, valid_lock_files, timepoint);
+        gc_mgr->cleanUnusedLocks(lock_store_id, S3Filename::getLockPrefix(), safe_sequence, valid_lock_files, timepoint);
 
         // lock is deleted and delmark is created
-        auto delete_keys = mock_client->delete_keys;
+        auto delete_keys = mock_s3_client->delete_keys;
         ASSERT_EQ(delete_keys.size(), 1);
         ASSERT_EQ(delete_keys[0], expected_deleted_lock_key);
-        auto put_keys = mock_client->put_keys;
+        auto put_keys = mock_s3_client->put_keys;
         ASSERT_EQ(put_keys.size(), 1);
         ASSERT_EQ(put_keys[0], expected_created_delmark);
     }

diff --git a/dbms/src/Storages/Transaction/TMTContext.cpp b/dbms/src/Storages/Transaction/TMTContext.cpp
@@ -102,7 +102,7 @@ TMTContext::TMTContext(Context & context_, const TiFlashRaftConfig & raft_config
 
         S3::S3GCConfig gc_config;
         gc_config.temp_path = context.getTemporaryPath(); // TODO: add suffix for it?
-        s3gc_manager = std::make_unique<S3::S3GCManagerService>(context, cluster->pd_client, s3_lock_client, gc_config);
+        s3gc_manager = std::make_unique<S3::S3GCManagerService>(context, cluster->pd_client, s3gc_owner, s3_lock_client, gc_config);
     }
 }
 
@@ -146,6 +146,12 @@ void TMTContext::shutdown()
         s3gc_owner = nullptr;
     }
 
+    if (s3gc_manager)
+    {
+        s3gc_manager->shutdown();
+        s3gc_manager = nullptr;
+    }
+
     if (background_service)
     {
         background_service->shutdown();

diff --git a/dbms/src/TestUtils/MockS3Client.cpp b/dbms/src/TestUtils/MockS3Client.cpp
@@ -1,3 +1,17 @@
+// Copyright 2023 PingCAP, Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 #include <Common/StringUtils/StringUtils.h>
 #include <TestUtils/MockS3Client.h>
 #include <aws/core/AmazonWebServiceRequest.h>

diff --git a/dbms/src/TestUtils/MockS3Client.h b/dbms/src/TestUtils/MockS3Client.h
@@ -1,3 +1,17 @@
+// Copyright 2023 PingCAP, Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 #include <Core/Types.h>
 #include <Storages/S3/S3Common.h>
 #include <aws/core/utils/DateTime.h>

diff --git a/dbms/src/TiDB/OwnerManager.h b/dbms/src/TiDB/OwnerManager.h
@@ -51,9 +51,6 @@ using LeaderKey = v3electionpb::LeaderKey;
 } // namespace Etcd
 
 class OwnerManager;
-// Now owner manager is created in TMTContext, but
-// used in S3LockService. It is hard to find out
-// which will be shutdown first. So use shared_ptr now.
 using OwnerManagerPtr = std::shared_ptr<OwnerManager>;
 
 namespace tests
+1 −0		include/pingcap/coprocessor/Client.h
+8 −2		include/pingcap/kv/RegionCache.h
+30 −11		src/coprocessor/Client.cc
+27 −4		src/kv/RegionCache.cc
+3 −0		src/test/batch_coprocessor_test.cc