From 99c0530bb6d1df0d974ad17f1ad977538d3698b3 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Fri, 23 Sep 2022 15:19:43 +0800 Subject: [PATCH] Fix invalid storage dir configurations lead to unexpected behavior (#4105) (#4131) close pingcap/tics#4093 --- dbms/src/Common/tests/gtest_cpptoml.cpp | 140 +++++++++ dbms/src/Server/StorageConfigParser.cpp | 76 +++-- .../src/Server/tests/gtest_storage_config.cpp | 276 ++++++++++++++++-- dbms/src/Storages/PathCapacityMetrics.cpp | 37 ++- 4 files changed, 466 insertions(+), 63 deletions(-) create mode 100644 dbms/src/Common/tests/gtest_cpptoml.cpp diff --git a/dbms/src/Common/tests/gtest_cpptoml.cpp b/dbms/src/Common/tests/gtest_cpptoml.cpp new file mode 100644 index 00000000000..9cd216b8167 --- /dev/null +++ b/dbms/src/Common/tests/gtest_cpptoml.cpp @@ -0,0 +1,140 @@ +#include +#include +#include + +#include + +namespace DB::tests +{ +TEST(CPPTomlTest, ContainsQualifiedArray) +{ + auto * log = &Poco::Logger::get("CPPTomlTest"); + + Strings failure_tests = { + R"( +[a] +[a.b] +c = "abc" + )", + R"( +[a] +[a.b] +c = 123 + )", + R"( +[a] +[a.b] +c = 123.45 + )", + }; + + for (size_t i = 0; i < failure_tests.size(); ++i) + { + const auto & test_case = failure_tests[i]; + SCOPED_TRACE("[index=" + std::to_string(i) + "] [content=" + test_case + "]"); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); + + std::istringstream ss(test_case); + cpptoml::parser p(ss); + auto table = p.parse(); + + const char * key = "a.b.c"; + ASSERT_TRUE(table->contains_qualified(key)); + auto qualified = table->get_qualified(key); + ASSERT_TRUE(qualified); + // not array + ASSERT_FALSE(qualified->is_array()); + // try to parse as vector, return false + cpptoml::option array = table->get_qualified_array_of(key); + ASSERT_FALSE(array); + } +} + +TEST(CPPTomlTest, ContainsQualifiedStringArray) +{ + auto * log = &Poco::Logger::get("CPPTomlTest"); + + Strings failure_tests = { + R"( +[a] +[a.b] +c = [["abc", "def"], ["z"]] + )", + R"( +[a] +[a.b] +c = [123, 456] + )", + }; + + for (size_t i = 0; i < failure_tests.size(); ++i) + { + const auto & test_case = failure_tests[i]; + SCOPED_TRACE("[index=" + std::to_string(i) + "] [content=" + test_case + "]"); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); + + std::istringstream ss(test_case); + cpptoml::parser p(ss); + auto table = p.parse(); + + const char * key = "a.b.c"; + ASSERT_TRUE(table->contains_qualified(key)); + auto qualified = table->get_qualified(key); + ASSERT_TRUE(qualified); + // is non-empty array but not string array + ASSERT_TRUE(qualified->is_array()); + auto qualified_array = qualified->as_array(); + ASSERT_NE(qualified_array->begin(), qualified_array->end()); + // try to parse as vector, return false + cpptoml::option array = table->get_qualified_array_of(key); + ASSERT_FALSE(array); + } +} + +TEST(CPPTomlTest, ContainsQualifiedStringArrayOrEmpty) +{ + auto * log = &Poco::Logger::get("CPPTomlTest"); + + Strings failure_tests = { + // a.b.c is not empty + R"( +[a] +[a.b] +c = ["abc", "def", "z"] + )", + // a.b.c is empty + R"( +[a] +[a.b] +c = [] + )", + }; + + for (size_t i = 0; i < failure_tests.size(); ++i) + { + const auto & test_case = failure_tests[i]; + SCOPED_TRACE("[index=" + std::to_string(i) + "] [content=" + test_case + "]"); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); + + std::istringstream ss(test_case); + cpptoml::parser p(ss); + auto table = p.parse(); + + const char * key = "a.b.c"; + ASSERT_TRUE(table->contains_qualified(key)); + auto qualified = table->get_qualified(key); + ASSERT_TRUE(qualified); + // is non-empty array but not string array + ASSERT_TRUE(qualified->is_array()); + + // try to parse as vector, return true + cpptoml::option array = table->get_qualified_array_of(key); + ASSERT_TRUE(array); + if (auto qualified_array = qualified->as_array(); qualified_array->begin() != qualified_array->end()) + { + ASSERT_EQ(array->size(), 3); + } + } +} + +} // namespace DB::tests diff --git a/dbms/src/Server/StorageConfigParser.cpp b/dbms/src/Server/StorageConfigParser.cpp index 3558b25c999..2c6ed64a14d 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -1,4 +1,13 @@ +/// Suppress gcc warning: ‘*((void*)& +4)’ may be used uninitialized in this function +#if !__clang__ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif #include +#if !__clang__ +#pragma GCC diagnostic pop +#endif + #include #include #include @@ -40,13 +49,39 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) cpptoml::parser p(ss); auto table = p.parse(); + auto get_checked_qualified_array = [log](const std::shared_ptr table, const char * key) -> cpptoml::option { + auto throw_invalid_value = [log, key]() { + String error_msg + = String("The configuration \"storage.") + key + "\" should be an array of strings. Please check your configuration file."; + LOG_ERROR(log, error_msg); + throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); + }; + // not exist key + if (!table->contains_qualified(key)) + return cpptoml::option(); + + // key exist, but not array + auto qualified_ptr = table->get_qualified(key); + if (!qualified_ptr->is_array()) + { + throw_invalid_value(); + } + // key exist, but can not convert to string array, maybe it is an int array + auto string_array = table->get_qualified_array_of(key); + if (!string_array) + { + throw_invalid_value(); + } + return string_array; + }; + // main - if (auto main_paths = table->get_qualified_array_of("main.dir"); main_paths) + if (auto main_paths = get_checked_qualified_array(table, "main.dir"); main_paths) main_data_paths = *main_paths; if (auto main_capacity = table->get_qualified_array_of("main.capacity"); main_capacity) { for (const auto & c : *main_capacity) - main_capacity_quota.emplace_back((size_t)c); + main_capacity_quota.emplace_back(static_cast(c)); } if (main_data_paths.empty()) { @@ -72,13 +107,14 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) } // latest - if (auto latest_paths = table->get_qualified_array_of("latest.dir"); latest_paths) + if (auto latest_paths = get_checked_qualified_array(table, "latest.dir"); latest_paths) latest_data_paths = *latest_paths; if (auto latest_capacity = table->get_qualified_array_of("latest.capacity"); latest_capacity) { for (const auto & c : *latest_capacity) - latest_capacity_quota.emplace_back((size_t)c); + latest_capacity_quota.emplace_back(static_cast(c)); } + // If it is empty, use the same dir as "main.dir" if (latest_data_paths.empty()) { LOG_INFO(log, "The configuration \"storage.latest.dir\" is empty, use the same dir and capacity of \"storage.main.dir\""); @@ -87,8 +123,8 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) } if (!latest_capacity_quota.empty() && latest_capacity_quota.size() != latest_data_paths.size()) { - String error_msg = "The array size of \"storage.main.dir\"[size=" + toString(latest_data_paths.size()) - + "] is not euqal to \"storage.main.capacity\"[size=" + toString(latest_capacity_quota.size()) + String error_msg = "The array size of \"storage.latest.dir\"[size=" + toString(latest_data_paths.size()) + + "] is not equal to \"storage.latest.capacity\"[size=" + toString(latest_capacity_quota.size()) + "]. Please check your configuration file."; LOG_ERROR(log, error_msg); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); @@ -103,7 +139,7 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) } // Raft - if (auto kvstore_paths = table->get_qualified_array_of("raft.dir"); kvstore_paths) + if (auto kvstore_paths = get_checked_qualified_array(table, "raft.dir"); kvstore_paths) kvstore_data_path = *kvstore_paths; if (kvstore_data_path.empty()) { @@ -114,11 +150,11 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) kvstore_data_path.emplace_back(std::move(path)); } } - for (size_t i = 0; i < kvstore_data_path.size(); ++i) + for (auto & path : kvstore_data_path) { // normalized - kvstore_data_path[i] = getNormalizedPath(kvstore_data_path[i]); - LOG_INFO(log, "Raft data candidate path: " << kvstore_data_path[i]); + path = getNormalizedPath(path); + LOG_INFO(log, "Raft data candidate path: " << path); } // rate limiter @@ -159,9 +195,9 @@ bool TiFlashStorageConfig::parseFromDeprecatedConfiguration(Poco::Util::LayeredC "The configuration \"path\" is empty! [path=" + config.getString("path") + "]", ErrorCodes::INVALID_CONFIG_PARAMETER); Strings all_normal_path; Poco::StringTokenizer string_tokens(paths, ","); - for (auto it = string_tokens.begin(); it != string_tokens.end(); it++) + for (const auto & string_token : string_tokens) { - all_normal_path.emplace_back(getNormalizedPath(*it)); + all_normal_path.emplace_back(getNormalizedPath(string_token)); } // If you set `path_realtime_mode` to `true` and multiple directories are deployed in the path, the latest data is stored in the first directory and older data is stored in the rest directories. @@ -218,15 +254,16 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc size_t global_capacity_quota = 0; // "0" by default, means no quota, use the whole disk capacity. TiFlashStorageConfig storage_config; + // Always try to parse storage miscellaneous configuration when [storage] section exist. if (config.has("storage")) { - storage_config.parse(config.getString("storage"), log); - if (config.has("path")) LOG_WARNING(log, "The configuration \"path\" is ignored when \"storage\" is defined."); if (config.has("capacity")) LOG_WARNING(log, "The configuration \"capacity\" is ignored when \"storage\" is defined."); + storage_config.parse(config.getString("storage"), log); + if (config.has("raft.kvstore_path")) { Strings & kvstore_paths = storage_config.kvstore_data_path; @@ -236,11 +273,11 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc LOG_WARNING(log, "The configuration \"raft.kvstore_path\" is deprecated. Check \"storage.raft.dir\" for new style."); kvstore_paths.clear(); kvstore_paths.emplace_back(getNormalizedPath(deprecated_kvstore_path)); - for (size_t i = 0; i < kvstore_paths.size(); ++i) + for (auto & kvstore_path : kvstore_paths) { LOG_WARNING(log, "Raft data candidate path: " - << kvstore_paths[i] << ". The path is overwritten by deprecated configuration for backward compatibility."); + << kvstore_path << ". The path is overwritten by deprecated configuration for backward compatibility."); } } } @@ -257,12 +294,11 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc Poco::trimInPlace(capacities); Poco::StringTokenizer string_tokens(capacities, ","); size_t num_token = 0; - for (auto it = string_tokens.begin(); it != string_tokens.end(); ++it) + for (const auto & string_token : string_tokens) { if (num_token == 0) { - const std::string & s = *it; - global_capacity_quota = DB::parse(s.data(), s.size()); + global_capacity_quota = DB::parse(string_token.data(), string_token.size()); } num_token++; } @@ -274,7 +310,7 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc if (!storage_config.parseFromDeprecatedConfiguration(config, log)) { // Can not parse from the deprecated configuration "path". - String msg = "The configuration \"storage\" section is not defined. Please check your configuration file."; + String msg = "The configuration \"storage.main\" section is not defined. Please check your configuration file."; LOG_ERROR(log, msg); throw Exception(msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } diff --git a/dbms/src/Server/tests/gtest_storage_config.cpp b/dbms/src/Server/tests/gtest_storage_config.cpp index 7bbdaa1c186..65af1d833f0 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -10,11 +10,20 @@ #include #undef private +/// Suppress gcc warning: ‘*((void*)& +4)’ may be used uninitialized in this function +#if !__clang__ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif +#include +#if !__clang__ +#pragma GCC diagnostic pop +#endif + namespace DB { namespace tests { - static auto loadConfigFromString(const String & s) { std::istringstream ss(s); @@ -25,10 +34,10 @@ static auto loadConfigFromString(const String & s) return config; } -class StorageConfig_test : public ::testing::Test +class StorageConfigTest : public ::testing::Test { public: - StorageConfig_test() : log(&Poco::Logger::get("StorageConfig_test")) {} + StorageConfigTest() : log(&Poco::Logger::get("StorageConfigTest")) {} static void SetUpTestCase() {} @@ -36,7 +45,118 @@ class StorageConfig_test : public ::testing::Test Poco::Logger * log; }; -TEST_F(StorageConfig_test, MultiSSDSettings) +TEST_F(StorageConfigTest, SimpleSinglePath) +try +{ + Strings tests = { + // Deprecated style + R"( +path="/data0/tiflash" + )", + // Deprecated style with capacity + R"( +path="/data0/tiflash" +capacity=1024000000 + )", + // New style + R"( +[storage] +[storage.main] +dir=["/data0/tiflash"] + )", + }; + + for (size_t i = 0; i < tests.size(); ++i) + { + const auto & test_case = tests[i]; + auto config = loadConfigFromString(test_case); + + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); + + size_t global_capacity_quota = 0; + TiFlashStorageConfig storage; + std::tie(global_capacity_quota, storage) = TiFlashStorageConfig::parseSettings(*config, log); + + ASSERT_EQ(storage.main_data_paths.size(), 1); + EXPECT_EQ(storage.main_data_paths[0], "/data0/tiflash/"); + + ASSERT_EQ(storage.latest_data_paths.size(), 1); + EXPECT_EQ(storage.latest_data_paths[0], "/data0/tiflash/"); + + ASSERT_EQ(storage.kvstore_data_path.size(), 1); + EXPECT_EQ(storage.kvstore_data_path[0], "/data0/tiflash/kvstore/"); + + auto all_paths = storage.getAllNormalPaths(); + EXPECT_EQ(all_paths[0], "/data0/tiflash/"); + + // Ensure that creating PathCapacityMetrics is OK. + PathCapacityMetrics path_capacity(global_capacity_quota, storage.main_data_paths, storage.main_capacity_quota, + storage.latest_data_paths, storage.latest_capacity_quota); + } +} +CATCH + +TEST_F(StorageConfigTest, ExplicitKVStorePath) +try +{ + Strings tests = { + // Deprecated style + R"( +path="/data0/tiflash" +[raft] +kvstore_path="/data1111/kvstore" + )", + // New style + R"( +[storage] +[storage.main] +dir=["/data0/tiflash"] +[storage.raft] +dir=["/data1111/kvstore"] + )", + // New style with remaining `raft.kvstore_path`, will be overwrite for backward compatibility + R"( +[raft] +kvstore_path="/data1111/kvstore" +[storage] +[storage.main] +dir=["/data0/tiflash"] +[storage.raft] +dir=["/data222/kvstore"] + )", + }; + + for (size_t i = 0; i < tests.size(); ++i) + { + const auto & test_case = tests[i]; + auto config = loadConfigFromString(test_case); + + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); + + size_t global_capacity_quota = 0; + TiFlashStorageConfig storage; + std::tie(global_capacity_quota, storage) = TiFlashStorageConfig::parseSettings(*config, log); + + ASSERT_EQ(storage.main_data_paths.size(), 1); + EXPECT_EQ(storage.main_data_paths[0], "/data0/tiflash/"); + + ASSERT_EQ(storage.latest_data_paths.size(), 1); + EXPECT_EQ(storage.latest_data_paths[0], "/data0/tiflash/"); + + ASSERT_EQ(storage.kvstore_data_path.size(), 1); + EXPECT_EQ(storage.kvstore_data_path[0], "/data1111/kvstore/"); + + auto all_paths = storage.getAllNormalPaths(); + EXPECT_EQ(all_paths[0], "/data0/tiflash/"); + + // Ensure that creating PathCapacityMetrics is OK. + PathCapacityMetrics path_capacity(global_capacity_quota, storage.main_data_paths, storage.main_capacity_quota, + storage.latest_data_paths, storage.latest_capacity_quota); + } +} +CATCH + +TEST_F(StorageConfigTest, MultiSSDSettings) try { Strings tests = { @@ -66,13 +186,70 @@ dir=["/data0/tiflash"] TiFlashStorageConfig storage; std::tie(global_capacity_quota, storage) = TiFlashStorageConfig::parseSettings(*config, log); - ASSERT_EQ(storage.main_data_paths.size(), 3UL); + ASSERT_EQ(storage.main_data_paths.size(), 3); + EXPECT_EQ(storage.main_data_paths[0], "/data0/tiflash/"); + EXPECT_EQ(storage.main_data_paths[1], "/data1/tiflash/"); + EXPECT_EQ(storage.main_data_paths[2], "/data2/tiflash/"); + + ASSERT_EQ(storage.latest_data_paths.size(), 1); + EXPECT_EQ(storage.latest_data_paths[0], "/data0/tiflash/"); + + ASSERT_EQ(storage.kvstore_data_path.size(), 1); + EXPECT_EQ(storage.kvstore_data_path[0], "/data0/tiflash/kvstore/"); + + auto all_paths = storage.getAllNormalPaths(); + EXPECT_EQ(all_paths[0], "/data0/tiflash/"); + + // Ensure that creating PathCapacityMetrics is OK. + PathCapacityMetrics path_capacity(global_capacity_quota, storage.main_data_paths, storage.main_capacity_quota, + storage.latest_data_paths, storage.latest_capacity_quota); + } +} +CATCH + +TEST_F(StorageConfigTest, MultiNVMeSSDSettings) +try +{ + Strings tests = { + R"( +[storage] +[storage.main] +dir=["/data0/tiflash", "/data1/tiflash", "/data2/tiflash"] + )", + R"( +[storage] +[storage.main] +dir=["/data0/tiflash", "/data1/tiflash", "/data2/tiflash"] +[storage.latest] +dir=["/data0/tiflash", "/data1/tiflash", "/data2/tiflash"] + )", + }; + + for (size_t i = 0; i < tests.size(); ++i) + { + const auto & test_case = tests[i]; + auto config = loadConfigFromString(test_case); + + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); + + size_t global_capacity_quota = 0; + TiFlashStorageConfig storage; + std::tie(global_capacity_quota, storage) = TiFlashStorageConfig::parseSettings(*config, log); + + ASSERT_EQ(storage.main_data_paths.size(), 3); EXPECT_EQ(storage.main_data_paths[0], "/data0/tiflash/"); EXPECT_EQ(storage.main_data_paths[1], "/data1/tiflash/"); EXPECT_EQ(storage.main_data_paths[2], "/data2/tiflash/"); - ASSERT_EQ(storage.latest_data_paths.size(), 1UL); + ASSERT_EQ(storage.latest_data_paths.size(), 3); EXPECT_EQ(storage.latest_data_paths[0], "/data0/tiflash/"); + EXPECT_EQ(storage.latest_data_paths[1], "/data1/tiflash/"); + EXPECT_EQ(storage.latest_data_paths[2], "/data2/tiflash/"); + + ASSERT_EQ(storage.kvstore_data_path.size(), 3); + EXPECT_EQ(storage.kvstore_data_path[0], "/data0/tiflash/kvstore/"); + EXPECT_EQ(storage.kvstore_data_path[1], "/data1/tiflash/kvstore/"); + EXPECT_EQ(storage.kvstore_data_path[2], "/data2/tiflash/kvstore/"); auto all_paths = storage.getAllNormalPaths(); EXPECT_EQ(all_paths[0], "/data0/tiflash/"); @@ -84,7 +261,7 @@ dir=["/data0/tiflash"] } CATCH -TEST_F(StorageConfig_test, SSD_HDD_Settings) +TEST_F(StorageConfigTest, SSD_HDD_Settings) try { Strings tests = { @@ -114,13 +291,16 @@ dir=["/ssd0/tiflash"] TiFlashStorageConfig storage; std::tie(global_capacity_quota, storage) = TiFlashStorageConfig::parseSettings(*config, log); - ASSERT_EQ(storage.main_data_paths.size(), 2UL); + ASSERT_EQ(storage.main_data_paths.size(), 2); EXPECT_EQ(storage.main_data_paths[0], "/hdd0/tiflash/"); EXPECT_EQ(storage.main_data_paths[1], "/hdd1/tiflash/"); - ASSERT_EQ(storage.latest_data_paths.size(), 1UL); + ASSERT_EQ(storage.latest_data_paths.size(), 1); EXPECT_EQ(storage.latest_data_paths[0], "/ssd0/tiflash/"); + ASSERT_EQ(storage.kvstore_data_path.size(), 1); + EXPECT_EQ(storage.kvstore_data_path[0], "/ssd0/tiflash/kvstore/"); + auto all_paths = storage.getAllNormalPaths(); EXPECT_EQ(all_paths[0], "/ssd0/tiflash/"); @@ -131,7 +311,7 @@ dir=["/ssd0/tiflash"] } CATCH -TEST_F(StorageConfig_test, ParseMaybeBrokenCases) +TEST_F(StorageConfigTest, ParseMaybeBrokenCases) try { Strings tests = { @@ -176,6 +356,57 @@ capacity = [ 10737418240 ] # [storage.raft] # dir = [ ] )", + // case for the length of storage.latest.dir is not the same with storage.latest.capacity + R"( +path = "/data0/tiflash,/data1/tiflash" +[storage] +[storage.main] +dir = [ "/data0/tiflash", "/data1/tiflash" ] +capacity = [ 10737418240, 10737418240 ] +[storage.latest] +dir = [ "/data0/tiflash", "/data1/tiflash" ] +capacity = [ 10737418240 ] + )", + // case for storage.main.dir is not an string array + R"( +[storage] +[storage.main] +dir = "/data0/tiflash,/data1/tiflash" + )", + // case for storage.latest.dir is not an string array + R"( +[storage] +[storage.main] +dir = [ "/data0/tiflash", "/data1/tiflash" ] +[storage.latest] +dir = "/data0/tiflash" + )", + // case for storage.raft.dir is not an string array + R"( +[storage] +[storage.main] +dir = [ "/data0/tiflash", "/data1/tiflash" ] +[storage.raft] +dir = "/data0/tiflash" + )", + // case for storage.main.dir is not an string array + R"( +[storage] +[storage.main] +dir = 123 + )", + // case for storage.main.dir is not an string array + R"( +[storage] +[storage.main] +dir = [["/data0/tiflash", "/data1/tiflash"], ["/data2/tiflash", ]] + )", + // case for storage.main.dir is not an string array + R"( +[storage] +[storage.main] +dir = [1,2,3] + )", }; for (size_t i = 0; i < tests.size(); ++i) @@ -192,7 +423,7 @@ capacity = [ 10737418240 ] } CATCH -TEST(PathCapacityMetrics_test, Quota) +TEST(PathCapacityMetricsTest, Quota) try { Strings tests = { @@ -227,7 +458,7 @@ dir=["/data0/tiflash"] capacity=[ 1024 ] )", }; - Poco::Logger * log = &Poco::Logger::get("PathCapacityMetrics_test"); + Poco::Logger * log = &Poco::Logger::get("PathCapacityMetricsTest"); for (size_t i = 0; i < tests.size(); ++i) { @@ -240,12 +471,12 @@ capacity=[ 1024 ] TiFlashStorageConfig storage; std::tie(global_capacity_quota, storage) = TiFlashStorageConfig::parseSettings(*config, log); - ASSERT_EQ(storage.main_data_paths.size(), 3UL); + ASSERT_EQ(storage.main_data_paths.size(), 3); EXPECT_EQ(storage.main_data_paths[0], "/data0/tiflash/"); EXPECT_EQ(storage.main_data_paths[1], "/data1/tiflash/"); EXPECT_EQ(storage.main_data_paths[2], "/data2/tiflash/"); - ASSERT_EQ(storage.latest_data_paths.size(), 1UL); + ASSERT_EQ(storage.latest_data_paths.size(), 1); EXPECT_EQ(storage.latest_data_paths[0], "/data0/tiflash/"); auto all_paths = storage.getAllNormalPaths(); @@ -261,26 +492,26 @@ capacity=[ 1024 ] { case 0: case 1: - EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 0UL); + EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 0); break; case 2: - EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 2048UL); + EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 2048); break; } idx = path_capacity.locatePath("/data1/tiflash/"); ASSERT_NE(idx, PathCapacityMetrics::INVALID_INDEX); - EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 3072UL); + EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 3072); idx = path_capacity.locatePath("/data2/tiflash/"); ASSERT_NE(idx, PathCapacityMetrics::INVALID_INDEX); - EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 4196UL); + EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 4196); } } CATCH -class UsersConfigParser_test : public ::testing::Test +class UsersConfigParserTest : public ::testing::Test { public: - UsersConfigParser_test() : log(&Poco::Logger::get("UsersConfigParser_test")) {} + UsersConfigParserTest() : log(&Poco::Logger::get("UsersConfigParserTest")) {} static void SetUpTestCase() {} @@ -289,7 +520,7 @@ class UsersConfigParser_test : public ::testing::Test }; -TEST_F(UsersConfigParser_test, ParseConfigs) +TEST_F(UsersConfigParserTest, ParseConfigs) try { Strings tests = { @@ -313,7 +544,7 @@ ip = "::/0" [profiles] [profiles.default] load_balancing = "random" -max_memory_usage = 0 +max_memory_usage = 0 use_uncompressed_cache = 1 [profiles.readonly] readonly = 1 @@ -385,6 +616,5 @@ dt_enable_rough_set_filter = false } CATCH - } // namespace tests } // namespace DB diff --git a/dbms/src/Storages/PathCapacityMetrics.cpp b/dbms/src/Storages/PathCapacityMetrics.cpp index 8ca8b61beb7..0b497b61ce7 100644 --- a/dbms/src/Storages/PathCapacityMetrics.cpp +++ b/dbms/src/Storages/PathCapacityMetrics.cpp @@ -21,19 +21,16 @@ extern const Metric StoreSizeUsed; namespace DB { -inline size_t safeGetQuota(const std::vector & quotas, size_t idx) -{ - return idx < quotas.size() ? quotas[idx] : 0; -} +inline size_t safeGetQuota(const std::vector & quotas, size_t idx) { return idx < quotas.size() ? quotas[idx] : 0; } -PathCapacityMetrics::PathCapacityMetrics( - const size_t capacity_quota_, // will be ignored if `main_capacity_quota` is not empty +PathCapacityMetrics::PathCapacityMetrics(const size_t capacity_quota_, // will be ignored if `main_capacity_quota` is not empty const Strings & main_paths_, - const std::vector main_capacity_quota_, + const std::vector + main_capacity_quota_, const Strings & latest_paths_, - const std::vector latest_capacity_quota_) - : capacity_quota(capacity_quota_) - , log(&Poco::Logger::get("PathCapacityMetrics")) + const std::vector + latest_capacity_quota_) + : capacity_quota(capacity_quota_), log(&Poco::Logger::get("PathCapacityMetrics")) { if (!main_capacity_quota_.empty()) { @@ -80,7 +77,7 @@ void PathCapacityMetrics::addUsedSize(std::string_view file_path, size_t used_by return; } - // Now we expect size of path_infos not change, don't acquire hevay lock on `path_infos` now. + // Now we expect size of path_infos not change, don't acquire heavy lock on `path_infos` now. path_infos[path_idx].used_bytes += used_bytes; } @@ -93,7 +90,7 @@ void PathCapacityMetrics::freeUsedSize(std::string_view file_path, size_t used_b return; } - // Now we expect size of path_infos not change, don't acquire hevay lock on `path_infos` now. + // Now we expect size of path_infos not change, don't acquire heavy lock on `path_infos` now. path_infos[path_idx].used_bytes -= used_bytes; } @@ -106,7 +103,7 @@ FsStats PathCapacityMetrics::getFsStats() const /// and we limit available size by first path. It is good enough for now. // Now we assume the size of `path_infos` will not change, don't acquire heavy lock on `path_infos`. - FsStats total_stat; + FsStats total_stat{}; for (size_t i = 0; i < path_infos.size(); ++i) { FsStats path_stat = path_infos[i].getStats(log); @@ -135,10 +132,10 @@ FsStats PathCapacityMetrics::getFsStats() const // Default threshold "schedule.low-space-ratio" in PD is 0.8, log warning message if avail ratio is low. if (avail_rate <= 0.2) LOG_WARNING(log, - "Available space is only " << DB::toString(avail_rate * 100.0, 2) - << "% of capacity size. Avail size: " << formatReadableSizeWithBinarySuffix(total_stat.avail_size) - << ", used size: " << formatReadableSizeWithBinarySuffix(total_stat.used_size) - << ", capacity size: " << formatReadableSizeWithBinarySuffix(total_stat.capacity_size)); + "Available space is only " << DB::toString(avail_rate * 100.0, 2) + << "% of capacity size. Avail size: " << formatReadableSizeWithBinarySuffix(total_stat.avail_size) + << ", used size: " << formatReadableSizeWithBinarySuffix(total_stat.used_size) + << ", capacity size: " << formatReadableSizeWithBinarySuffix(total_stat.capacity_size)); total_stat.ok = 1; CurrentMetrics::set(CurrentMetrics::StoreSizeCapacity, total_stat.capacity_size); @@ -195,7 +192,7 @@ ssize_t PathCapacityMetrics::locatePath(std::string_view file_path) const FsStats PathCapacityMetrics::CapacityInfo::getStats(Poco::Logger * log) const { - FsStats res; + FsStats res{}; /// Get capacity, used, available size for one path. /// Similar to `handle_store_heartbeat` in TiKV release-4.0 branch /// https://github.com/tikv/tikv/blob/f14e8288f3/components/raftstore/src/store/worker/pd.rs#L593 @@ -224,8 +221,8 @@ FsStats PathCapacityMetrics::CapacityInfo::getStats(Poco::Logger * log) const avail = capacity - res.used_size; else if (log) LOG_WARNING(log, - "No available space for path: " << path << ", capacity: " << formatReadableSizeWithBinarySuffix(capacity) // - << ", used: " << formatReadableSizeWithBinarySuffix(used_bytes)); + "No available space for path: " << path << ", capacity: " << formatReadableSizeWithBinarySuffix(capacity) // + << ", used: " << formatReadableSizeWithBinarySuffix(used_bytes)); const uint64_t disk_free_bytes = vfs.f_bavail * vfs.f_frsize; if (avail > disk_free_bytes)