From cec6e09e25302985f1ae469b93198034593f642b Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 24 Feb 2022 12:23:43 +0800 Subject: [PATCH 1/3] 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 | 66 +++- .../src/Server/tests/gtest_storage_config.cpp | 362 +++++++++++++++++- dbms/src/Storages/PathCapacityMetrics.cpp | 25 +- 4 files changed, 578 insertions(+), 16 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 3558b25c999..3d30d0a6c27 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -40,8 +40,33 @@ 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 = 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) { @@ -51,15 +76,25 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) 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()) { +<<<<<<< HEAD String error_msg = "The array size of \"storage.main.dir\"[size=" + toString(main_data_paths.size()) + "] is not equal to \"storage.main.capacity\"[size=" + toString(main_capacity_quota.size()) + "]. Please check your configuration file."; LOG_ERROR(log, error_msg); +======= + String error_msg = fmt::format( + "The array size of \"storage.main.dir\"[size={}] " + "is not equal to \"storage.main.capacity\"[size={}]. " + "Please check your configuration file.", + main_data_paths.size(), + main_capacity_quota.size()); + LOG_FMT_ERROR(log, "{}", error_msg); +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < main_data_paths.size(); ++i) @@ -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); } + // 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,10 +123,20 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) } if (!latest_capacity_quota.empty() && latest_capacity_quota.size() != latest_data_paths.size()) { +<<<<<<< HEAD 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()) + "]. Please check your configuration file."; LOG_ERROR(log, error_msg); +======= + String error_msg = fmt::format( + "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_FMT_ERROR(log, "{}", error_msg); +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < latest_data_paths.size(); ++i) @@ -103,7 +149,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()) { @@ -238,9 +284,17 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc kvstore_paths.emplace_back(getNormalizedPath(deprecated_kvstore_path)); for (size_t i = 0; i < kvstore_paths.size(); ++i) { +<<<<<<< HEAD LOG_WARNING(log, "Raft data candidate path: " << kvstore_paths[i] << ". The path is overwritten by deprecated configuration for backward compatibility."); +======= + LOG_FMT_WARNING( + log, + "Raft data candidate path: {}. " + "The path is overwritten by deprecated configuration for backward compatibility.", + kvstore_path); +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) } } } @@ -274,8 +328,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 7bbdaa1c186..12b36617a5d 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -36,7 +36,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 = { @@ -66,14 +179,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/"); @@ -114,13 +283,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/"); @@ -170,12 +342,67 @@ path = "/data0/tiflash,/data1/tiflash" [storage.main] dir = [ "/data0/tiflash", "/data1/tiflash" ] capacity = [ 10737418240 ] +<<<<<<< HEAD # [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] + )", +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) }; for (size_t i = 0; i < tests.size(); ++i) @@ -240,12 +467,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(); @@ -259,6 +486,7 @@ capacity=[ 1024 ] ASSERT_NE(idx, PathCapacityMetrics::INVALID_INDEX); switch (i) { +<<<<<<< HEAD case 0: case 1: EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 0UL); @@ -266,13 +494,22 @@ capacity=[ 1024 ] case 2: EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 2048UL); break; +======= + case 0: + case 1: + EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 0); + break; + case 2: + EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 2048); + break; +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) } 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 @@ -340,6 +577,7 @@ dt_enable_rough_set_filter = false )", }; +<<<<<<< HEAD // Ensure that connection is not blocked by any address const std::vector test_addrs = { "127.0.0.1:443", @@ -347,12 +585,114 @@ dt_enable_rough_set_filter = false }; for (size_t i = 0; i < tests.size(); ++i) +======= + Poco::Logger * log = &Poco::Logger::get("StorageIORateLimitConfigTest"); + + 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); + ASSERT_TRUE(io_config.use_max_bytes_per_sec); + ASSERT_EQ(io_config.fg_write_weight, 25); + ASSERT_EQ(io_config.bg_write_weight, 25); + ASSERT_EQ(io_config.fg_read_weight, 25); + ASSERT_EQ(io_config.bg_read_weight, 25); + ASSERT_EQ(io_config.readWeight(), 50); + ASSERT_EQ(io_config.writeWeight(), 50); + ASSERT_EQ(io_config.totalWeight(), 100); + ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 0); + ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 0); + ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 0); + ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 0); + }; + + 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); + ASSERT_TRUE(io_config.use_max_bytes_per_sec); + ASSERT_EQ(io_config.fg_write_weight, 1); + ASSERT_EQ(io_config.bg_write_weight, 2); + ASSERT_EQ(io_config.fg_read_weight, 5); + ASSERT_EQ(io_config.bg_read_weight, 2); + ASSERT_EQ(io_config.readWeight(), 7); + ASSERT_EQ(io_config.writeWeight(), 3); + ASSERT_EQ(io_config.totalWeight(), 10); + ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 0); + ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 0); + ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 0); + ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 0); + }; + + 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); + ASSERT_TRUE(io_config.use_max_bytes_per_sec); + ASSERT_EQ(io_config.fg_write_weight, 1); + ASSERT_EQ(io_config.bg_write_weight, 2); + ASSERT_EQ(io_config.fg_read_weight, 5); + ASSERT_EQ(io_config.bg_read_weight, 2); + ASSERT_EQ(io_config.readWeight(), 7); + ASSERT_EQ(io_config.writeWeight(), 3); + ASSERT_EQ(io_config.totalWeight(), 10); + ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 102400); + ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 102400 * 2); + ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 102400 * 5); + ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 102400 * 2); + }; + + 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); + ASSERT_FALSE(io_config.use_max_bytes_per_sec); + ASSERT_EQ(io_config.fg_write_weight, 1); + ASSERT_EQ(io_config.bg_write_weight, 2); + ASSERT_EQ(io_config.fg_read_weight, 5); + ASSERT_EQ(io_config.bg_read_weight, 2); + ASSERT_EQ(io_config.readWeight(), 7); + ASSERT_EQ(io_config.writeWeight(), 3); + ASSERT_EQ(io_config.totalWeight(), 10); + ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 731428); + ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 341333); + ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 292571); + ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 682666); + }; + + 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); + ASSERT_TRUE(io_config.use_max_bytes_per_sec); + ASSERT_EQ(io_config.fg_write_weight, 1); + ASSERT_EQ(io_config.bg_write_weight, 2); + ASSERT_EQ(io_config.fg_read_weight, 5); + ASSERT_EQ(io_config.bg_read_weight, 2); + ASSERT_EQ(io_config.readWeight(), 7); + ASSERT_EQ(io_config.writeWeight(), 3); + ASSERT_EQ(io_config.totalWeight(), 10); + ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 102400); + ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 102400 * 2); + ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 102400 * 5); + ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 102400 * 2); + }; + + std::vector> case_verifiers; + 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) +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) { const auto & test_case = tests[i]; auto config = loadConfigFromString(test_case); LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); +<<<<<<< HEAD // Reload users config with test case auto & global_ctx = TiFlashTestEnv::getGlobalContext(); global_ctx.setUsersConfig(config); @@ -381,6 +721,12 @@ dt_enable_rough_set_filter = false ASSERT_NO_THROW(ctx.checkDatabaseAccessRights("system")); ASSERT_NO_THROW(ctx.checkDatabaseAccessRights("test")); } +======= + StorageIORateLimitConfig io_config; + verify_default(io_config); + io_config.parse(config->getString("storage.io_rate_limit"), log); + case_verifiers[i](io_config); +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) } } CATCH diff --git a/dbms/src/Storages/PathCapacityMetrics.cpp b/dbms/src/Storages/PathCapacityMetrics.cpp index 8ca8b61beb7..4e7608f9b39 100644 --- a/dbms/src/Storages/PathCapacityMetrics.cpp +++ b/dbms/src/Storages/PathCapacityMetrics.cpp @@ -80,7 +80,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 +93,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,8 +106,18 @@ 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`. +<<<<<<< HEAD FsStats total_stat; for (size_t i = 0; i < path_infos.size(); ++i) +======= + FsStats total_stat{}; + + // Build the disk stats map + // which use to measure single disk capacity and available size + auto disk_stats_map = getDiskStats(); + + for (auto & fs_it : disk_stats_map) +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) { FsStats path_stat = path_infos[i].getStats(log); if (!path_stat.ok) @@ -116,6 +126,17 @@ FsStats PathCapacityMetrics::getFsStats() const return total_stat; } +<<<<<<< HEAD +======= + const uint64_t disk_capacity_size = vfs_info.f_blocks * vfs_info.f_frsize; + if (disk_stat.capacity_size == 0 || disk_capacity_size < disk_stat.capacity_size) + disk_stat.capacity_size = disk_capacity_size; + + // 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); + +>>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) // sum of all path's capacity and used_size total_stat.capacity_size += path_stat.capacity_size; total_stat.used_size += path_stat.used_size; From 41c36616c05f764c2a63047ad7f5cf82c1e9f398 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 20 Jun 2022 12:56:27 +0800 Subject: [PATCH 2/3] try fix conflict Signed-off-by: JaySon-Huang --- dbms/src/Common/tests/gtest_cpptoml.cpp | 15 +- dbms/src/Server/StorageConfigParser.cpp | 81 ++++---- .../src/Server/tests/gtest_storage_config.cpp | 178 +++--------------- dbms/src/Storages/PathCapacityMetrics.cpp | 54 ++---- 4 files changed, 86 insertions(+), 242 deletions(-) diff --git a/dbms/src/Common/tests/gtest_cpptoml.cpp b/dbms/src/Common/tests/gtest_cpptoml.cpp index f92876a623b..9cd216b8167 100644 --- a/dbms/src/Common/tests/gtest_cpptoml.cpp +++ b/dbms/src/Common/tests/gtest_cpptoml.cpp @@ -1,12 +1,11 @@ +#include #include #include -#include #include namespace DB::tests { - TEST(CPPTomlTest, ContainsQualifiedArray) { auto * log = &Poco::Logger::get("CPPTomlTest"); @@ -32,8 +31,8 @@ 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); + 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); @@ -71,8 +70,8 @@ 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); + 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); @@ -114,8 +113,8 @@ 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); + 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); diff --git a/dbms/src/Server/StorageConfigParser.cpp b/dbms/src/Server/StorageConfigParser.cpp index 3d30d0a6c27..cf6e6c749ac 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 @@ -42,8 +51,9 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) 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); + 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 @@ -71,30 +81,20 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) 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()) { String error_msg = "The configuration \"storage.main.dir\" is empty. Please check your configuration file."; - LOG_FMT_ERROR(log, "{}", error_msg); + LOG_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()) { -<<<<<<< HEAD String error_msg = "The array size of \"storage.main.dir\"[size=" + toString(main_data_paths.size()) + "] is not equal to \"storage.main.capacity\"[size=" + toString(main_capacity_quota.size()) + "]. Please check your configuration file."; LOG_ERROR(log, error_msg); -======= - String error_msg = fmt::format( - "The array size of \"storage.main.dir\"[size={}] " - "is not equal to \"storage.main.capacity\"[size={}]. " - "Please check your configuration file.", - main_data_paths.size(), - main_capacity_quota.size()); - LOG_FMT_ERROR(log, "{}", error_msg); ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < main_data_paths.size(); ++i) @@ -112,7 +112,7 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) 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()) @@ -123,20 +123,10 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) } if (!latest_capacity_quota.empty() && latest_capacity_quota.size() != latest_data_paths.size()) { -<<<<<<< HEAD - 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); -======= - String error_msg = fmt::format( - "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_FMT_ERROR(log, "{}", error_msg); ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < latest_data_paths.size(); ++i) @@ -160,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 @@ -173,6 +163,9 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) if (auto version = table->get_qualified_as("format_version"); version) format_version = *version; + + if (auto lazily_init = table->get_qualified_as("lazily_init_store"); lazily_init) + lazily_init_store = (*lazily_init != 0); } Strings TiFlashStorageConfig::getAllNormalPaths() const @@ -205,9 +198,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. @@ -264,15 +257,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; @@ -282,19 +276,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) { -<<<<<<< HEAD LOG_WARNING(log, "Raft data candidate path: " - << kvstore_paths[i] << ". The path is overwritten by deprecated configuration for backward compatibility."); -======= - LOG_FMT_WARNING( - log, - "Raft data candidate path: {}. " - "The path is overwritten by deprecated configuration for backward compatibility.", - kvstore_path); ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) + << kvstore_path << ". The path is overwritten by deprecated configuration for backward compatibility."); } } } @@ -311,12 +297,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++; } @@ -329,7 +314,7 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc { // Can not parse from the deprecated configuration "path". String msg = "The configuration \"storage.main\" section is not defined. Please check your configuration file."; - LOG_FMT_ERROR(log, "{}", msg); + 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 12b36617a5d..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,9 +45,6 @@ class StorageConfig_test : public ::testing::Test Poco::Logger * log; }; -<<<<<<< HEAD -TEST_F(StorageConfig_test, MultiSSDSettings) -======= TEST_F(StorageConfigTest, SimpleSinglePath) try { @@ -65,7 +71,7 @@ dir=["/data0/tiflash"] const auto & test_case = tests[i]; auto config = loadConfigFromString(test_case); - LOG_FMT_INFO(log, "parsing [index={}] [content={}]", i, test_case); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); size_t global_capacity_quota = 0; TiFlashStorageConfig storage; @@ -84,7 +90,8 @@ dir=["/data0/tiflash"] 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); + PathCapacityMetrics path_capacity(global_capacity_quota, storage.main_data_paths, storage.main_capacity_quota, + storage.latest_data_paths, storage.latest_capacity_quota); } } CATCH @@ -124,7 +131,7 @@ dir=["/data222/kvstore"] const auto & test_case = tests[i]; auto config = loadConfigFromString(test_case); - LOG_FMT_INFO(log, "parsing [index={}] [content={}]", i, test_case); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); size_t global_capacity_quota = 0; TiFlashStorageConfig storage; @@ -143,13 +150,13 @@ dir=["/data222/kvstore"] 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); + 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 = { @@ -194,7 +201,8 @@ dir=["/data0/tiflash"] 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); + PathCapacityMetrics path_capacity(global_capacity_quota, storage.main_data_paths, storage.main_capacity_quota, + storage.latest_data_paths, storage.latest_capacity_quota); } } CATCH @@ -222,7 +230,7 @@ dir=["/data0/tiflash", "/data1/tiflash", "/data2/tiflash"] const auto & test_case = tests[i]; auto config = loadConfigFromString(test_case); - LOG_FMT_INFO(log, "parsing [index={}] [content={}]", i, test_case); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); size_t global_capacity_quota = 0; TiFlashStorageConfig storage; @@ -253,7 +261,7 @@ dir=["/data0/tiflash", "/data1/tiflash", "/data2/tiflash"] } CATCH -TEST_F(StorageConfig_test, SSD_HDD_Settings) +TEST_F(StorageConfigTest, SSD_HDD_Settings) try { Strings tests = { @@ -303,7 +311,7 @@ dir=["/ssd0/tiflash"] } CATCH -TEST_F(StorageConfig_test, ParseMaybeBrokenCases) +TEST_F(StorageConfigTest, ParseMaybeBrokenCases) try { Strings tests = { @@ -342,15 +350,12 @@ path = "/data0/tiflash,/data1/tiflash" [storage.main] dir = [ "/data0/tiflash", "/data1/tiflash" ] capacity = [ 10737418240 ] -<<<<<<< HEAD # [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" @@ -402,7 +407,6 @@ dir = [["/data0/tiflash", "/data1/tiflash"], ["/data2/tiflash", ]] [storage.main] dir = [1,2,3] )", ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) }; for (size_t i = 0; i < tests.size(); ++i) @@ -419,7 +423,7 @@ dir = [1,2,3] } CATCH -TEST(PathCapacityMetrics_test, Quota) +TEST(PathCapacityMetricsTest, Quota) try { Strings tests = { @@ -454,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) { @@ -486,23 +490,13 @@ capacity=[ 1024 ] ASSERT_NE(idx, PathCapacityMetrics::INVALID_INDEX); switch (i) { -<<<<<<< HEAD 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; -======= - case 0: - case 1: - EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 0); - break; - case 2: - EXPECT_EQ(path_capacity.path_infos[idx].capacity_bytes, 2048); - break; ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) } idx = path_capacity.locatePath("/data1/tiflash/"); ASSERT_NE(idx, PathCapacityMetrics::INVALID_INDEX); @@ -514,10 +508,10 @@ capacity=[ 1024 ] } 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() {} @@ -526,7 +520,7 @@ class UsersConfigParser_test : public ::testing::Test }; -TEST_F(UsersConfigParser_test, ParseConfigs) +TEST_F(UsersConfigParserTest, ParseConfigs) try { Strings tests = { @@ -550,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 @@ -577,7 +571,6 @@ dt_enable_rough_set_filter = false )", }; -<<<<<<< HEAD // Ensure that connection is not blocked by any address const std::vector test_addrs = { "127.0.0.1:443", @@ -585,114 +578,12 @@ dt_enable_rough_set_filter = false }; for (size_t i = 0; i < tests.size(); ++i) -======= - Poco::Logger * log = &Poco::Logger::get("StorageIORateLimitConfigTest"); - - 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); - ASSERT_TRUE(io_config.use_max_bytes_per_sec); - ASSERT_EQ(io_config.fg_write_weight, 25); - ASSERT_EQ(io_config.bg_write_weight, 25); - ASSERT_EQ(io_config.fg_read_weight, 25); - ASSERT_EQ(io_config.bg_read_weight, 25); - ASSERT_EQ(io_config.readWeight(), 50); - ASSERT_EQ(io_config.writeWeight(), 50); - ASSERT_EQ(io_config.totalWeight(), 100); - ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 0); - ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 0); - ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 0); - ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 0); - }; - - 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); - ASSERT_TRUE(io_config.use_max_bytes_per_sec); - ASSERT_EQ(io_config.fg_write_weight, 1); - ASSERT_EQ(io_config.bg_write_weight, 2); - ASSERT_EQ(io_config.fg_read_weight, 5); - ASSERT_EQ(io_config.bg_read_weight, 2); - ASSERT_EQ(io_config.readWeight(), 7); - ASSERT_EQ(io_config.writeWeight(), 3); - ASSERT_EQ(io_config.totalWeight(), 10); - ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 0); - ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 0); - ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 0); - ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 0); - }; - - 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); - ASSERT_TRUE(io_config.use_max_bytes_per_sec); - ASSERT_EQ(io_config.fg_write_weight, 1); - ASSERT_EQ(io_config.bg_write_weight, 2); - ASSERT_EQ(io_config.fg_read_weight, 5); - ASSERT_EQ(io_config.bg_read_weight, 2); - ASSERT_EQ(io_config.readWeight(), 7); - ASSERT_EQ(io_config.writeWeight(), 3); - ASSERT_EQ(io_config.totalWeight(), 10); - ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 102400); - ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 102400 * 2); - ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 102400 * 5); - ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 102400 * 2); - }; - - 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); - ASSERT_FALSE(io_config.use_max_bytes_per_sec); - ASSERT_EQ(io_config.fg_write_weight, 1); - ASSERT_EQ(io_config.bg_write_weight, 2); - ASSERT_EQ(io_config.fg_read_weight, 5); - ASSERT_EQ(io_config.bg_read_weight, 2); - ASSERT_EQ(io_config.readWeight(), 7); - ASSERT_EQ(io_config.writeWeight(), 3); - ASSERT_EQ(io_config.totalWeight(), 10); - ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 731428); - ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 341333); - ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 292571); - ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 682666); - }; - - 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); - ASSERT_TRUE(io_config.use_max_bytes_per_sec); - ASSERT_EQ(io_config.fg_write_weight, 1); - ASSERT_EQ(io_config.bg_write_weight, 2); - ASSERT_EQ(io_config.fg_read_weight, 5); - ASSERT_EQ(io_config.bg_read_weight, 2); - ASSERT_EQ(io_config.readWeight(), 7); - ASSERT_EQ(io_config.writeWeight(), 3); - ASSERT_EQ(io_config.totalWeight(), 10); - ASSERT_EQ(io_config.getFgReadMaxBytesPerSec(), 102400); - ASSERT_EQ(io_config.getFgWriteMaxBytesPerSec(), 102400 * 2); - ASSERT_EQ(io_config.getBgReadMaxBytesPerSec(), 102400 * 5); - ASSERT_EQ(io_config.getBgWriteMaxBytesPerSec(), 102400 * 2); - }; - - std::vector> case_verifiers; - 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) ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) { const auto & test_case = tests[i]; auto config = loadConfigFromString(test_case); LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); -<<<<<<< HEAD // Reload users config with test case auto & global_ctx = TiFlashTestEnv::getGlobalContext(); global_ctx.setUsersConfig(config); @@ -721,16 +612,9 @@ dt_enable_rough_set_filter = false ASSERT_NO_THROW(ctx.checkDatabaseAccessRights("system")); ASSERT_NO_THROW(ctx.checkDatabaseAccessRights("test")); } -======= - StorageIORateLimitConfig io_config; - verify_default(io_config); - io_config.parse(config->getString("storage.io_rate_limit"), log); - case_verifiers[i](io_config); ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) } } CATCH - } // namespace tests } // namespace DB diff --git a/dbms/src/Storages/PathCapacityMetrics.cpp b/dbms/src/Storages/PathCapacityMetrics.cpp index 4e7608f9b39..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()) { @@ -106,18 +103,8 @@ 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`. -<<<<<<< HEAD - FsStats total_stat; - for (size_t i = 0; i < path_infos.size(); ++i) -======= FsStats total_stat{}; - - // Build the disk stats map - // which use to measure single disk capacity and available size - auto disk_stats_map = getDiskStats(); - - for (auto & fs_it : disk_stats_map) ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) + for (size_t i = 0; i < path_infos.size(); ++i) { FsStats path_stat = path_infos[i].getStats(log); if (!path_stat.ok) @@ -126,17 +113,6 @@ FsStats PathCapacityMetrics::getFsStats() const return total_stat; } -<<<<<<< HEAD -======= - const uint64_t disk_capacity_size = vfs_info.f_blocks * vfs_info.f_frsize; - if (disk_stat.capacity_size == 0 || disk_capacity_size < disk_stat.capacity_size) - disk_stat.capacity_size = disk_capacity_size; - - // 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); - ->>>>>>> e50c06c46d (Fix invalid storage dir configurations lead to unexpected behavior (#4105)) // sum of all path's capacity and used_size total_stat.capacity_size += path_stat.capacity_size; total_stat.used_size += path_stat.used_size; @@ -156,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); @@ -216,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 @@ -245,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) From 0f9282d583d553ef282dcbfd87cb3b0d92d7ef69 Mon Sep 17 00:00:00 2001 From: JaySon Date: Mon, 20 Jun 2022 14:42:00 +0800 Subject: [PATCH 3/3] Apply suggestions from code review --- dbms/src/Server/StorageConfigParser.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/dbms/src/Server/StorageConfigParser.cpp b/dbms/src/Server/StorageConfigParser.cpp index cf6e6c749ac..2c6ed64a14d 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -163,9 +163,6 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) if (auto version = table->get_qualified_as("format_version"); version) format_version = *version; - - if (auto lazily_init = table->get_qualified_as("lazily_init_store"); lazily_init) - lazily_init_store = (*lazily_init != 0); } Strings TiFlashStorageConfig::getAllNormalPaths() const