From f790e618979591219be3a154786996bf5ce5b5bb Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 24 Feb 2022 12:23:43 +0800 Subject: [PATCH 1/4] This is an automated cherry-pick of #4105 Signed-off-by: ti-chi-bot --- dbms/src/Common/tests/gtest_cpptoml.cpp | 141 +++++++++ dbms/src/Server/StorageConfigParser.cpp | 50 +++- .../src/Server/tests/gtest_storage_config.cpp | 270 ++++++++++++++++-- dbms/src/Storages/PathCapacityMetrics.cpp | 8 +- 4 files changed, 427 insertions(+), 42 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..f92876a623b --- /dev/null +++ b/dbms/src/Common/tests/gtest_cpptoml.cpp @@ -0,0 +1,141 @@ +#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(fmt::format("[index={}] [content={}]", i, test_case)); + LOG_FMT_INFO(log, "parsing [index={}] [content={}]", i, 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(fmt::format("[index={}] [content={}]", i, test_case)); + LOG_FMT_INFO(log, "parsing [index={}] [content={}]", i, 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(fmt::format("[index={}] [content={}]", i, test_case)); + LOG_FMT_INFO(log, "parsing [index={}] [content={}]", i, 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 221ff710af7..eee00c3e40d 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -53,8 +53,33 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger 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 = fmt::format("The configuration \"storage.{}\" should be an array of strings. Please check your configuration file.", key); + LOG_FMT_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) { @@ -64,7 +89,7 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger if (main_data_paths.empty()) { String error_msg = "The configuration \"storage.main.dir\" is empty. Please check your configuration file."; - LOG_ERROR(log, error_msg); + LOG_FMT_ERROR(log, "{}", error_msg); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } if (!main_capacity_quota.empty() && main_capacity_quota.size() != main_data_paths.size()) @@ -75,7 +100,7 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger "Please check your configuration file.", main_data_paths.size(), main_capacity_quota.size()); - LOG_ERROR(log, error_msg); + LOG_FMT_ERROR(log, "{}", error_msg); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < main_data_paths.size(); ++i) @@ -88,13 +113,14 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger } // 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(static_cast(c)); } + // If it is empty, use the same dir as "main.dir" if (latest_data_paths.empty()) { LOG_FMT_INFO(log, "The configuration \"storage.latest.dir\" is empty, use the same dir and capacity of \"storage.main.dir\""); @@ -104,12 +130,12 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger if (!latest_capacity_quota.empty() && latest_capacity_quota.size() != latest_data_paths.size()) { String error_msg = fmt::format( - "The array size of \"storage.main.dir\"[size={}] " - "is not equal to \"storage.main.capacity\"[size={}]. " + "The array size of \"storage.latest.dir\"[size={}] " + "is not equal to \"storage.latest.capacity\"[size={}]. " "Please check your configuration file.", latest_data_paths.size(), latest_capacity_quota.size()); - LOG_ERROR(log, error_msg); + LOG_FMT_ERROR(log, "{}", error_msg); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < latest_data_paths.size(); ++i) @@ -122,7 +148,7 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger } // 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()) { @@ -283,8 +309,8 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc { LOG_FMT_WARNING( log, - "Raft data candidate path: {}" - ". The path is overwritten by deprecated configuration for backward compatibility.", + "Raft data candidate path: {}. " + "The path is overwritten by deprecated configuration for backward compatibility.", kvstore_path); } } @@ -318,8 +344,8 @@ 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."; - LOG_ERROR(log, msg); + String msg = "The configuration \"storage.main\" section is not defined. Please check your configuration file."; + LOG_FMT_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 80301fa8f3a..9dd6260d2bc 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -32,7 +32,120 @@ class StorageConfig_test : public ::testing::Test Poco::Logger * log; }; +<<<<<<< HEAD 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_FMT_INFO(log, "parsing [index={}] [content={}]", i, 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_FMT_INFO(log, "parsing [index={}] [content={}]", i, 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) +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) try { Strings tests = { @@ -62,14 +175,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(), 1UL); + 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_FMT_INFO(log, "parsing [index={}] [content={}]", i, 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(), 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/"); @@ -109,13 +278,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/"); @@ -164,11 +336,57 @@ path = "/data0/tiflash,/data1/tiflash" [storage.main] dir = [ "/data0/tiflash", "/data1/tiflash" ] capacity = [ 10737418240 ] -# [storage.latest] -# dir = [ ] -# capacity = [ 10737418240, 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] )", }; @@ -234,12 +452,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(); @@ -254,18 +472,18 @@ 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 @@ -377,7 +595,7 @@ background_read_weight=2 Poco::Logger * log = &Poco::Logger::get("StorageIORateLimitConfig_test"); - auto verifyDefault = [](const StorageIORateLimitConfig & io_config) { + auto verify_default = [](const StorageIORateLimitConfig & io_config) { ASSERT_EQ(io_config.max_bytes_per_sec, 0); ASSERT_EQ(io_config.max_read_bytes_per_sec, 0); ASSERT_EQ(io_config.max_write_bytes_per_sec, 0); @@ -395,7 +613,7 @@ background_read_weight=2 ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 0); }; - auto verifyCase0 = [](const StorageIORateLimitConfig & io_config) { + auto verify_case0 = [](const StorageIORateLimitConfig & io_config) { ASSERT_EQ(io_config.max_bytes_per_sec, 0); ASSERT_EQ(io_config.max_read_bytes_per_sec, 0); ASSERT_EQ(io_config.max_write_bytes_per_sec, 0); @@ -413,7 +631,7 @@ background_read_weight=2 ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 0); }; - auto verifyCase1 = [](const StorageIORateLimitConfig & io_config) { + auto verify_case1 = [](const StorageIORateLimitConfig & io_config) { ASSERT_EQ(io_config.max_bytes_per_sec, 1024000); ASSERT_EQ(io_config.max_read_bytes_per_sec, 0); ASSERT_EQ(io_config.max_write_bytes_per_sec, 0); @@ -431,7 +649,7 @@ background_read_weight=2 ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 102400 * 2); }; - auto verifyCase2 = [](const StorageIORateLimitConfig & io_config) { + auto verify_case2 = [](const StorageIORateLimitConfig & io_config) { ASSERT_EQ(io_config.max_bytes_per_sec, 0); ASSERT_EQ(io_config.max_read_bytes_per_sec, 1024000); ASSERT_EQ(io_config.max_write_bytes_per_sec, 1024000); @@ -449,7 +667,7 @@ background_read_weight=2 ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 682666); }; - auto verifyCase3 = [](const StorageIORateLimitConfig & io_config) { + auto verify_case3 = [](const StorageIORateLimitConfig & io_config) { ASSERT_EQ(io_config.max_bytes_per_sec, 1024000); ASSERT_EQ(io_config.max_read_bytes_per_sec, 1024000); ASSERT_EQ(io_config.max_write_bytes_per_sec, 1024000); @@ -468,10 +686,10 @@ background_read_weight=2 }; std::vector> case_verifiers; - case_verifiers.push_back(verifyCase0); - case_verifiers.push_back(verifyCase1); - case_verifiers.push_back(verifyCase2); - case_verifiers.push_back(verifyCase3); + case_verifiers.push_back(verify_case0); + case_verifiers.push_back(verify_case1); + case_verifiers.push_back(verify_case2); + case_verifiers.push_back(verify_case3); for (size_t i = 0; i < 2u /*tests.size()*/; ++i) { @@ -482,11 +700,11 @@ background_read_weight=2 ASSERT_TRUE(config->has("storage.io_rate_limit")); StorageIORateLimitConfig io_config; - verifyDefault(io_config); + verify_default(io_config); io_config.parse(config->getString("storage.io_rate_limit"), log); case_verifiers[i](io_config); } } CATCH } // namespace tests -} // namespace DB \ No newline at end of file +} // namespace DB diff --git a/dbms/src/Storages/PathCapacityMetrics.cpp b/dbms/src/Storages/PathCapacityMetrics.cpp index cd43bf9d9b7..75e83c9e638 100644 --- a/dbms/src/Storages/PathCapacityMetrics.cpp +++ b/dbms/src/Storages/PathCapacityMetrics.cpp @@ -81,7 +81,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; } @@ -94,7 +94,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; } @@ -132,7 +132,7 @@ FsStats PathCapacityMetrics::getFsStats() FsStats total_stat{}; // Build the disk stats map - // which use to measure single disk capacoty and available size + // which use to measure single disk capacity and available size auto disk_stats_map = getDiskStats(); for (auto fs_it = disk_stats_map.begin(); fs_it != disk_stats_map.end(); ++fs_it) @@ -153,7 +153,7 @@ FsStats PathCapacityMetrics::getFsStats() if (disk_stat.capacity_size == 0 || disk_capacity_size < disk_stat.capacity_size) disk_stat.capacity_size = disk_capacity_size; - // Calutate single disk info + // Calculate single disk info const uint64_t disk_free_bytes = vfs_info.f_bavail * vfs_info.f_frsize; disk_stat.avail_size = std::min(disk_free_bytes, disk_stat.avail_size); From 75ee81bb6cb63839610d4d0a9f40981daf14108e Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 26 Apr 2022 19:41:06 +0800 Subject: [PATCH 2/4] Format files Signed-off-by: JaySon-Huang --- dbms/src/Common/tests/gtest_cpptoml.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/dbms/src/Common/tests/gtest_cpptoml.cpp b/dbms/src/Common/tests/gtest_cpptoml.cpp index f92876a623b..337b6db5238 100644 --- a/dbms/src/Common/tests/gtest_cpptoml.cpp +++ b/dbms/src/Common/tests/gtest_cpptoml.cpp @@ -6,7 +6,6 @@ namespace DB::tests { - TEST(CPPTomlTest, ContainsQualifiedArray) { auto * log = &Poco::Logger::get("CPPTomlTest"); From 9711b5a892435b8e662035d4753d40896890aae6 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 26 Apr 2022 21:18:14 +0800 Subject: [PATCH 3/4] Fix lint Signed-off-by: JaySon-Huang --- dbms/src/Storages/PathCapacityMetrics.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dbms/src/Storages/PathCapacityMetrics.cpp b/dbms/src/Storages/PathCapacityMetrics.cpp index 75e83c9e638..5f95ab4f4ef 100644 --- a/dbms/src/Storages/PathCapacityMetrics.cpp +++ b/dbms/src/Storages/PathCapacityMetrics.cpp @@ -101,12 +101,12 @@ void PathCapacityMetrics::freeUsedSize(std::string_view file_path, size_t used_b std::map PathCapacityMetrics::getDiskStats() { std::map disk_stats_map; - for (size_t i = 0; i < path_infos.size(); ++i) + for (auto & path_info : path_infos) { struct statvfs vfs; FsStats path_stat; - std::tie(path_stat, vfs) = path_infos[i].getStats(log); + std::tie(path_stat, vfs) = path_info.getStats(log); if (!path_stat.ok) { // Disk may be hot remove, Ignore this disk. @@ -135,11 +135,11 @@ FsStats PathCapacityMetrics::getFsStats() // which use to measure single disk capacity and available size auto disk_stats_map = getDiskStats(); - for (auto fs_it = disk_stats_map.begin(); fs_it != disk_stats_map.end(); ++fs_it) + for (auto & fs_it : disk_stats_map) { FsStats disk_stat{}; - auto & disk_stat_vec = fs_it->second; + auto & disk_stat_vec = fs_it.second; auto & vfs_info = disk_stat_vec.vfs_info; for (const auto & single_path_stats : disk_stat_vec.path_stats) From 608eb9132f41856f8592af3e0391f6e71ff8ab98 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 26 Apr 2022 22:53:07 +0800 Subject: [PATCH 4/4] Fix lint Signed-off-by: JaySon-Huang --- .../src/Server/tests/gtest_storage_config.cpp | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/dbms/src/Server/tests/gtest_storage_config.cpp b/dbms/src/Server/tests/gtest_storage_config.cpp index 9dd6260d2bc..7620c8c97b0 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -19,11 +19,11 @@ namespace DB { namespace tests { -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() {} @@ -32,9 +32,6 @@ class StorageConfig_test : public ::testing::Test Poco::Logger * log; }; -<<<<<<< HEAD -TEST_F(StorageConfig_test, MultiSSDSettings) -======= TEST_F(StorageConfigTest, SimpleSinglePath) try { @@ -145,7 +142,6 @@ dir=["/data222/kvstore"] CATCH TEST_F(StorageConfigTest, MultiSSDSettings) ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) try { Strings tests = { @@ -248,7 +244,7 @@ dir=["/data0/tiflash", "/data1/tiflash", "/data2/tiflash"] } CATCH -TEST_F(StorageConfig_test, SSD_HDD_Settings) +TEST_F(StorageConfigTest, SSDHDDSettings) try { Strings tests = { @@ -297,7 +293,7 @@ dir=["/ssd0/tiflash"] } CATCH -TEST_F(StorageConfig_test, ParseMaybeBrokenCases) +TEST_F(StorageConfigTest, ParseMaybeBrokenCases) try { Strings tests = { @@ -404,7 +400,7 @@ dir = [1,2,3] } CATCH -TEST(PathCapacityMetrics_test, Quota) +TEST(PathCapacityMetricsTest, Quota) try { Strings tests = { @@ -488,7 +484,7 @@ capacity=[ 1024 ] } CATCH -TEST_F(StorageConfig_test, CompatibilityWithIORateLimitConfig) +TEST_F(StorageConfigTest, CompatibilityWithIORateLimitConfig) try { Strings tests = { @@ -543,7 +539,7 @@ max_bytes_per_sec=1024000 } CATCH -TEST(StorageIORateLimitConfig_test, StorageIORateLimitConfig) +TEST(StorageIORateLimitConfigTest, StorageIORateLimitConfig) try { Strings tests = {