From 646ebf0d42edfc39207637449dd43c75ec8fb61f Mon Sep 17 00:00:00 2001 From: JaySon Date: Thu, 24 Feb 2022 12:23:43 +0800 Subject: [PATCH 1/9] 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 | 374 +++++++++++++++++- dbms/src/Storages/PathCapacityMetrics.cpp | 24 +- 4 files changed, 589 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 2effefbd496..8f65e4d73aa 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -49,8 +49,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) { @@ -60,15 +85,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) @@ -81,13 +116,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\""); @@ -96,10 +132,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) @@ -112,7 +158,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()) { @@ -250,9 +296,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)) } } } @@ -286,8 +340,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 b39211d4df3..dc1d3cf4547 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -46,7 +46,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 = { @@ -76,13 +189,69 @@ 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_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(), 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/"); @@ -124,13 +293,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/"); @@ -180,12 +352,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) @@ -250,12 +477,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(); @@ -269,6 +496,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); @@ -276,13 +504,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 @@ -343,6 +580,7 @@ result_rows = 0 )", // case for set some settings R"( +<<<<<<< HEAD [profiles] [profiles.default] max_memory_usage = 123456 @@ -357,6 +595,119 @@ dt_enable_rough_set_filter = false }; for (size_t i = 0; i < tests.size(); ++i) +======= +[storage] +[storage.io_rate_limit] +max_bytes_per_sec=1024000 +max_read_bytes_per_sec=1024000 +max_write_bytes_per_sec=1024000 +foreground_write_weight=1 +background_write_weight=2 +foreground_read_weight=5 +background_read_weight=2 + )", + }; + + 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); @@ -367,6 +718,7 @@ dt_enable_rough_set_filter = false auto & global_ctx = TiFlashTestEnv::getGlobalContext(); global_ctx.setUsersConfig(config); +<<<<<<< HEAD // Create a copy of global_ctx auto ctx = global_ctx; for (const auto & addr_ : test_addrs) @@ -391,6 +743,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 ec9dc667f96..9394f50ea29 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; } @@ -107,7 +107,16 @@ FsStats PathCapacityMetrics::getFsStats() const // Now we assume the size of `path_infos` will not change, don't acquire heavy lock on `path_infos`. FsStats total_stat{}; +<<<<<<< HEAD for (size_t i = 0; i < path_infos.size(); ++i) +======= + + // 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 +125,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 f689013aa9cc6703759f9ff7bcb12fad7300bc48 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Wed, 15 Jun 2022 16:58:10 +0800 Subject: [PATCH 2/9] fix conflict Signed-off-by: JaySon-Huang --- dbms/src/Common/tests/gtest_cpptoml.cpp | 8 +- dbms/src/Server/StorageConfigParser.cpp | 105 ++++---- .../src/Server/tests/gtest_storage_config.cpp | 240 ++++++------------ dbms/src/Storages/PathCapacityMetrics.cpp | 49 +--- 4 files changed, 145 insertions(+), 257 deletions(-) diff --git a/dbms/src/Common/tests/gtest_cpptoml.cpp b/dbms/src/Common/tests/gtest_cpptoml.cpp index f92876a623b..a4d80837556 100644 --- a/dbms/src/Common/tests/gtest_cpptoml.cpp +++ b/dbms/src/Common/tests/gtest_cpptoml.cpp @@ -1,12 +1,12 @@ #include #include #include +#include #include namespace DB::tests { - TEST(CPPTomlTest, ContainsQualifiedArray) { auto * log = &Poco::Logger::get("CPPTomlTest"); @@ -33,7 +33,7 @@ c = 123.45 { 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); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); std::istringstream ss(test_case); cpptoml::parser p(ss); @@ -72,7 +72,7 @@ c = [123, 456] { 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); + LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); std::istringstream ss(test_case); cpptoml::parser p(ss); @@ -115,7 +115,7 @@ c = [] { 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); + 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 8f65e4d73aa..588660e0ee1 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -3,7 +3,7 @@ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" #endif -#include +#include #if !__clang__ #pragma GCC diagnostic pop #endif @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -43,7 +44,7 @@ static std::string getCanonicalPath(std::string path) static String getNormalizedPath(const String & s) { return getCanonicalPath(Poco::Path{s}.toString()); } -void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) +void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger * log) { std::istringstream ss(storage); cpptoml::parser p(ss); @@ -51,8 +52,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 + = fmt::format("The configuration \"storage.{}\" should be an array of strings. Please check your configuration file.", key); + LOG_ERROR(log, error_msg); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); }; // not exist key @@ -80,30 +82,22 @@ 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.", + 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)) + LOG_ERROR(log, error_msg); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < main_data_paths.size(); ++i) @@ -121,7 +115,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()) @@ -132,20 +126,12 @@ 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.", + 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)) + LOG_ERROR(log, error_msg); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } for (size_t i = 0; i < latest_data_paths.size(); ++i) @@ -169,22 +155,36 @@ 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); } +} + +void TiFlashStorageConfig::parseMisc(const String & storage_section, Poco::Logger * log) +{ + std::istringstream ss(storage_section); + cpptoml::parser p(ss); + auto table = p.parse(); - // rate limiter - if (auto rate_limit = table->get_qualified_as("bg_task_io_rate_limit"); rate_limit) - bg_task_io_rate_limit = *rate_limit; + if (table->contains("bg_task_io_rate_limit")) + { + LOG_WARNING(log, "The configuration \"bg_task_io_rate_limit\" is deprecated. Check [storage.io_rate_limit] section for new style."); + } 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); + } + + LOG_INFO(log, fmt::format("format_version {} lazily_init_store {}", format_version, lazily_init_store)); } Strings TiFlashStorageConfig::getAllNormalPaths() const @@ -217,9 +217,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. @@ -276,15 +276,21 @@ 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); + storage_config.parseMisc(config.getString("storage"), log); + } + if (config.has("storage.main")) + { 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.parseStoragePath(config.getString("storage"), log); + if (config.has("raft.kvstore_path")) { Strings & kvstore_paths = storage_config.kvstore_data_path; @@ -294,19 +300,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."); } } } @@ -323,12 +321,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++; } @@ -341,7 +338,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 dc1d3cf4547..e491aa43175 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -4,18 +4,15 @@ #include #include #include -#include - -#define private public // hack for test #include -#undef private +#include /// 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 +#include #if !__clang__ #pragma GCC diagnostic pop #endif @@ -24,7 +21,6 @@ namespace DB { namespace tests { - static auto loadConfigFromString(const String & s) { std::istringstream ss(s); @@ -35,10 +31,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() {} @@ -46,9 +42,6 @@ class StorageConfig_test : public ::testing::Test Poco::Logger * log; }; -<<<<<<< HEAD -TEST_F(StorageConfig_test, MultiSSDSettings) -======= TEST_F(StorageConfigTest, SimpleSinglePath) try { @@ -75,7 +68,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; @@ -94,7 +87,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 @@ -134,7 +128,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; @@ -153,13 +147,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 = { @@ -204,7 +198,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 @@ -232,7 +227,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; @@ -263,7 +258,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 = { @@ -313,7 +308,7 @@ dir=["/ssd0/tiflash"] } CATCH -TEST_F(StorageConfig_test, ParseMaybeBrokenCases) +TEST_F(StorageConfigTest, ParseMaybeBrokenCases) try { Strings tests = { @@ -352,15 +347,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" @@ -412,7 +404,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) @@ -429,7 +420,7 @@ dir = [1,2,3] } CATCH -TEST(PathCapacityMetrics_test, Quota) +TEST(PathCapacityMetricsTest, Quota) try { Strings tests = { @@ -464,7 +455,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) { @@ -496,23 +487,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); @@ -524,10 +505,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() {} @@ -536,7 +517,7 @@ class UsersConfigParser_test : public ::testing::Test }; -TEST_F(UsersConfigParser_test, ParseConfigs) +TEST_F(UsersConfigParserTest, ParseConfigs) try { Strings tests = { @@ -560,7 +541,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 @@ -580,7 +561,6 @@ result_rows = 0 )", // case for set some settings R"( -<<<<<<< HEAD [profiles] [profiles.default] max_memory_usage = 123456 @@ -595,119 +575,6 @@ dt_enable_rough_set_filter = false }; for (size_t i = 0; i < tests.size(); ++i) -======= -[storage] -[storage.io_rate_limit] -max_bytes_per_sec=1024000 -max_read_bytes_per_sec=1024000 -max_write_bytes_per_sec=1024000 -foreground_write_weight=1 -background_write_weight=2 -foreground_read_weight=5 -background_read_weight=2 - )", - }; - - 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); @@ -718,7 +585,6 @@ background_read_weight=2 auto & global_ctx = TiFlashTestEnv::getGlobalContext(); global_ctx.setUsersConfig(config); -<<<<<<< HEAD // Create a copy of global_ctx auto ctx = global_ctx; for (const auto & addr_ : test_addrs) @@ -743,16 +609,64 @@ background_read_weight=2 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 +TEST_F(StorageConfigTest, CompatibilityWithIORateLimitConfig) +try +{ + Strings tests = { + R"( +path = "/tmp/tiflash/data/db0/,/tmp/tiflash/data/db1/" +[storage] +format_version = 123 +lazily_init_store = 1 +)", + R"( +path = "/tmp/tiflash/data/db0/,/tmp/tiflash/data/db1/" +[storage] +format_version = 123 +lazily_init_store = 1 +[storage.main] +dir = [ "/data0/tiflash/", "/data1/tiflash/" ] +)", + R"( +path = "/data0/tiflash/,/data1/tiflash/" +[storage] +format_version = 123 +lazily_init_store = 1 +[storage.io_rate_limit] +max_bytes_per_sec=1024000 +)", + }; + + 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 << "]"); + auto [global_capacity_quota, storage] = TiFlashStorageConfig::parseSettings(*config, log); + std::ignore = global_capacity_quota; + Strings paths; + if (i == 0) + { + paths = Strings{"/tmp/tiflash/data/db0/", "/tmp/tiflash/data/db1/"}; + } + else if (i == 1) + { + paths = Strings{"/data0/tiflash/", "/data1/tiflash/"}; + } + else if (i == 2) + { + paths = Strings{"/data0/tiflash/", "/data1/tiflash/"}; + } + ASSERT_EQ(storage.main_data_paths, paths); + ASSERT_EQ(storage.format_version, 123); + ASSERT_EQ(storage.lazily_init_store, 1); + } +} +CATCH } // namespace tests } // namespace DB diff --git a/dbms/src/Storages/PathCapacityMetrics.cpp b/dbms/src/Storages/PathCapacityMetrics.cpp index 9394f50ea29..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()) { @@ -107,16 +104,7 @@ FsStats PathCapacityMetrics::getFsStats() const // Now we assume the size of `path_infos` will not change, don't acquire heavy lock on `path_infos`. FsStats total_stat{}; -<<<<<<< HEAD for (size_t i = 0; i < path_infos.size(); ++i) -======= - - // 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) @@ -125,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; @@ -155,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); @@ -244,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 d319dae1dcd6f66c0a0690107fcfd4332e5172ce Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 16 Jun 2022 15:47:19 +0800 Subject: [PATCH 3/9] fix header Signed-off-by: JaySon-Huang --- dbms/src/Common/tests/gtest_cpptoml.cpp | 2 +- dbms/src/Server/StorageConfigParser.cpp | 2 +- dbms/src/Server/tests/gtest_storage_config.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dbms/src/Common/tests/gtest_cpptoml.cpp b/dbms/src/Common/tests/gtest_cpptoml.cpp index a4d80837556..a1206dc7414 100644 --- a/dbms/src/Common/tests/gtest_cpptoml.cpp +++ b/dbms/src/Common/tests/gtest_cpptoml.cpp @@ -1,6 +1,6 @@ +#include #include #include -#include #include #include diff --git a/dbms/src/Server/StorageConfigParser.cpp b/dbms/src/Server/StorageConfigParser.cpp index 588660e0ee1..2fc839ad7a3 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -3,7 +3,7 @@ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" #endif -#include +#include #if !__clang__ #pragma GCC diagnostic pop #endif diff --git a/dbms/src/Server/tests/gtest_storage_config.cpp b/dbms/src/Server/tests/gtest_storage_config.cpp index e491aa43175..239d151c812 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -12,7 +12,7 @@ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" #endif -#include +#include #if !__clang__ #pragma GCC diagnostic pop #endif From 9ec889b7ed239054bbdcf276beaba755d0126298 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 16 Jun 2022 16:07:16 +0800 Subject: [PATCH 4/9] try fix Signed-off-by: JaySon-Huang --- dbms/src/Common/tests/gtest_cpptoml.cpp | 7 ++-- dbms/src/Server/StorageConfigParser.cpp | 50 ++++--------------------- 2 files changed, 11 insertions(+), 46 deletions(-) diff --git a/dbms/src/Common/tests/gtest_cpptoml.cpp b/dbms/src/Common/tests/gtest_cpptoml.cpp index a1206dc7414..9cd216b8167 100644 --- a/dbms/src/Common/tests/gtest_cpptoml.cpp +++ b/dbms/src/Common/tests/gtest_cpptoml.cpp @@ -1,7 +1,6 @@ #include #include #include -#include #include @@ -32,7 +31,7 @@ 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)); + SCOPED_TRACE("[index=" + std::to_string(i) + "] [content=" + test_case + "]"); LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); std::istringstream ss(test_case); @@ -71,7 +70,7 @@ 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)); + SCOPED_TRACE("[index=" + std::to_string(i) + "] [content=" + test_case + "]"); LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); std::istringstream ss(test_case); @@ -114,7 +113,7 @@ 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)); + SCOPED_TRACE("[index=" + std::to_string(i) + "] [content=" + test_case + "]"); LOG_INFO(log, "parsing [index=" << i << "] [content=" << test_case << "]"); std::istringstream ss(test_case); diff --git a/dbms/src/Server/StorageConfigParser.cpp b/dbms/src/Server/StorageConfigParser.cpp index 2fc839ad7a3..1dba2eeeba0 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -18,7 +18,6 @@ #include #include #include -#include #include #include @@ -53,7 +52,7 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger 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); + = 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); }; @@ -92,11 +91,9 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger } if (!main_capacity_quota.empty() && main_capacity_quota.size() != main_data_paths.size()) { - 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()); + 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); throw Exception(error_msg, ErrorCodes::INVALID_CONFIG_PARAMETER); } @@ -126,11 +123,9 @@ 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.latest.dir\"[size={}] " - "is not equal to \"storage.latest.capacity\"[size={}]. " - "Please check your configuration file.", - latest_data_paths.size(), - 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); } @@ -163,30 +158,6 @@ void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger } } -void TiFlashStorageConfig::parseMisc(const String & storage_section, Poco::Logger * log) -{ - std::istringstream ss(storage_section); - cpptoml::parser p(ss); - auto table = p.parse(); - - if (table->contains("bg_task_io_rate_limit")) - { - LOG_WARNING(log, "The configuration \"bg_task_io_rate_limit\" is deprecated. Check [storage.io_rate_limit] section for new style."); - } - - 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); - } - - LOG_INFO(log, fmt::format("format_version {} lazily_init_store {}", format_version, lazily_init_store)); -} - Strings TiFlashStorageConfig::getAllNormalPaths() const { Strings all_normal_path; @@ -278,18 +249,13 @@ std::tuple TiFlashStorageConfig::parseSettings(Poc // Always try to parse storage miscellaneous configuration when [storage] section exist. if (config.has("storage")) - { - storage_config.parseMisc(config.getString("storage"), log); - } - - if (config.has("storage.main")) { 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.parseStoragePath(config.getString("storage"), log); + storage_config.parse(config.getString("storage"), log); if (config.has("raft.kvstore_path")) { From 61794bb9c2c95760964b7086b71ac72f2ba137da Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 16 Jun 2022 16:09:06 +0800 Subject: [PATCH 5/9] try fix Signed-off-by: JaySon-Huang --- dbms/src/Server/StorageConfigParser.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Server/StorageConfigParser.cpp b/dbms/src/Server/StorageConfigParser.cpp index 1dba2eeeba0..4238c928f5a 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -43,7 +43,7 @@ static std::string getCanonicalPath(std::string path) static String getNormalizedPath(const String & s) { return getCanonicalPath(Poco::Path{s}.toString()); } -void TiFlashStorageConfig::parseStoragePath(const String & storage, Poco::Logger * log) +void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) { std::istringstream ss(storage); cpptoml::parser p(ss); From 07fbf4f92bcbbc59665859b660819d3ef31b56c8 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 16 Jun 2022 16:12:45 +0800 Subject: [PATCH 6/9] add back format_version, etc Signed-off-by: JaySon-Huang --- dbms/src/Server/StorageConfigParser.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dbms/src/Server/StorageConfigParser.cpp b/dbms/src/Server/StorageConfigParser.cpp index 4238c928f5a..cf6e6c749ac 100644 --- a/dbms/src/Server/StorageConfigParser.cpp +++ b/dbms/src/Server/StorageConfigParser.cpp @@ -156,6 +156,16 @@ void TiFlashStorageConfig::parse(const String & storage, Poco::Logger * log) path = getNormalizedPath(path); LOG_INFO(log, "Raft data candidate path: " << path); } + + // rate limiter + if (auto rate_limit = table->get_qualified_as("bg_task_io_rate_limit"); rate_limit) + bg_task_io_rate_limit = *rate_limit; + + 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 From ed8b545d9d8178fcdeb3f10bd50112a4b3953c62 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 16 Jun 2022 23:55:42 +0800 Subject: [PATCH 7/9] fix ut Signed-off-by: JaySon-Huang --- dbms/src/Server/tests/gtest_storage_config.cpp | 5 ++++- dbms/src/Storages/PathCapacityMetrics.h | 4 +--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dbms/src/Server/tests/gtest_storage_config.cpp b/dbms/src/Server/tests/gtest_storage_config.cpp index 239d151c812..ac34e36c305 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -1,10 +1,13 @@ +#define private public // hack for test +#include +#undef private + #include #include #include #include #include #include -#include #include /// Suppress gcc warning: ‘*((void*)& +4)’ may be used uninitialized in this function diff --git a/dbms/src/Storages/PathCapacityMetrics.h b/dbms/src/Storages/PathCapacityMetrics.h index 116a26aa44b..1d42865416c 100644 --- a/dbms/src/Storages/PathCapacityMetrics.h +++ b/dbms/src/Storages/PathCapacityMetrics.h @@ -47,9 +47,7 @@ class PathCapacityMetrics : private boost::noncopyable CapacityInfo() = default; CapacityInfo(String p, uint64_t c) : path(std::move(p)), capacity_bytes(c) {} - CapacityInfo(const CapacityInfo & rhs) - : path(rhs.path), capacity_bytes(rhs.capacity_bytes), used_bytes(rhs.used_bytes.load()) - {} + CapacityInfo(const CapacityInfo & rhs) : path(rhs.path), capacity_bytes(rhs.capacity_bytes), used_bytes(rhs.used_bytes.load()) {} }; // Max quota bytes can be use for this TiFlash instance. From 9748e44e0b5901fb2b07ef590ed92e60c1667a03 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sun, 19 Jun 2022 15:00:20 +0800 Subject: [PATCH 8/9] fix header Signed-off-by: JaySon-Huang --- dbms/src/Server/tests/gtest_storage_config.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dbms/src/Server/tests/gtest_storage_config.cpp b/dbms/src/Server/tests/gtest_storage_config.cpp index ac34e36c305..8e31031b5bf 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -1,7 +1,3 @@ -#define private public // hack for test -#include -#undef private - #include #include #include @@ -10,6 +6,10 @@ #include #include +#define private public // hack for test +#include +#undef private + /// Suppress gcc warning: ‘*((void*)& +4)’ may be used uninitialized in this function #if !__clang__ #pragma GCC diagnostic push From ef30a19cd1651e2b393e049a1233ace3d2fdc3c9 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sun, 19 Jun 2022 16:13:38 +0800 Subject: [PATCH 9/9] remove ut Signed-off-by: JaySon-Huang --- .../src/Server/tests/gtest_storage_config.cpp | 55 ------------------- 1 file changed, 55 deletions(-) diff --git a/dbms/src/Server/tests/gtest_storage_config.cpp b/dbms/src/Server/tests/gtest_storage_config.cpp index 8e31031b5bf..65af1d833f0 100644 --- a/dbms/src/Server/tests/gtest_storage_config.cpp +++ b/dbms/src/Server/tests/gtest_storage_config.cpp @@ -616,60 +616,5 @@ dt_enable_rough_set_filter = false } CATCH -TEST_F(StorageConfigTest, CompatibilityWithIORateLimitConfig) -try -{ - Strings tests = { - R"( -path = "/tmp/tiflash/data/db0/,/tmp/tiflash/data/db1/" -[storage] -format_version = 123 -lazily_init_store = 1 -)", - R"( -path = "/tmp/tiflash/data/db0/,/tmp/tiflash/data/db1/" -[storage] -format_version = 123 -lazily_init_store = 1 -[storage.main] -dir = [ "/data0/tiflash/", "/data1/tiflash/" ] -)", - R"( -path = "/data0/tiflash/,/data1/tiflash/" -[storage] -format_version = 123 -lazily_init_store = 1 -[storage.io_rate_limit] -max_bytes_per_sec=1024000 -)", - }; - - 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 << "]"); - auto [global_capacity_quota, storage] = TiFlashStorageConfig::parseSettings(*config, log); - std::ignore = global_capacity_quota; - Strings paths; - if (i == 0) - { - paths = Strings{"/tmp/tiflash/data/db0/", "/tmp/tiflash/data/db1/"}; - } - else if (i == 1) - { - paths = Strings{"/data0/tiflash/", "/data1/tiflash/"}; - } - else if (i == 2) - { - paths = Strings{"/data0/tiflash/", "/data1/tiflash/"}; - } - ASSERT_EQ(storage.main_data_paths, paths); - ASSERT_EQ(storage.format_version, 123); - ASSERT_EQ(storage.lazily_init_store, 1); - } -} -CATCH - } // namespace tests } // namespace DB