From e21ab223c5ae7af17b6a1722b559dfcea17bf851 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Tue, 8 Mar 2022 16:11:21 +0800 Subject: [PATCH 01/52] finish dev --- programs/copier/ClusterCopierApp.cpp | 8 ++++++++ programs/extract-from-config/ExtractFromConfig.cpp | 14 ++++++++++++-- programs/server/Server.cpp | 6 ++++-- src/Interpreters/Context.cpp | 7 ++++--- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/programs/copier/ClusterCopierApp.cpp b/programs/copier/ClusterCopierApp.cpp index d1ea59ed119..5f8bb84f47b 100644 --- a/programs/copier/ClusterCopierApp.cpp +++ b/programs/copier/ClusterCopierApp.cpp @@ -12,6 +12,11 @@ namespace fs = std::filesystem; namespace DB { +namespace ErrorCodes +{ + extern const int EXCESSIVE_ELEMENT_IN_CONFIG; +} + /// ClusterCopierApp void ClusterCopierApp::initialize(Poco::Util::Application & self) @@ -192,6 +197,9 @@ void ClusterCopierApp::mainImpl() if (!task_file.empty()) copier->uploadTaskDescription(task_path, task_file, config().getBool("task-upload-force", false)); + if (config().has("zookeeper") || config().has("keeper")) + throw Exception("Both zookeeper and keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); + copier->init(); copier->process(ConnectionTimeouts::getTCPTimeoutsWithoutFailover(context->getSettingsRef())); diff --git a/programs/extract-from-config/ExtractFromConfig.cpp b/programs/extract-from-config/ExtractFromConfig.cpp index 3fd665bcb26..e346ea83888 100644 --- a/programs/extract-from-config/ExtractFromConfig.cpp +++ b/programs/extract-from-config/ExtractFromConfig.cpp @@ -12,6 +12,10 @@ #include #include +namespace DB::ErrorCodes +{ + extern const int EXCESSIVE_ELEMENT_IN_CONFIG; +} static void setupLogging(const std::string & log_level) { @@ -32,8 +36,14 @@ static std::string extractFromConfig( if (has_zk_includes && process_zk_includes) { DB::ConfigurationPtr bootstrap_configuration(new Poco::Util::XMLConfiguration(config_xml)); - zkutil::ZooKeeperPtr zookeeper = std::make_shared( - *bootstrap_configuration, "zookeeper", nullptr); + + if (bootstrap_configuration->has("zookeeper") && bootstrap_configuration->has("keeper")) + throw DB::Exception(DB::ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG, "Both zookeeper and keeper are specified"); + + zkutil::ZooKeeperPtr zookeeper; + zookeeper = std::make_shared( + *bootstrap_configuration, bootstrap_configuration->has("zookeeper") ? "zookeeper" : "keeper", nullptr); + zkutil::ZooKeeperNodeCache zk_node_cache([&] { return zookeeper; }); config_xml = processor.processConfig(&has_zk_includes, &zk_node_cache); } diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index b856131d821..72b5eb6540a 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -592,7 +592,9 @@ if (ThreadFuzzer::instance().isEffective()) ConnectionCollector::init(global_context, config().getUInt("max_threads_for_connection_collector", 10)); - bool has_zookeeper = config().has("zookeeper"); + if (config().has("zookeeper") && config().has("keeper")) + throw Exception("Both zookeeper and keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); + bool has_zookeeper = config().has("zookeeper") || config().has("keeper"); zkutil::ZooKeeperNodeCache main_config_zk_node_cache([&] { return global_context->getZooKeeper(); }); zkutil::EventPtr main_config_zk_changed_event = std::make_shared(); @@ -955,7 +957,7 @@ if (ThreadFuzzer::instance().isEffective()) { /// We do not load ZooKeeper configuration on the first config loading /// because TestKeeper server is not started yet. - if (config->has("zookeeper")) + if (config->has("zookeeper") || config->has("keeper")) global_context->reloadZooKeeperIfChanged(config); global_context->reloadAuxiliaryZooKeepersConfigIfChanged(config); diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index b03875eed7a..756afed415a 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1847,7 +1847,8 @@ zkutil::ZooKeeperPtr Context::getZooKeeper() const const auto & config = shared->zookeeper_config ? *shared->zookeeper_config : getConfigRef(); if (!shared->zookeeper) - shared->zookeeper = std::make_shared(config, "zookeeper", getZooKeeperLog()); + shared->zookeeper + = std::make_shared(config, config.has("zookeeper") ? "zookeeper" : "keeper", getZooKeeperLog()); else if (shared->zookeeper->expired()) shared->zookeeper = shared->zookeeper->startNewSession(); @@ -2058,7 +2059,7 @@ void Context::reloadZooKeeperIfChanged(const ConfigurationPtr & config) const { std::lock_guard lock(shared->zookeeper_mutex); shared->zookeeper_config = config; - reloadZooKeeperIfChangedImpl(config, "zookeeper", shared->zookeeper, getZooKeeperLog()); + reloadZooKeeperIfChangedImpl(config, config->has("zookeeper") ? "zookeeper" : "keeper", shared->zookeeper, getZooKeeperLog()); } void Context::reloadAuxiliaryZooKeepersConfigIfChanged(const ConfigurationPtr & config) @@ -2082,7 +2083,7 @@ void Context::reloadAuxiliaryZooKeepersConfigIfChanged(const ConfigurationPtr & bool Context::hasZooKeeper() const { - return getConfigRef().has("zookeeper"); + return getConfigRef().has("zookeeper") || getConfigRef().has("keeper"); } bool Context::hasAuxiliaryZooKeeper(const String & name) const From d78493eeb2242531ead1b6fce8033cdd0220e6a4 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Tue, 8 Mar 2022 16:41:16 +0800 Subject: [PATCH 02/52] commit again --- programs/copier/ClusterCopierApp.cpp | 2 +- programs/server/Server.cpp | 1 + src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp | 2 +- src/Interpreters/Context.cpp | 2 +- .../examples/get_abandonable_lock_in_all_partitions.cpp | 2 +- src/Storages/examples/get_current_inserts_in_replicated.cpp | 2 +- 6 files changed, 6 insertions(+), 5 deletions(-) diff --git a/programs/copier/ClusterCopierApp.cpp b/programs/copier/ClusterCopierApp.cpp index 5f8bb84f47b..6e674473af3 100644 --- a/programs/copier/ClusterCopierApp.cpp +++ b/programs/copier/ClusterCopierApp.cpp @@ -197,7 +197,7 @@ void ClusterCopierApp::mainImpl() if (!task_file.empty()) copier->uploadTaskDescription(task_path, task_file, config().getBool("task-upload-force", false)); - if (config().has("zookeeper") || config().has("keeper")) + if (config().has("zookeeper") && config().has("keeper")) throw Exception("Both zookeeper and keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); copier->init(); diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 72b5eb6540a..049d97f03f6 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -594,6 +594,7 @@ if (ThreadFuzzer::instance().isEffective()) if (config().has("zookeeper") && config().has("keeper")) throw Exception("Both zookeeper and keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); + bool has_zookeeper = config().has("zookeeper") || config().has("keeper"); zkutil::ZooKeeperNodeCache main_config_zk_node_cache([&] { return global_context->getZooKeeper(); }); diff --git a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp b/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp index cf819121234..e91296af61b 100644 --- a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp +++ b/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp @@ -25,7 +25,7 @@ int main(int argc, char ** argv) DB::ConfigProcessor processor(argv[1], false, true); auto config = processor.loadConfig().configuration; - zkutil::ZooKeeper zk(*config, "zookeeper", nullptr); + zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : "keeper", nullptr); zkutil::EventPtr watch = std::make_shared(); /// NOTE: setting watches in multiple threads because doing it in a single thread is too slow. diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 756afed415a..b7759e461ca 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1883,7 +1883,7 @@ bool Context::tryCheckClientConnectionToMyKeeperCluster() const try { /// If our server is part of main Keeper cluster - if (checkZooKeeperConfigIsLocal(getConfigRef(), "zookeeper")) + if (checkZooKeeperConfigIsLocal(getConfigRef(), "zookeeper") || checkZooKeeperConfigIsLocal(getConfigRef(), "keeper")) { LOG_DEBUG(shared->log, "Keeper server is participant of the main zookeeper cluster, will try to connect to it"); getZooKeeper(); diff --git a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp index e2145f5afb0..d947af71a60 100644 --- a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp +++ b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp @@ -26,7 +26,7 @@ try auto config = processor.loadConfig().configuration; String root_path = argv[2]; - zkutil::ZooKeeper zk(*config, "zookeeper", nullptr); + zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : "keeper", nullptr); String temp_path = root_path + "/temp"; String blocks_path = root_path + "/block_numbers"; diff --git a/src/Storages/examples/get_current_inserts_in_replicated.cpp b/src/Storages/examples/get_current_inserts_in_replicated.cpp index d7dedbcab9c..2355851bd89 100644 --- a/src/Storages/examples/get_current_inserts_in_replicated.cpp +++ b/src/Storages/examples/get_current_inserts_in_replicated.cpp @@ -29,7 +29,7 @@ try auto config = processor.loadConfig().configuration; String zookeeper_path = argv[2]; - auto zookeeper = std::make_shared(*config, "zookeeper", nullptr); + auto zookeeper = std::make_shared(*config, config->has("zookeeper") ? "zookeeper" : "keeper", nullptr); std::unordered_map> current_inserts; From 3b957367e160bb986d590fe1ccf98ca4951a4833 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Tue, 8 Mar 2022 17:03:24 +0800 Subject: [PATCH 03/52] add tests --- .../test_zookeeper_config/configs/zookeeper_config_root_a.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml b/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml index 6c413378524..d3c62862002 100644 --- a/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml +++ b/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml @@ -1,5 +1,5 @@ - + zoo1 2181 @@ -14,5 +14,5 @@ 3000 /root_a - + From e2bb06dbb5eab4db2326862cc6b4b9ee27d9ac5a Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Tue, 8 Mar 2022 17:14:55 +0800 Subject: [PATCH 04/52] add tests --- tests/integration/helpers/zookeeper_secure_config.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/helpers/zookeeper_secure_config.xml b/tests/integration/helpers/zookeeper_secure_config.xml index b3b20eb3429..7f4e6f8a12b 100644 --- a/tests/integration/helpers/zookeeper_secure_config.xml +++ b/tests/integration/helpers/zookeeper_secure_config.xml @@ -1,5 +1,5 @@ - + zoo1 2281 @@ -13,5 +13,5 @@ 2281 3000 - + From 22a938fff02bb8592947c5f0ce604d5c96c76ec2 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Tue, 8 Mar 2022 19:16:09 +0800 Subject: [PATCH 05/52] solve issue: https://github.com/ClickHouse/ClickHouse/issues/34767 --- programs/server/Server.cpp | 4 +- src/Common/ZooKeeper/ZooKeeper.cpp | 56 ++++++++++++++++--- .../examples/zk_many_watches_reconnect.cpp | 2 +- src/Interpreters/Context.cpp | 21 +++++-- ...get_abandonable_lock_in_all_partitions.cpp | 2 +- .../get_current_inserts_in_replicated.cpp | 7 ++- 6 files changed, 74 insertions(+), 18 deletions(-) diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 049d97f03f6..1ed0d27c80d 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -595,7 +595,7 @@ if (ThreadFuzzer::instance().isEffective()) if (config().has("zookeeper") && config().has("keeper")) throw Exception("Both zookeeper and keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); - bool has_zookeeper = config().has("zookeeper") || config().has("keeper"); + bool has_zookeeper = config().has("zookeeper") || config().has("keeper") || config().has("keeper_server"); zkutil::ZooKeeperNodeCache main_config_zk_node_cache([&] { return global_context->getZooKeeper(); }); zkutil::EventPtr main_config_zk_changed_event = std::make_shared(); @@ -958,7 +958,7 @@ if (ThreadFuzzer::instance().isEffective()) { /// We do not load ZooKeeper configuration on the first config loading /// because TestKeeper server is not started yet. - if (config->has("zookeeper") || config->has("keeper")) + if (config->has("zookeeper") || config->has("keeper") || config->has("keeper_server")) global_context->reloadZooKeeperIfChanged(config); global_context->reloadAuxiliaryZooKeepersConfigIfChanged(config); diff --git a/src/Common/ZooKeeper/ZooKeeper.cpp b/src/Common/ZooKeeper/ZooKeeper.cpp index b1574341c40..19647166b2a 100644 --- a/src/Common/ZooKeeper/ZooKeeper.cpp +++ b/src/Common/ZooKeeper/ZooKeeper.cpp @@ -175,14 +175,32 @@ ZooKeeper::ZooKeeper(const Strings & hosts_, const std::string & identity_, int3 struct ZooKeeperArgs { +public: ZooKeeperArgs(const Poco::Util::AbstractConfiguration & config, const std::string & config_name) { - Poco::Util::AbstractConfiguration::Keys keys; - config.keys(config_name, keys); - session_timeout_ms = Coordination::DEFAULT_SESSION_TIMEOUT_MS; operation_timeout_ms = Coordination::DEFAULT_OPERATION_TIMEOUT_MS; implementation = "zookeeper"; + + if (endsWith(config_name, "keeper_server.raft_configuration")) + initFromSectionKeeperServer(config, config_name); + else + initFromSectionKeeper(config, config_name); + + if (!chroot.empty()) + { + if (chroot.front() != '/') + throw KeeperException(std::string("Root path in config file should start with '/', but got ") + chroot, Coordination::Error::ZBADARGUMENTS); + if (chroot.back() == '/') + chroot.pop_back(); + } + } + +private: + void initFromSectionKeeper(const Poco::Util::AbstractConfiguration & config, const std::string & config_name) + { + Poco::Util::AbstractConfiguration::Keys keys; + config.keys(config_name, keys); for (const auto & key : keys) { if (startsWith(key, "node")) @@ -216,16 +234,38 @@ struct ZooKeeperArgs else throw KeeperException(std::string("Unknown key ") + key + " in config file", Coordination::Error::ZBADARGUMENTS); } + } - if (!chroot.empty()) + void initFromSectionKeeperServer(const Poco::Util::AbstractConfiguration & config, const std::string & config_name) + { + Poco::Util::AbstractConfiguration::Keys keys; + config.keys(config_name, keys); + + bool secure = false; + for (const auto & key : keys) { - if (chroot.front() != '/') - throw KeeperException(std::string("Root path in config file should start with '/', but got ") + chroot, Coordination::Error::ZBADARGUMENTS); - if (chroot.back() == '/') - chroot.pop_back(); + if (key == "server") + { + hosts.push_back( + // (config.getBool(config_name + "." + key + ".secure", false) ? "secure://" : "") + + config.getString(config_name + "." + key + ".hostname") + ":" + + config.getString(config_name + "." + key + ".port", "9234") + ); + } + else if (key == "secure") + { + secure = config.getBool(config_name + "." + key, false); + } + } + + if (secure && !hosts.empty()) + { + for (auto & host : hosts) + host = "secure://" + host; } } +public: Strings hosts; std::string identity; int session_timeout_ms; diff --git a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp b/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp index e91296af61b..b68035b4c4c 100644 --- a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp +++ b/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp @@ -25,7 +25,7 @@ int main(int argc, char ** argv) DB::ConfigProcessor processor(argv[1], false, true); auto config = processor.loadConfig().configuration; - zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : "keeper", nullptr); + zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : config->has("keeper") ? "keeper" : "keeper_server", nullptr); zkutil::EventPtr watch = std::make_shared(); /// NOTE: setting watches in multiple threads because doing it in a single thread is too slow. diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index b7759e461ca..a8efe834034 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1847,8 +1847,12 @@ zkutil::ZooKeeperPtr Context::getZooKeeper() const const auto & config = shared->zookeeper_config ? *shared->zookeeper_config : getConfigRef(); if (!shared->zookeeper) - shared->zookeeper - = std::make_shared(config, config.has("zookeeper") ? "zookeeper" : "keeper", getZooKeeperLog()); + shared->zookeeper = std::make_shared( + config, + config.has("zookeeper") ? "zookeeper" + : config.has("keeper") ? "keeper" + : "keeper_server", + getZooKeeperLog()); else if (shared->zookeeper->expired()) shared->zookeeper = shared->zookeeper->startNewSession(); @@ -1883,7 +1887,8 @@ bool Context::tryCheckClientConnectionToMyKeeperCluster() const try { /// If our server is part of main Keeper cluster - if (checkZooKeeperConfigIsLocal(getConfigRef(), "zookeeper") || checkZooKeeperConfigIsLocal(getConfigRef(), "keeper")) + if (checkZooKeeperConfigIsLocal(getConfigRef(), "zookeeper") || checkZooKeeperConfigIsLocal(getConfigRef(), "keeper") + || (!getConfigRef().has("zookeeper") && !getConfigRef().has("keeper") && getConfigRef().has("keeper_server"))) { LOG_DEBUG(shared->log, "Keeper server is participant of the main zookeeper cluster, will try to connect to it"); getZooKeeper(); @@ -2059,7 +2064,13 @@ void Context::reloadZooKeeperIfChanged(const ConfigurationPtr & config) const { std::lock_guard lock(shared->zookeeper_mutex); shared->zookeeper_config = config; - reloadZooKeeperIfChangedImpl(config, config->has("zookeeper") ? "zookeeper" : "keeper", shared->zookeeper, getZooKeeperLog()); + reloadZooKeeperIfChangedImpl( + config, + config->has("zookeeper") ? "zookeeper" + : config->has("keeper") ? "keeper" + : "keeper_server", + shared->zookeeper, + getZooKeeperLog()); } void Context::reloadAuxiliaryZooKeepersConfigIfChanged(const ConfigurationPtr & config) @@ -2083,7 +2094,7 @@ void Context::reloadAuxiliaryZooKeepersConfigIfChanged(const ConfigurationPtr & bool Context::hasZooKeeper() const { - return getConfigRef().has("zookeeper") || getConfigRef().has("keeper"); + return getConfigRef().has("zookeeper") || getConfigRef().has("keeper") || getConfigRef().has("keeper_server"); } bool Context::hasAuxiliaryZooKeeper(const String & name) const diff --git a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp index d947af71a60..ee068d1bf26 100644 --- a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp +++ b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp @@ -26,7 +26,7 @@ try auto config = processor.loadConfig().configuration; String root_path = argv[2]; - zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : "keeper", nullptr); + zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : config->has("keeper") ? "keeper" : "keeper_server", nullptr); String temp_path = root_path + "/temp"; String blocks_path = root_path + "/block_numbers"; diff --git a/src/Storages/examples/get_current_inserts_in_replicated.cpp b/src/Storages/examples/get_current_inserts_in_replicated.cpp index 2355851bd89..c4ffa08ce27 100644 --- a/src/Storages/examples/get_current_inserts_in_replicated.cpp +++ b/src/Storages/examples/get_current_inserts_in_replicated.cpp @@ -29,7 +29,12 @@ try auto config = processor.loadConfig().configuration; String zookeeper_path = argv[2]; - auto zookeeper = std::make_shared(*config, config->has("zookeeper") ? "zookeeper" : "keeper", nullptr); + auto zookeeper = std::make_shared( + *config, + config->has("zookeeper") ? "zookeeper" + : config->has("keeper") ? "keeper" + : "keeper_server", + nullptr); std::unordered_map> current_inserts; From 08c3b361080ca2af99f6df1e143b5c9d8441b324 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Wed, 9 Mar 2022 12:25:30 +0800 Subject: [PATCH 06/52] commit again --- src/Common/ZooKeeper/ZooKeeper.cpp | 54 ++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeper.cpp b/src/Common/ZooKeeper/ZooKeeper.cpp index 19647166b2a..c4a562ae6b7 100644 --- a/src/Common/ZooKeeper/ZooKeeper.cpp +++ b/src/Common/ZooKeeper/ZooKeeper.cpp @@ -182,7 +182,7 @@ public: operation_timeout_ms = Coordination::DEFAULT_OPERATION_TIMEOUT_MS; implementation = "zookeeper"; - if (endsWith(config_name, "keeper_server.raft_configuration")) + if (endsWith(config_name, "keeper_server")) initFromSectionKeeperServer(config, config_name); else initFromSectionKeeper(config, config_name); @@ -238,30 +238,48 @@ private: void initFromSectionKeeperServer(const Poco::Util::AbstractConfiguration & config, const std::string & config_name) { - Poco::Util::AbstractConfiguration::Keys keys; + Poco::Util::AbstractConfiguration::Keys keys; config.keys(config_name, keys); - + bool secure = false; + String tcp_port; + String tcp_port_secure; for (const auto & key : keys) { - if (key == "server") + if (key == "tcp_port_secure") + { + secure = true; + tcp_port_secure = config.getString(config_name + "." + key); + } + else if (key == "tcp_port") + { + tcp_port = config.getString(config_name + "." + key); + } + else if (key == "coordination_settings") + { + if (config.has(config_name + "." + key + ".operation_timeout_ms")) + operation_timeout_ms = config.getInt(config_name + "." + key + ".operation_timeout_ms"); + if (config.has(config_name + "." + key + ".session_timeout_ms")) + session_timeout_ms = config.getInt(config_name + "." + key + ".session_timeout_ms"); + } + + /// TODO: consider digest + } + + if (secure && tcp_port_secure.empty()) + throw KeeperException("No tcp_port_secure in config file", Coordination::Error::ZBADARGUMENTS); + if (!secure && tcp_port.empty()) + throw KeeperException("No tcp_port in config file", Coordination::Error::ZBADARGUMENTS); + + config.keys(config_name + ".raft_configuration", keys); + for (const auto & key : keys) + { + if (startsWith(key, "server")) { hosts.push_back( - // (config.getBool(config_name + "." + key + ".secure", false) ? "secure://" : "") + - config.getString(config_name + "." + key + ".hostname") + ":" - + config.getString(config_name + "." + key + ".port", "9234") - ); + (secure ? "secure://" : "") + config.getString(config_name + ".raft_configuration." + key + ".hostname") + ":" + + (secure ? tcp_port_secure : tcp_port)); } - else if (key == "secure") - { - secure = config.getBool(config_name + "." + key, false); - } - } - - if (secure && !hosts.empty()) - { - for (auto & host : hosts) - host = "secure://" + host; } } From 4e3098bbc21f13a827af6a4132341bfd3df1d2fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E6=89=AC?= <654010905@qq.com> Date: Wed, 28 Dec 2022 15:06:57 +0800 Subject: [PATCH 07/52] Update programs/extract-from-config/ExtractFromConfig.cpp Co-authored-by: Antonio Andelic --- programs/extract-from-config/ExtractFromConfig.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/extract-from-config/ExtractFromConfig.cpp b/programs/extract-from-config/ExtractFromConfig.cpp index 2d50ba240a7..75b0d311fdb 100644 --- a/programs/extract-from-config/ExtractFromConfig.cpp +++ b/programs/extract-from-config/ExtractFromConfig.cpp @@ -96,7 +96,7 @@ static std::vector extractFromConfig( DB::ConfigurationPtr bootstrap_configuration(new Poco::Util::XMLConfiguration(config_xml)); if (bootstrap_configuration->has("zookeeper") && bootstrap_configuration->has("keeper")) - throw DB::Exception(DB::ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG, "Both zookeeper and keeper are specified"); + throw DB::Exception(DB::ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG, "Both ZooKeeper and Keeper are specified"); zkutil::ZooKeeperPtr zookeeper; zookeeper = std::make_shared( From 24052916121e479233d4c3661c9182a109849dff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E6=89=AC?= <654010905@qq.com> Date: Wed, 28 Dec 2022 15:07:06 +0800 Subject: [PATCH 08/52] Update programs/copier/ClusterCopierApp.cpp Co-authored-by: Antonio Andelic --- programs/copier/ClusterCopierApp.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/copier/ClusterCopierApp.cpp b/programs/copier/ClusterCopierApp.cpp index 024cab2a32d..e8d62224c48 100644 --- a/programs/copier/ClusterCopierApp.cpp +++ b/programs/copier/ClusterCopierApp.cpp @@ -198,7 +198,7 @@ void ClusterCopierApp::mainImpl() copier->uploadTaskDescription(task_path, task_file, config().getBool("task-upload-force", false)); if (config().has("zookeeper") && config().has("keeper")) - throw Exception("Both zookeeper and keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); + throw Exception("Both ZooKeeper and Keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); copier->init(); copier->process(ConnectionTimeouts::getTCPTimeoutsWithoutFailover(context->getSettingsRef())); From 56b6378c121fd4ee17e8465ee21f47f502575235 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9D=8E=E6=89=AC?= <654010905@qq.com> Date: Wed, 28 Dec 2022 15:08:50 +0800 Subject: [PATCH 09/52] Update programs/server/Server.cpp Co-authored-by: Antonio Andelic --- programs/server/Server.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 759e652562c..69e56abed16 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -767,7 +767,7 @@ try ConnectionCollector::init(global_context, config().getUInt("max_threads_for_connection_collector", 10)); if (config().has("zookeeper") && config().has("keeper")) - throw Exception("Both zookeeper and keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); + throw Exception("Both ZooKeeper and Keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); bool has_zookeeper = config().has("zookeeper") || config().has("keeper") || config().has("keeper_server"); From 866c8d0fd0ced14a71b59625e521fcb49799e947 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Wed, 28 Dec 2022 16:04:34 +0800 Subject: [PATCH 10/52] change as request --- src/Common/ZooKeeper/ZooKeeper.cpp | 13 +++++++++++++ src/Common/ZooKeeper/ZooKeeper.h | 2 ++ .../examples/zk_many_watches_reconnect.cpp | 2 +- src/Interpreters/Context.cpp | 15 ++------------- .../get_abandonable_lock_in_all_partitions.cpp | 2 +- .../get_current_inserts_in_replicated.cpp | 7 +------ 6 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/Common/ZooKeeper/ZooKeeper.cpp b/src/Common/ZooKeeper/ZooKeeper.cpp index ab5d918e1f0..6bb7b465a6d 100644 --- a/src/Common/ZooKeeper/ZooKeeper.cpp +++ b/src/Common/ZooKeeper/ZooKeeper.cpp @@ -29,6 +29,7 @@ namespace ErrorCodes extern const int LOGICAL_ERROR; extern const int NOT_IMPLEMENTED; extern const int BAD_ARGUMENTS; + extern const int NO_ELEMENTS_IN_CONFIG; } } @@ -1333,4 +1334,16 @@ String getSequentialNodeName(const String & prefix, UInt64 number) return name; } +String getZookeeperConfigName(const Poco::Util::AbstractConfiguration & config) +{ + if (config.has("zookeeper")) + return "zookeeper"; + else if (config.has("keeper")) + return "keeper"; + else if (config.has("keeper_server")) + return "keeper_server"; + else + throw DB::Exception("There is no Zookeeper configuration in server config", DB::ErrorCodes::NO_ELEMENTS_IN_CONFIG); +} + } diff --git a/src/Common/ZooKeeper/ZooKeeper.h b/src/Common/ZooKeeper/ZooKeeper.h index 9de8241cfbe..67ae166452f 100644 --- a/src/Common/ZooKeeper/ZooKeeper.h +++ b/src/Common/ZooKeeper/ZooKeeper.h @@ -645,4 +645,6 @@ String extractZooKeeperPath(const String & path, bool check_starts_with_slash, P String getSequentialNodeName(const String & prefix, UInt64 number); +String getZookeeperConfigName(const Poco::Util::AbstractConfiguration & config); + } diff --git a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp b/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp index b68035b4c4c..aad8913ca8b 100644 --- a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp +++ b/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp @@ -25,7 +25,7 @@ int main(int argc, char ** argv) DB::ConfigProcessor processor(argv[1], false, true); auto config = processor.loadConfig().configuration; - zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : config->has("keeper") ? "keeper" : "keeper_server", nullptr); + zkutil::ZooKeeper zk(*config, zkutil::getZookeeperConfigName(*config), nullptr); zkutil::EventPtr watch = std::make_shared(); /// NOTE: setting watches in multiple threads because doing it in a single thread is too slow. diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 5fb2715c6ce..2713ee5b50e 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -2233,12 +2233,7 @@ zkutil::ZooKeeperPtr Context::getZooKeeper() const const auto & config = shared->zookeeper_config ? *shared->zookeeper_config : getConfigRef(); if (!shared->zookeeper) - shared->zookeeper = std::make_shared( - config, - config.has("zookeeper") ? "zookeeper" - : config.has("keeper") ? "keeper" - : "keeper_server", - getZooKeeperLog()); + shared->zookeeper = std::make_shared(config, zkutil::getZookeeperConfigName(config), getZooKeeperLog()); else if (shared->zookeeper->expired()) { Stopwatch watch; @@ -2478,13 +2473,7 @@ void Context::reloadZooKeeperIfChanged(const ConfigurationPtr & config) const { std::lock_guard lock(shared->zookeeper_mutex); shared->zookeeper_config = config; - reloadZooKeeperIfChangedImpl( - config, - config->has("zookeeper") ? "zookeeper" - : config->has("keeper") ? "keeper" - : "keeper_server", - shared->zookeeper, - getZooKeeperLog()); + reloadZooKeeperIfChangedImpl(config, zkutil::getZookeeperConfigName(*config), shared->zookeeper, getZooKeeperLog()); } void Context::reloadAuxiliaryZooKeepersConfigIfChanged(const ConfigurationPtr & config) diff --git a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp index ee068d1bf26..e1faa67eb45 100644 --- a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp +++ b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp @@ -26,7 +26,7 @@ try auto config = processor.loadConfig().configuration; String root_path = argv[2]; - zkutil::ZooKeeper zk(*config, config->has("zookeeper") ? "zookeeper" : config->has("keeper") ? "keeper" : "keeper_server", nullptr); + zkutil::ZooKeeper zk(*config, zkutil::getZookeeperConfigName(*config), nullptr); String temp_path = root_path + "/temp"; String blocks_path = root_path + "/block_numbers"; diff --git a/src/Storages/examples/get_current_inserts_in_replicated.cpp b/src/Storages/examples/get_current_inserts_in_replicated.cpp index 36584b8593a..9ba75b81440 100644 --- a/src/Storages/examples/get_current_inserts_in_replicated.cpp +++ b/src/Storages/examples/get_current_inserts_in_replicated.cpp @@ -29,12 +29,7 @@ try auto config = processor.loadConfig().configuration; String zookeeper_path = argv[2]; - auto zookeeper = std::make_shared( - *config, - config->has("zookeeper") ? "zookeeper" - : config->has("keeper") ? "keeper" - : "keeper_server", - nullptr); + auto zookeeper = std::make_shared(*config, zkutil::getZookeeperConfigName(*config), nullptr); std::unordered_map> current_inserts; From 28f726f82532f85214d71d4de4358cf4dad39239 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Wed, 28 Dec 2022 16:58:21 +0800 Subject: [PATCH 11/52] add zookeeper integration tests --- .../__init__.py | 0 .../configs/keeper_config.xml | 17 ++++++ .../configs/remote_servers.xml | 18 +++++++ .../configs/zookeeper_config.xml | 17 ++++++ .../test_alternative_keeper_config/test.py | 54 +++++++++++++++++++ 5 files changed, 106 insertions(+) create mode 100644 tests/integration/test_alternative_keeper_config/__init__.py create mode 100644 tests/integration/test_alternative_keeper_config/configs/keeper_config.xml create mode 100644 tests/integration/test_alternative_keeper_config/configs/remote_servers.xml create mode 100644 tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml create mode 100644 tests/integration/test_alternative_keeper_config/test.py diff --git a/tests/integration/test_alternative_keeper_config/__init__.py b/tests/integration/test_alternative_keeper_config/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_alternative_keeper_config/configs/keeper_config.xml b/tests/integration/test_alternative_keeper_config/configs/keeper_config.xml new file mode 100644 index 00000000000..bd783b83853 --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs/keeper_config.xml @@ -0,0 +1,17 @@ + + + + zoo1 + 2181 + + + zoo2 + 2181 + + + zoo3 + 2181 + + 3000 + + diff --git a/tests/integration/test_alternative_keeper_config/configs/remote_servers.xml b/tests/integration/test_alternative_keeper_config/configs/remote_servers.xml new file mode 100644 index 00000000000..e77cc5c65e6 --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs/remote_servers.xml @@ -0,0 +1,18 @@ + + + + + + node1 + 9000 + + + + node2 + 9000 + + + + + + diff --git a/tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml b/tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml new file mode 100644 index 00000000000..7a0d7c1de92 --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml @@ -0,0 +1,17 @@ + + + + zoo1 + 2181 + + + zoo2 + 2181 + + + zoo3 + 2181 + + 3000 + + diff --git a/tests/integration/test_alternative_keeper_config/test.py b/tests/integration/test_alternative_keeper_config/test.py new file mode 100644 index 00000000000..86877e677ba --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/test.py @@ -0,0 +1,54 @@ +import time +import pytest +import logging +from helpers.cluster import ClickHouseCluster +from tests.integration.helpers.test_tools import TSV + +cluster = ClickHouseCluster(__file__) + +node1 = cluster.add_instance( + "node1", + with_zookeeper=True, + main_configs=["configs/remote_servers.xml", "configs/keeper_config.xml"], + macros={"replica": "node1"}, +) + +node2 = cluster.add_instance( + "node2", + with_zookeeper=True, + main_configs=["configs/remote_servers.xml", "configs/zookeeper_config.xml"], + macros={"replica": "node2"}, +) + + +@pytest.fixture(scope="module", autouse=True) +def started_cluster(): + try: + cluster.start() + yield cluster + + finally: + cluster.shutdown() + + +def test_create_insert(started_cluster): + node1.query("DROP TABLE IF EXISTS tbl ON CLUSTER 'test_cluster' NO DELAY") + node1.query( + """ + CREATE TABLE tbl ON CLUSTER 'test_cluster' ( + id Int64, + str String + ) ENGINE=ReplicatedMergeTree('/clickhouse/tables/tbl/', '{replica}') + ORDER BY id + """ + ) + + node1.query("INSERT INTO tbl VALUES (1, 'str1')") + node2.query("INSERT INTO tbl VALUES (1, 'str1')") # Test deduplication + node2.query("INSERT INTO tbl VALUES (2, 'str2')") + + expected = [[1, "str1"], [2, "str2"]] + assert node1.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) + assert node2.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) + assert node1.query("CHECK TABLE tbl") == "1\n" + assert node2.query("CHECK TABLE tbl") == "1\n" From dc34393f668919684a2a1c20aec18aac8364e271 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Wed, 28 Dec 2022 18:27:36 +0800 Subject: [PATCH 12/52] add another test --- .../configs_keeper_server/enable_keeper1.xml | 33 ++++++++++ .../configs_keeper_server/enable_keeper2.xml | 33 ++++++++++ .../configs_keeper_server/remote_servers.xml | 18 ++++++ .../configs_keeper_server/use_keeper.xml | 12 ++++ .../test_keeper_server.py | 61 +++++++++++++++++++ 5 files changed, 157 insertions(+) create mode 100644 tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper1.xml create mode 100644 tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper2.xml create mode 100644 tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml create mode 100644 tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml create mode 100644 tests/integration/test_alternative_keeper_config/test_keeper_server.py diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper1.xml b/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper1.xml new file mode 100644 index 00000000000..7c2e283e89f --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper1.xml @@ -0,0 +1,33 @@ + + + 9181 + 1 + /var/lib/clickhouse/coordination/log + /var/lib/clickhouse/coordination/snapshots + + + 5000 + 10000 + 75 + trace + + + + + 1 + node1 + 9234 + true + 3 + + + 2 + node2 + 9234 + true + true + 2 + + + + diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper2.xml b/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper2.xml new file mode 100644 index 00000000000..618e6a04aec --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper2.xml @@ -0,0 +1,33 @@ + + + 9181 + 2 + /var/lib/clickhouse/coordination/log + /var/lib/clickhouse/coordination/snapshots + + + 5000 + 10000 + 75 + trace + + + + + 1 + node1 + 9234 + true + 3 + + + 2 + node2 + 9234 + true + true + 2 + + + + diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml b/tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml new file mode 100644 index 00000000000..e77cc5c65e6 --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml @@ -0,0 +1,18 @@ + + + + + + node1 + 9000 + + + + node2 + 9000 + + + + + + diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml b/tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml new file mode 100644 index 00000000000..b250f06cf81 --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml @@ -0,0 +1,12 @@ + + + + node1 + 9181 + + + node2 + 9181 + + + diff --git a/tests/integration/test_alternative_keeper_config/test_keeper_server.py b/tests/integration/test_alternative_keeper_config/test_keeper_server.py new file mode 100644 index 00000000000..7311b32e0af --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/test_keeper_server.py @@ -0,0 +1,61 @@ +import time +import pytest +import logging +from helpers.cluster import ClickHouseCluster +from tests.integration.helpers.test_tools import TSV + +cluster = ClickHouseCluster(__file__) + +node1 = cluster.add_instance( + "node1", + with_zookeeper=True, + main_configs=[ + "configs_keeper_server/remote_servers.xml", + "configs_keeper_server/enable_keeper1.xml", + "configs_keeper_server/use_keeper.xml", + ], + macros={"replica": "node1"}, +) + +node2 = cluster.add_instance( + "node2", + with_zookeeper=True, + main_configs=[ + "configs_keeper_server/remote_servers.xml", + "configs_keeper_server/enable_keeper2.xml", + ], + macros={"replica": "node2"}, +) + + +@pytest.fixture(scope="module", autouse=True) +def started_cluster(): + try: + cluster.start() + yield cluster + + finally: + cluster.shutdown() + + +def test_create_insert(started_cluster): + node1.query("DROP TABLE IF EXISTS tbl ON CLUSTER 'test_cluster' NO DELAY") + node1.query( + """ + CREATE TABLE tbl ON CLUSTER 'test_cluster' ( + id Int64, + str String + ) ENGINE=ReplicatedMergeTree('/clickhouse/tables/tbl/', '{replica}') + ORDER BY id + """ + ) + + node1.query("INSERT INTO tbl VALUES (1, 'str1')") + node2.query("INSERT INTO tbl VALUES (1, 'str1')") # Test deduplication + node2.query("INSERT INTO tbl VALUES (2, 'str2')") + + expected = [[1, "str1"], [2, "str2"]] + assert node1.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) + assert node2.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) + assert node1.query("CHECK TABLE tbl") == "1\n" + assert node2.query("CHECK TABLE tbl") == "1\n" From 84f33a3114a9feb2421d27aa100f6f528534a317 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Thu, 29 Dec 2022 14:56:09 +0800 Subject: [PATCH 13/52] commit again --- tests/integration/test_alternative_keeper_config/test.py | 4 ++-- .../test_alternative_keeper_config/test_keeper_server.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_alternative_keeper_config/test.py b/tests/integration/test_alternative_keeper_config/test.py index 86877e677ba..f2afc9c77ca 100644 --- a/tests/integration/test_alternative_keeper_config/test.py +++ b/tests/integration/test_alternative_keeper_config/test.py @@ -1,6 +1,6 @@ -import time +#!/usr/bin/env python3 + import pytest -import logging from helpers.cluster import ClickHouseCluster from tests.integration.helpers.test_tools import TSV diff --git a/tests/integration/test_alternative_keeper_config/test_keeper_server.py b/tests/integration/test_alternative_keeper_config/test_keeper_server.py index 7311b32e0af..711be4c7136 100644 --- a/tests/integration/test_alternative_keeper_config/test_keeper_server.py +++ b/tests/integration/test_alternative_keeper_config/test_keeper_server.py @@ -1,6 +1,6 @@ -import time +#!/usr/bin/env python3 + import pytest -import logging from helpers.cluster import ClickHouseCluster from tests.integration.helpers.test_tools import TSV From d5dcf9377599589a41d010c8e523a3f11fb82d32 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Thu, 29 Dec 2022 16:37:20 +0800 Subject: [PATCH 14/52] fix import erorr --- tests/integration/test_alternative_keeper_config/test.py | 2 +- .../test_alternative_keeper_config/test_keeper_server.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_alternative_keeper_config/test.py b/tests/integration/test_alternative_keeper_config/test.py index f2afc9c77ca..8784cb9be56 100644 --- a/tests/integration/test_alternative_keeper_config/test.py +++ b/tests/integration/test_alternative_keeper_config/test.py @@ -2,7 +2,7 @@ import pytest from helpers.cluster import ClickHouseCluster -from tests.integration.helpers.test_tools import TSV +from helpers.test_tools import TSV cluster = ClickHouseCluster(__file__) diff --git a/tests/integration/test_alternative_keeper_config/test_keeper_server.py b/tests/integration/test_alternative_keeper_config/test_keeper_server.py index 711be4c7136..9c61e076671 100644 --- a/tests/integration/test_alternative_keeper_config/test_keeper_server.py +++ b/tests/integration/test_alternative_keeper_config/test_keeper_server.py @@ -2,7 +2,7 @@ import pytest from helpers.cluster import ClickHouseCluster -from tests.integration.helpers.test_tools import TSV +from helpers.test_tools import TSV cluster = ClickHouseCluster(__file__) From 5bc21538e516f01c634f51330639d8721b41c9af Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Thu, 9 Mar 2023 10:31:55 +0000 Subject: [PATCH 15/52] Enable use_environment_credentials by default --- src/Backups/BackupIO_S3.cpp | 2 +- src/Coordination/KeeperSnapshotManagerS3.cpp | 2 +- src/Disks/ObjectStorages/S3/diskSettings.cpp | 2 +- src/Storages/StorageS3.cpp | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Backups/BackupIO_S3.cpp b/src/Backups/BackupIO_S3.cpp index 2f315e8d488..2be5484262a 100644 --- a/src/Backups/BackupIO_S3.cpp +++ b/src/Backups/BackupIO_S3.cpp @@ -65,7 +65,7 @@ namespace settings.auth_settings.server_side_encryption_customer_key_base64, std::move(headers), settings.auth_settings.use_environment_credentials.value_or( - context->getConfigRef().getBool("s3.use_environment_credentials", false)), + context->getConfigRef().getBool("s3.use_environment_credentials", true)), settings.auth_settings.use_insecure_imds_request.value_or( context->getConfigRef().getBool("s3.use_insecure_imds_request", false))); } diff --git a/src/Coordination/KeeperSnapshotManagerS3.cpp b/src/Coordination/KeeperSnapshotManagerS3.cpp index 7b47324a890..108578f03a7 100644 --- a/src/Coordination/KeeperSnapshotManagerS3.cpp +++ b/src/Coordination/KeeperSnapshotManagerS3.cpp @@ -102,7 +102,7 @@ void KeeperSnapshotManagerS3::updateS3Configuration(const Poco::Util::AbstractCo credentials.GetAWSSecretKey(), auth_settings.server_side_encryption_customer_key_base64, std::move(headers), - auth_settings.use_environment_credentials.value_or(false), + auth_settings.use_environment_credentials.value_or(true), auth_settings.use_insecure_imds_request.value_or(false)); auto new_client = std::make_shared(std::move(new_uri), std::move(auth_settings), std::move(client)); diff --git a/src/Disks/ObjectStorages/S3/diskSettings.cpp b/src/Disks/ObjectStorages/S3/diskSettings.cpp index e0e4735f519..0fd04fdcc69 100644 --- a/src/Disks/ObjectStorages/S3/diskSettings.cpp +++ b/src/Disks/ObjectStorages/S3/diskSettings.cpp @@ -151,7 +151,7 @@ std::unique_ptr getClient( config.getString(config_prefix + ".secret_access_key", ""), config.getString(config_prefix + ".server_side_encryption_customer_key_base64", ""), {}, - config.getBool(config_prefix + ".use_environment_credentials", config.getBool("s3.use_environment_credentials", false)), + config.getBool(config_prefix + ".use_environment_credentials", config.getBool("s3.use_environment_credentials", true)), config.getBool(config_prefix + ".use_insecure_imds_request", config.getBool("s3.use_insecure_imds_request", false))); } diff --git a/src/Storages/StorageS3.cpp b/src/Storages/StorageS3.cpp index ed290c38c1f..e29c5e17cc1 100644 --- a/src/Storages/StorageS3.cpp +++ b/src/Storages/StorageS3.cpp @@ -1265,7 +1265,7 @@ void StorageS3::updateConfiguration(ContextPtr ctx, StorageS3::Configuration & u credentials.GetAWSSecretKey(), upd.auth_settings.server_side_encryption_customer_key_base64, std::move(headers), - upd.auth_settings.use_environment_credentials.value_or(ctx->getConfigRef().getBool("s3.use_environment_credentials", false)), + upd.auth_settings.use_environment_credentials.value_or(ctx->getConfigRef().getBool("s3.use_environment_credentials", true)), upd.auth_settings.use_insecure_imds_request.value_or(ctx->getConfigRef().getBool("s3.use_insecure_imds_request", false))); } @@ -1281,7 +1281,7 @@ void StorageS3::processNamedCollectionResult(StorageS3::Configuration & configur configuration.auth_settings.access_key_id = collection.getOrDefault("access_key_id", ""); configuration.auth_settings.secret_access_key = collection.getOrDefault("secret_access_key", ""); - configuration.auth_settings.use_environment_credentials = collection.getOrDefault("use_environment_credentials", 0); + configuration.auth_settings.use_environment_credentials = collection.getOrDefault("use_environment_credentials", 1); configuration.format = collection.getOrDefault("format", "auto"); configuration.compression_method = collection.getOrDefault("compression_method", collection.getOrDefault("compression", "auto")); From a4d1cd514d3ffd6f109dab4d9184b47d01ea8fa9 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Thu, 23 Mar 2023 08:58:56 +0000 Subject: [PATCH 16/52] Refactor --- programs/copier/ClusterCopierApp.cpp | 9 +-- .../extract-from-config/ExtractFromConfig.cpp | 11 +--- programs/server/Server.cpp | 8 +-- src/Common/ZooKeeper/ZooKeeper.cpp | 24 +++++-- src/Common/ZooKeeper/ZooKeeper.h | 6 +- .../examples/zk_many_watches_reconnect.cpp | 66 ------------------- src/Interpreters/Context.cpp | 8 +-- ...get_abandonable_lock_in_all_partitions.cpp | 2 +- .../get_current_inserts_in_replicated.cpp | 2 +- 9 files changed, 37 insertions(+), 99 deletions(-) delete mode 100644 src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp diff --git a/programs/copier/ClusterCopierApp.cpp b/programs/copier/ClusterCopierApp.cpp index 297648280aa..822289dd89c 100644 --- a/programs/copier/ClusterCopierApp.cpp +++ b/programs/copier/ClusterCopierApp.cpp @@ -1,4 +1,5 @@ #include "ClusterCopierApp.h" +#include #include #include #include @@ -12,11 +13,6 @@ namespace fs = std::filesystem; namespace DB { -namespace ErrorCodes -{ - extern const int EXCESSIVE_ELEMENT_IN_CONFIG; -} - /// ClusterCopierApp void ClusterCopierApp::initialize(Poco::Util::Application & self) @@ -197,8 +193,7 @@ void ClusterCopierApp::mainImpl() if (!task_file.empty()) copier->uploadTaskDescription(task_path, task_file, config().getBool("task-upload-force", false)); - if (config().has("zookeeper") && config().has("keeper")) - throw Exception("Both ZooKeeper and Keeper are specified", ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG); + zkutil::validateZooKeeperConfig(config()); copier->init(); copier->process(ConnectionTimeouts::getTCPTimeoutsWithoutFailover(context->getSettingsRef())); diff --git a/programs/extract-from-config/ExtractFromConfig.cpp b/programs/extract-from-config/ExtractFromConfig.cpp index 75b0d311fdb..5305c61b730 100644 --- a/programs/extract-from-config/ExtractFromConfig.cpp +++ b/programs/extract-from-config/ExtractFromConfig.cpp @@ -20,11 +20,6 @@ #include -namespace DB::ErrorCodes -{ - extern const int EXCESSIVE_ELEMENT_IN_CONFIG; -} - static void setupLogging(const std::string & log_level) { Poco::AutoPtr channel(new Poco::ConsoleChannel); @@ -95,11 +90,9 @@ static std::vector extractFromConfig( { DB::ConfigurationPtr bootstrap_configuration(new Poco::Util::XMLConfiguration(config_xml)); - if (bootstrap_configuration->has("zookeeper") && bootstrap_configuration->has("keeper")) - throw DB::Exception(DB::ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG, "Both ZooKeeper and Keeper are specified"); + zkutil::validateZooKeeperConfig(*bootstrap_configuration); - zkutil::ZooKeeperPtr zookeeper; - zookeeper = std::make_shared( + zkutil::ZooKeeperPtr zookeeper = std::make_shared( *bootstrap_configuration, bootstrap_configuration->has("zookeeper") ? "zookeeper" : "keeper", nullptr); zkutil::ZooKeeperNodeCache zk_node_cache([&] { return zookeeper; }); diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 79c14dac0a9..802bee7fad5 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -815,10 +815,8 @@ try } ); - if (config().has("zookeeper") && config().has("keeper")) - throw Exception(ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG, "Both ZooKeeper and Keeper are specified"); - - bool has_zookeeper = config().has("zookeeper") || config().has("keeper") || config().has("keeper_server"); + zkutil::validateZooKeeperConfig(config()); + bool has_zookeeper = zkutil::hasZooKeeperConfig(config()); zkutil::ZooKeeperNodeCache main_config_zk_node_cache([&] { return global_context->getZooKeeper(); }); zkutil::EventPtr main_config_zk_changed_event = std::make_shared(); @@ -1310,7 +1308,7 @@ try { /// We do not load ZooKeeper configuration on the first config loading /// because TestKeeper server is not started yet. - if (config->has("zookeeper") || config->has("keeper") || config->has("keeper_server")) + if (zkutil::hasZooKeeperConfig(config)) global_context->reloadZooKeeperIfChanged(config); global_context->reloadAuxiliaryZooKeepersConfigIfChanged(config); diff --git a/src/Common/ZooKeeper/ZooKeeper.cpp b/src/Common/ZooKeeper/ZooKeeper.cpp index bda1b168a14..f9d851f9697 100644 --- a/src/Common/ZooKeeper/ZooKeeper.cpp +++ b/src/Common/ZooKeeper/ZooKeeper.cpp @@ -30,6 +30,7 @@ namespace ErrorCodes extern const int NOT_IMPLEMENTED; extern const int BAD_ARGUMENTS; extern const int NO_ELEMENTS_IN_CONFIG; + extern const int EXCESSIVE_ELEMENT_IN_CONFIG; } } @@ -1335,16 +1336,29 @@ String getSequentialNodeName(const String & prefix, UInt64 number) return name; } -String getZookeeperConfigName(const Poco::Util::AbstractConfiguration & config) +void validateZooKeeperConfig(const Poco::Util::AbstractConfiguration & config) +{ + if (config.has("zookeeper") && config.has("keeper")) + throw DB::Exception(DB::ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG, "Both ZooKeeper and Keeper are specified"); +} + +bool hasZooKeeperConfig(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server) +{ + return config.has("zookeeper") || config.has("keeper") || (allow_keeper_server && config.has("keeper_server")); +} + +String getZooKeeperConfigName(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server) { if (config.has("zookeeper")) return "zookeeper"; - else if (config.has("keeper")) + + if (config.has("keeper")) return "keeper"; - else if (config.has("keeper_server")) + + if (allow_keeper_server && config.has("keeper_server")) return "keeper_server"; - else - throw DB::Exception("There is no Zookeeper configuration in server config", DB::ErrorCodes::NO_ELEMENTS_IN_CONFIG); + + throw DB::Exception(DB::ErrorCodes::NO_ELEMENTS_IN_CONFIG, "There is no Zookeeper configuration in server config"); } } diff --git a/src/Common/ZooKeeper/ZooKeeper.h b/src/Common/ZooKeeper/ZooKeeper.h index d02fbbedd86..8776497a41d 100644 --- a/src/Common/ZooKeeper/ZooKeeper.h +++ b/src/Common/ZooKeeper/ZooKeeper.h @@ -667,6 +667,10 @@ String extractZooKeeperPath(const String & path, bool check_starts_with_slash, P String getSequentialNodeName(const String & prefix, UInt64 number); -String getZookeeperConfigName(const Poco::Util::AbstractConfiguration & config); +void validateZooKeeperConfig(const Poco::Util::AbstractConfiguration & config); + +bool hasZooKeeperConfig(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server = true); + +String getZooKeeperConfigName(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server = true); } diff --git a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp b/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp deleted file mode 100644 index aad8913ca8b..00000000000 --- a/src/Common/ZooKeeper/examples/zk_many_watches_reconnect.cpp +++ /dev/null @@ -1,66 +0,0 @@ -#include -#include -#include -#include - -/// A tool for reproducing https://issues.apache.org/jira/browse/ZOOKEEPER-706 -/// Original libzookeeper can't reconnect the session if the length of SET_WATCHES message -/// exceeds jute.maxbuffer (0xfffff by default). -/// This happens when the number of watches exceeds ~29000. -/// -/// Session reconnect can be caused by forbidding packets to the current zookeeper server, e.g. -/// sudo ip6tables -A OUTPUT -d mtzoo01it.haze.yandex.net -j REJECT - -const size_t N_THREADS = 100; - -int main(int argc, char ** argv) -{ - try - { - if (argc != 3) - { - std::cerr << "usage: " << argv[0] << " " << std::endl; - return 3; - } - - DB::ConfigProcessor processor(argv[1], false, true); - auto config = processor.loadConfig().configuration; - zkutil::ZooKeeper zk(*config, zkutil::getZookeeperConfigName(*config), nullptr); - zkutil::EventPtr watch = std::make_shared(); - - /// NOTE: setting watches in multiple threads because doing it in a single thread is too slow. - size_t watches_per_thread = std::stoull(argv[2]) / N_THREADS; - std::vector threads; - for (size_t i_thread = 0; i_thread < N_THREADS; ++i_thread) - { - threads.emplace_back([&, i_thread] - { - for (size_t i = 0; i < watches_per_thread; ++i) - zk.exists("/clickhouse/nonexistent_node" + std::to_string(i * N_THREADS + i_thread), nullptr, watch); - }); - } - for (size_t i_thread = 0; i_thread < N_THREADS; ++i_thread) - threads[i_thread].join(); - - while (true) - { - std::cerr << "WAITING..." << std::endl; - sleep(10); - } - } - catch (Poco::Exception & e) - { - std::cerr << "Exception: " << e.displayText() << std::endl; - return 1; - } - catch (std::exception & e) - { - std::cerr << "std::exception: " << e.what() << std::endl; - return 3; - } - catch (...) - { - std::cerr << "Some exception" << std::endl; - return 2; - } -} diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 9c133a60ea6..e51a831684f 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -2360,7 +2360,7 @@ zkutil::ZooKeeperPtr Context::getZooKeeper() const const auto & config = shared->zookeeper_config ? *shared->zookeeper_config : getConfigRef(); if (!shared->zookeeper) - shared->zookeeper = std::make_shared(config, zkutil::getZookeeperConfigName(config), getZooKeeperLog()); + shared->zookeeper = std::make_shared(config, zkutil::getZooKeeperConfigName(config), getZooKeeperLog()); else if (shared->zookeeper->expired()) { Stopwatch watch; @@ -2399,9 +2399,9 @@ bool Context::tryCheckClientConnectionToMyKeeperCluster() const { try { + const auto config_name = zkutil::getZooKeeperConfigName(getConfigRef()); /// If our server is part of main Keeper cluster - if (checkZooKeeperConfigIsLocal(getConfigRef(), "zookeeper") || checkZooKeeperConfigIsLocal(getConfigRef(), "keeper") - || (!getConfigRef().has("zookeeper") && !getConfigRef().has("keeper") && getConfigRef().has("keeper_server"))) + if (config_name == "keeper_server" || checkZooKeeperConfigIsLocal(getConfigRef(), config_name)) { LOG_DEBUG(shared->log, "Keeper server is participant of the main zookeeper cluster, will try to connect to it"); getZooKeeper(); @@ -2600,7 +2600,7 @@ void Context::reloadZooKeeperIfChanged(const ConfigurationPtr & config) const { std::lock_guard lock(shared->zookeeper_mutex); shared->zookeeper_config = config; - reloadZooKeeperIfChangedImpl(config, zkutil::getZookeeperConfigName(*config), shared->zookeeper, getZooKeeperLog()); + reloadZooKeeperIfChangedImpl(config, zkutil::getZooKeeperConfigName(*config), shared->zookeeper, getZooKeeperLog()); } void Context::reloadAuxiliaryZooKeepersConfigIfChanged(const ConfigurationPtr & config) diff --git a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp index e1faa67eb45..9e2b2a83b98 100644 --- a/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp +++ b/src/Storages/examples/get_abandonable_lock_in_all_partitions.cpp @@ -26,7 +26,7 @@ try auto config = processor.loadConfig().configuration; String root_path = argv[2]; - zkutil::ZooKeeper zk(*config, zkutil::getZookeeperConfigName(*config), nullptr); + zkutil::ZooKeeper zk(*config, zkutil::getZooKeeperConfigName(*config), nullptr); String temp_path = root_path + "/temp"; String blocks_path = root_path + "/block_numbers"; diff --git a/src/Storages/examples/get_current_inserts_in_replicated.cpp b/src/Storages/examples/get_current_inserts_in_replicated.cpp index 9ba75b81440..d77b0f5177d 100644 --- a/src/Storages/examples/get_current_inserts_in_replicated.cpp +++ b/src/Storages/examples/get_current_inserts_in_replicated.cpp @@ -29,7 +29,7 @@ try auto config = processor.loadConfig().configuration; String zookeeper_path = argv[2]; - auto zookeeper = std::make_shared(*config, zkutil::getZookeeperConfigName(*config), nullptr); + auto zookeeper = std::make_shared(*config, zkutil::getZooKeeperConfigName(*config), nullptr); std::unordered_map> current_inserts; From d6cbc5d05b34f630f64445105632ccb8b2429470 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Thu, 23 Mar 2023 12:58:39 +0000 Subject: [PATCH 17/52] Fix tests --- programs/server/Server.cpp | 2 +- src/Common/ZooKeeper/ZooKeeper.cpp | 8 +- src/Common/ZooKeeper/ZooKeeper.h | 4 +- src/Common/ZooKeeper/ZooKeeperArgs.cpp | 98 ++++++++++++------- src/Common/ZooKeeper/ZooKeeperArgs.h | 2 +- src/Interpreters/Context.cpp | 2 +- .../helpers/zookeeper_secure_config.xml | 4 +- .../enable_keeper1.xml | 8 ++ .../enable_keeper2.xml | 8 ++ .../configs/enable_keeper3.xml | 41 ++++++++ .../configs/keeper_config.xml | 12 +-- .../configs/remote_servers.xml | 6 +- .../configs/zookeeper_config.xml | 12 +-- .../configs_keeper_server/remote_servers.xml | 18 ---- .../configs_keeper_server/use_keeper.xml | 12 --- .../test_alternative_keeper_config/test.py | 31 ++++-- .../test_keeper_server.py | 61 ------------ .../configs/enable_keeper1.xml | 6 +- .../configs/enable_keeper2.xml | 6 +- .../configs/enable_keeper3.xml | 6 +- .../configs/keeper_config_with_allow_list.xml | 4 +- .../keeper_config_with_allow_list_all.xml | 4 +- .../keeper_config_without_allow_list.xml | 4 +- .../test_keeper_four_word_command/test.py | 9 +- .../configs/enable_keeper1.xml | 2 + .../configs/enable_keeper_three_nodes_1.xml | 2 + .../configs/enable_keeper_three_nodes_2.xml | 2 + .../configs/enable_keeper_three_nodes_3.xml | 2 + .../configs/enable_keeper_two_nodes_1.xml | 2 + .../configs/enable_keeper_two_nodes_2.xml | 2 + .../configs/keeper_config.xml | 2 + .../configs/zookeeper_config_root_a.xml | 4 +- 32 files changed, 204 insertions(+), 182 deletions(-) rename tests/integration/test_alternative_keeper_config/{configs_keeper_server => configs}/enable_keeper1.xml (79%) rename tests/integration/test_alternative_keeper_config/{configs_keeper_server => configs}/enable_keeper2.xml (79%) create mode 100644 tests/integration/test_alternative_keeper_config/configs/enable_keeper3.xml delete mode 100644 tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml delete mode 100644 tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml delete mode 100644 tests/integration/test_alternative_keeper_config/test_keeper_server.py diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 802bee7fad5..7e120e0fa17 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -1308,7 +1308,7 @@ try { /// We do not load ZooKeeper configuration on the first config loading /// because TestKeeper server is not started yet. - if (zkutil::hasZooKeeperConfig(config)) + if (zkutil::hasZooKeeperConfig(*config)) global_context->reloadZooKeeperIfChanged(config); global_context->reloadAuxiliaryZooKeepersConfigIfChanged(config); diff --git a/src/Common/ZooKeeper/ZooKeeper.cpp b/src/Common/ZooKeeper/ZooKeeper.cpp index f9d851f9697..c9062d9fd4c 100644 --- a/src/Common/ZooKeeper/ZooKeeper.cpp +++ b/src/Common/ZooKeeper/ZooKeeper.cpp @@ -1342,12 +1342,12 @@ void validateZooKeeperConfig(const Poco::Util::AbstractConfiguration & config) throw DB::Exception(DB::ErrorCodes::EXCESSIVE_ELEMENT_IN_CONFIG, "Both ZooKeeper and Keeper are specified"); } -bool hasZooKeeperConfig(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server) +bool hasZooKeeperConfig(const Poco::Util::AbstractConfiguration & config) { - return config.has("zookeeper") || config.has("keeper") || (allow_keeper_server && config.has("keeper_server")); + return config.has("zookeeper") || config.has("keeper") || (config.has("keeper_server") && config.getBool("keeper_server.use_cluster", true)); } -String getZooKeeperConfigName(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server) +String getZooKeeperConfigName(const Poco::Util::AbstractConfiguration & config) { if (config.has("zookeeper")) return "zookeeper"; @@ -1355,7 +1355,7 @@ String getZooKeeperConfigName(const Poco::Util::AbstractConfiguration & config, if (config.has("keeper")) return "keeper"; - if (allow_keeper_server && config.has("keeper_server")) + if (config.has("keeper_server") && config.getBool("keeper_server.use_cluster", true)) return "keeper_server"; throw DB::Exception(DB::ErrorCodes::NO_ELEMENTS_IN_CONFIG, "There is no Zookeeper configuration in server config"); diff --git a/src/Common/ZooKeeper/ZooKeeper.h b/src/Common/ZooKeeper/ZooKeeper.h index 8776497a41d..41f8d110964 100644 --- a/src/Common/ZooKeeper/ZooKeeper.h +++ b/src/Common/ZooKeeper/ZooKeeper.h @@ -669,8 +669,8 @@ String getSequentialNodeName(const String & prefix, UInt64 number); void validateZooKeeperConfig(const Poco::Util::AbstractConfiguration & config); -bool hasZooKeeperConfig(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server = true); +bool hasZooKeeperConfig(const Poco::Util::AbstractConfiguration & config); -String getZooKeeperConfigName(const Poco::Util::AbstractConfiguration & config, bool allow_keeper_server = true); +String getZooKeeperConfigName(const Poco::Util::AbstractConfiguration & config); } diff --git a/src/Common/ZooKeeper/ZooKeeperArgs.cpp b/src/Common/ZooKeeper/ZooKeeperArgs.cpp index 3d6d75ba5d5..7cd85db931b 100644 --- a/src/Common/ZooKeeper/ZooKeeperArgs.cpp +++ b/src/Common/ZooKeeper/ZooKeeperArgs.cpp @@ -19,8 +19,8 @@ namespace zkutil ZooKeeperArgs::ZooKeeperArgs(const Poco::Util::AbstractConfiguration & config, const String & config_name) { - if (endsWith(config_name, "keeper_server")) - initFromKeeperServerSection(config, config_name); + if (config_name == "keeper_server") + initFromKeeperServerSection(config); else initFromKeeperSection(config, config_name); @@ -52,49 +52,79 @@ ZooKeeperArgs::ZooKeeperArgs(const String & hosts_string) splitInto<','>(hosts, hosts_string); } -void ZooKeeperArgs::initFromKeeperServerSection(const Poco::Util::AbstractConfiguration & config, const std::string & config_name) +void ZooKeeperArgs::initFromKeeperServerSection(const Poco::Util::AbstractConfiguration & config) { - Poco::Util::AbstractConfiguration::Keys keys; - config.keys(config_name, keys); + static constexpr std::string_view config_name = "keeper_server"; - bool secure = false; - String tcp_port; - String tcp_port_secure; - for (const auto & key : keys) + if (auto key = std::string{config_name} + ".tcp_port_secure"; + config.has(key)) { - if (key == "tcp_port_secure") - { - secure = true; - tcp_port_secure = config.getString(config_name + "." + key); - } - else if (key == "tcp_port") - { - tcp_port = config.getString(config_name + "." + key); - } - else if (key == "coordination_settings") - { - if (config.has(config_name + "." + key + ".operation_timeout_ms")) - operation_timeout_ms = config.getInt(config_name + "." + key + ".operation_timeout_ms"); - if (config.has(config_name + "." + key + ".session_timeout_ms")) - session_timeout_ms = config.getInt(config_name + "." + key + ".session_timeout_ms"); - } + auto tcp_port_secure = config.getString(key); + + if (tcp_port_secure.empty()) + throw KeeperException("Empty tcp_port_secure in config file", Coordination::Error::ZBADARGUMENTS); } - if (secure && tcp_port_secure.empty()) - throw KeeperException("No tcp_port_secure in config file", Coordination::Error::ZBADARGUMENTS); - if (!secure && tcp_port.empty()) - throw KeeperException("No tcp_port in config file", Coordination::Error::ZBADARGUMENTS); + bool secure{false}; + std::string tcp_port; + if (auto tcp_port_secure_key = std::string{config_name} + ".tcp_port_secure"; + config.has(tcp_port_secure_key)) + { + secure = true; + tcp_port = config.getString(tcp_port_secure_key); + } + else if (auto tcp_port_key = std::string{config_name} + ".tcp_port"; + config.has(tcp_port_key)) + { + tcp_port = config.getString(tcp_port_key); + } - config.keys(config_name + ".raft_configuration", keys); + if (tcp_port.empty()) + throw KeeperException("No tcp_port or tcp_port_secure in config file", Coordination::Error::ZBADARGUMENTS); + + if (auto coordination_key = std::string{config_name} + ".coordination_settings"; + config.has(coordination_key)) + { + if (auto operation_timeout_key = coordination_key + ".operation_timeout_ms"; + config.has(operation_timeout_key)) + operation_timeout_ms = config.getInt(operation_timeout_key); + + if (auto session_timeout_key = coordination_key + ".session_timeout_ms"; + config.has(session_timeout_key)) + session_timeout_key = config.getInt(session_timeout_key); + } + + Poco::Util::AbstractConfiguration::Keys keys; + std::string raft_configuration_key = std::string{config_name} + ".raft_configuration"; + config.keys(raft_configuration_key, keys); for (const auto & key : keys) { if (startsWith(key, "server")) - { hosts.push_back( - (secure ? "secure://" : "") + config.getString(config_name + ".raft_configuration." + key + ".hostname") + ":" - + (secure ? tcp_port_secure : tcp_port)); + (secure ? "secure://" : "") + config.getString(raft_configuration_key + "." + key + ".hostname") + ":" + tcp_port); + } + + static constexpr std::array load_balancing_keys + { + ".zookeeper_load_balancing", + ".keeper_load_balancing" + }; + + for (const auto * load_balancing_key : load_balancing_keys) + { + if (auto load_balancing_config = std::string{config_name} + load_balancing_key; + config.has(load_balancing_config)) + { + String load_balancing_str = config.getString(load_balancing_config); + /// Use magic_enum to avoid dependency from dbms (`SettingFieldLoadBalancingTraits::fromString(...)`) + auto load_balancing = magic_enum::enum_cast(Poco::toUpper(load_balancing_str)); + if (!load_balancing) + throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Unknown load balancing: {}", load_balancing_str); + get_priority_load_balancing.load_balancing = *load_balancing; + break; } } + } void ZooKeeperArgs::initFromKeeperSection(const Poco::Util::AbstractConfiguration & config, const std::string & config_name) @@ -144,7 +174,7 @@ void ZooKeeperArgs::initFromKeeperSection(const Poco::Util::AbstractConfiguratio { implementation = config.getString(config_name + "." + key); } - else if (key == "zookeeper_load_balancing") + else if (key == "zookeeper_load_balancing" || key == "keeper_load_balancing") { String load_balancing_str = config.getString(config_name + "." + key); /// Use magic_enum to avoid dependency from dbms (`SettingFieldLoadBalancingTraits::fromString(...)`) diff --git a/src/Common/ZooKeeper/ZooKeeperArgs.h b/src/Common/ZooKeeper/ZooKeeperArgs.h index fa97f8b860f..3f33721804f 100644 --- a/src/Common/ZooKeeper/ZooKeeperArgs.h +++ b/src/Common/ZooKeeper/ZooKeeperArgs.h @@ -34,7 +34,7 @@ struct ZooKeeperArgs DB::GetPriorityForLoadBalancing get_priority_load_balancing; private: - void initFromKeeperServerSection(const Poco::Util::AbstractConfiguration & config, const std::string & config_name); + void initFromKeeperServerSection(const Poco::Util::AbstractConfiguration & config); void initFromKeeperSection(const Poco::Util::AbstractConfiguration & config, const std::string & config_name); }; diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index e51a831684f..107adc3ac57 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -2624,7 +2624,7 @@ void Context::reloadAuxiliaryZooKeepersConfigIfChanged(const ConfigurationPtr & bool Context::hasZooKeeper() const { - return getConfigRef().has("zookeeper") || getConfigRef().has("keeper") || getConfigRef().has("keeper_server"); + return zkutil::hasZooKeeperConfig(getConfigRef()); } bool Context::hasAuxiliaryZooKeeper(const String & name) const diff --git a/tests/integration/helpers/zookeeper_secure_config.xml b/tests/integration/helpers/zookeeper_secure_config.xml index 349bcbc5eba..3e3706988ab 100644 --- a/tests/integration/helpers/zookeeper_secure_config.xml +++ b/tests/integration/helpers/zookeeper_secure_config.xml @@ -1,5 +1,5 @@ - + zoo1 2281 @@ -13,5 +13,5 @@ 2281 15000 - + diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper1.xml b/tests/integration/test_alternative_keeper_config/configs/enable_keeper1.xml similarity index 79% rename from tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper1.xml rename to tests/integration/test_alternative_keeper_config/configs/enable_keeper1.xml index 7c2e283e89f..fbdece06085 100644 --- a/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper1.xml +++ b/tests/integration/test_alternative_keeper_config/configs/enable_keeper1.xml @@ -28,6 +28,14 @@ true 2 + + 3 + node3 + 9234 + true + true + 3 + diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper2.xml b/tests/integration/test_alternative_keeper_config/configs/enable_keeper2.xml similarity index 79% rename from tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper2.xml rename to tests/integration/test_alternative_keeper_config/configs/enable_keeper2.xml index 618e6a04aec..dc3ce6c30c4 100644 --- a/tests/integration/test_alternative_keeper_config/configs_keeper_server/enable_keeper2.xml +++ b/tests/integration/test_alternative_keeper_config/configs/enable_keeper2.xml @@ -28,6 +28,14 @@ true 2 + + 3 + node3 + 9234 + true + true + 3 + diff --git a/tests/integration/test_alternative_keeper_config/configs/enable_keeper3.xml b/tests/integration/test_alternative_keeper_config/configs/enable_keeper3.xml new file mode 100644 index 00000000000..af2566565e4 --- /dev/null +++ b/tests/integration/test_alternative_keeper_config/configs/enable_keeper3.xml @@ -0,0 +1,41 @@ + + + 9181 + 3 + /var/lib/clickhouse/coordination/log + /var/lib/clickhouse/coordination/snapshots + + + 5000 + 10000 + 75 + trace + + + + + 1 + node1 + 9234 + true + 3 + + + 2 + node2 + 9234 + true + true + 2 + + + 3 + node3 + 9234 + true + true + 3 + + + + diff --git a/tests/integration/test_alternative_keeper_config/configs/keeper_config.xml b/tests/integration/test_alternative_keeper_config/configs/keeper_config.xml index bd783b83853..b62e2728085 100644 --- a/tests/integration/test_alternative_keeper_config/configs/keeper_config.xml +++ b/tests/integration/test_alternative_keeper_config/configs/keeper_config.xml @@ -1,16 +1,16 @@ - zoo1 - 2181 + node1 + 9181 - zoo2 - 2181 + node2 + 9181 - zoo3 - 2181 + node3 + 9181 3000 diff --git a/tests/integration/test_alternative_keeper_config/configs/remote_servers.xml b/tests/integration/test_alternative_keeper_config/configs/remote_servers.xml index e77cc5c65e6..5b453fdeb67 100644 --- a/tests/integration/test_alternative_keeper_config/configs/remote_servers.xml +++ b/tests/integration/test_alternative_keeper_config/configs/remote_servers.xml @@ -6,12 +6,14 @@ node1 9000 - node2 9000 - + + node3 + 9000 + diff --git a/tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml b/tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml index 7a0d7c1de92..31913bb6e2c 100644 --- a/tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml +++ b/tests/integration/test_alternative_keeper_config/configs/zookeeper_config.xml @@ -1,16 +1,16 @@ - zoo1 - 2181 + node1 + 9181 - zoo2 - 2181 + node2 + 9181 - zoo3 - 2181 + node3 + 9181 3000 diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml b/tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml deleted file mode 100644 index e77cc5c65e6..00000000000 --- a/tests/integration/test_alternative_keeper_config/configs_keeper_server/remote_servers.xml +++ /dev/null @@ -1,18 +0,0 @@ - - - - - - node1 - 9000 - - - - node2 - 9000 - - - - - - diff --git a/tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml b/tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml deleted file mode 100644 index b250f06cf81..00000000000 --- a/tests/integration/test_alternative_keeper_config/configs_keeper_server/use_keeper.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - node1 - 9181 - - - node2 - 9181 - - - diff --git a/tests/integration/test_alternative_keeper_config/test.py b/tests/integration/test_alternative_keeper_config/test.py index 8784cb9be56..d2cfc4fe25e 100644 --- a/tests/integration/test_alternative_keeper_config/test.py +++ b/tests/integration/test_alternative_keeper_config/test.py @@ -8,18 +8,30 @@ cluster = ClickHouseCluster(__file__) node1 = cluster.add_instance( "node1", - with_zookeeper=True, - main_configs=["configs/remote_servers.xml", "configs/keeper_config.xml"], + main_configs=[ + "configs/remote_servers.xml", + "configs/keeper_config.xml", + "configs/enable_keeper1.xml", + ], macros={"replica": "node1"}, ) node2 = cluster.add_instance( "node2", - with_zookeeper=True, - main_configs=["configs/remote_servers.xml", "configs/zookeeper_config.xml"], + main_configs=[ + "configs/remote_servers.xml", + "configs/zookeeper_config.xml", + "configs/enable_keeper2.xml", + ], macros={"replica": "node2"}, ) +node3 = cluster.add_instance( + "node3", + main_configs=["configs/remote_servers.xml", "configs/enable_keeper3.xml"], + macros={"replica": "node3"}, +) + @pytest.fixture(scope="module", autouse=True) def started_cluster(): @@ -45,10 +57,9 @@ def test_create_insert(started_cluster): node1.query("INSERT INTO tbl VALUES (1, 'str1')") node2.query("INSERT INTO tbl VALUES (1, 'str1')") # Test deduplication - node2.query("INSERT INTO tbl VALUES (2, 'str2')") + node3.query("INSERT INTO tbl VALUES (2, 'str2')") - expected = [[1, "str1"], [2, "str2"]] - assert node1.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) - assert node2.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) - assert node1.query("CHECK TABLE tbl") == "1\n" - assert node2.query("CHECK TABLE tbl") == "1\n" + for node in [node1, node2, node3]: + expected = [[1, "str1"], [2, "str2"]] + assert node.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) + assert node.query("CHECK TABLE tbl") == "1\n" diff --git a/tests/integration/test_alternative_keeper_config/test_keeper_server.py b/tests/integration/test_alternative_keeper_config/test_keeper_server.py deleted file mode 100644 index 9c61e076671..00000000000 --- a/tests/integration/test_alternative_keeper_config/test_keeper_server.py +++ /dev/null @@ -1,61 +0,0 @@ -#!/usr/bin/env python3 - -import pytest -from helpers.cluster import ClickHouseCluster -from helpers.test_tools import TSV - -cluster = ClickHouseCluster(__file__) - -node1 = cluster.add_instance( - "node1", - with_zookeeper=True, - main_configs=[ - "configs_keeper_server/remote_servers.xml", - "configs_keeper_server/enable_keeper1.xml", - "configs_keeper_server/use_keeper.xml", - ], - macros={"replica": "node1"}, -) - -node2 = cluster.add_instance( - "node2", - with_zookeeper=True, - main_configs=[ - "configs_keeper_server/remote_servers.xml", - "configs_keeper_server/enable_keeper2.xml", - ], - macros={"replica": "node2"}, -) - - -@pytest.fixture(scope="module", autouse=True) -def started_cluster(): - try: - cluster.start() - yield cluster - - finally: - cluster.shutdown() - - -def test_create_insert(started_cluster): - node1.query("DROP TABLE IF EXISTS tbl ON CLUSTER 'test_cluster' NO DELAY") - node1.query( - """ - CREATE TABLE tbl ON CLUSTER 'test_cluster' ( - id Int64, - str String - ) ENGINE=ReplicatedMergeTree('/clickhouse/tables/tbl/', '{replica}') - ORDER BY id - """ - ) - - node1.query("INSERT INTO tbl VALUES (1, 'str1')") - node2.query("INSERT INTO tbl VALUES (1, 'str1')") # Test deduplication - node2.query("INSERT INTO tbl VALUES (2, 'str2')") - - expected = [[1, "str1"], [2, "str2"]] - assert node1.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) - assert node2.query("SELECT * FROM tbl ORDER BY id") == TSV(expected) - assert node1.query("CHECK TABLE tbl") == "1\n" - assert node2.query("CHECK TABLE tbl") == "1\n" diff --git a/tests/integration/test_keeper_four_word_command/configs/enable_keeper1.xml b/tests/integration/test_keeper_four_word_command/configs/enable_keeper1.xml index a686c96e426..0ec413ac2ec 100644 --- a/tests/integration/test_keeper_four_word_command/configs/enable_keeper1.xml +++ b/tests/integration/test_keeper_four_word_command/configs/enable_keeper1.xml @@ -1,5 +1,7 @@ - + + false + 9181 1 /var/lib/clickhouse/coordination/log @@ -39,4 +41,4 @@ - + diff --git a/tests/integration/test_keeper_four_word_command/configs/enable_keeper2.xml b/tests/integration/test_keeper_four_word_command/configs/enable_keeper2.xml index 9818d32a74a..fde345f67b3 100644 --- a/tests/integration/test_keeper_four_word_command/configs/enable_keeper2.xml +++ b/tests/integration/test_keeper_four_word_command/configs/enable_keeper2.xml @@ -1,5 +1,7 @@ - + + false + 9181 2 /var/lib/clickhouse/coordination/log @@ -39,4 +41,4 @@ - + diff --git a/tests/integration/test_keeper_four_word_command/configs/enable_keeper3.xml b/tests/integration/test_keeper_four_word_command/configs/enable_keeper3.xml index 5a883fac3f6..84a8a402b46 100644 --- a/tests/integration/test_keeper_four_word_command/configs/enable_keeper3.xml +++ b/tests/integration/test_keeper_four_word_command/configs/enable_keeper3.xml @@ -1,5 +1,7 @@ - + + false + 9181 3 /var/lib/clickhouse/coordination/log @@ -39,4 +41,4 @@ - + diff --git a/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list.xml b/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list.xml index feafd3f6b44..b06f845ecf7 100644 --- a/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list.xml +++ b/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list.xml @@ -1,4 +1,4 @@ - + 9181 1 @@ -21,4 +21,4 @@ - + diff --git a/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list_all.xml b/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list_all.xml index 523e6b2fa27..46c2681c581 100644 --- a/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list_all.xml +++ b/tests/integration/test_keeper_four_word_command/configs/keeper_config_with_allow_list_all.xml @@ -1,4 +1,4 @@ - + 9181 3 @@ -21,4 +21,4 @@ - + diff --git a/tests/integration/test_keeper_four_word_command/configs/keeper_config_without_allow_list.xml b/tests/integration/test_keeper_four_word_command/configs/keeper_config_without_allow_list.xml index 891f8a2ec12..cd5dea882af 100644 --- a/tests/integration/test_keeper_four_word_command/configs/keeper_config_without_allow_list.xml +++ b/tests/integration/test_keeper_four_word_command/configs/keeper_config_without_allow_list.xml @@ -1,4 +1,4 @@ - + 9181 2 @@ -20,4 +20,4 @@ - + diff --git a/tests/integration/test_keeper_four_word_command/test.py b/tests/integration/test_keeper_four_word_command/test.py index 412780c8f0f..df333ec479e 100644 --- a/tests/integration/test_keeper_four_word_command/test.py +++ b/tests/integration/test_keeper_four_word_command/test.py @@ -1,14 +1,7 @@ -import socket import pytest from helpers.cluster import ClickHouseCluster import helpers.keeper_utils as keeper_utils -import random -import string -import os import time -from multiprocessing.dummy import Pool -from helpers.test_tools import assert_eq_with_retry -from io import StringIO import csv import re @@ -23,7 +16,7 @@ node3 = cluster.add_instance( "node3", main_configs=["configs/enable_keeper3.xml"], stay_alive=True ) -from kazoo.client import KazooClient, KazooState +from kazoo.client import KazooClient def wait_nodes(): diff --git a/tests/integration/test_keeper_nodes_add/configs/enable_keeper1.xml b/tests/integration/test_keeper_nodes_add/configs/enable_keeper1.xml index c1d38a1de52..03307e912f6 100644 --- a/tests/integration/test_keeper_nodes_add/configs/enable_keeper1.xml +++ b/tests/integration/test_keeper_nodes_add/configs/enable_keeper1.xml @@ -1,5 +1,7 @@ + false + 9181 1 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_1.xml b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_1.xml index d2717283a8d..39a60afffec 100644 --- a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_1.xml +++ b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_1.xml @@ -1,5 +1,7 @@ + false + 9181 1 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_2.xml b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_2.xml index 5924ee1c2dc..0f2d5ff912b 100644 --- a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_2.xml +++ b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_2.xml @@ -1,5 +1,7 @@ + false + 9181 2 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_3.xml b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_3.xml index d261e4f67f3..f5061fe0f36 100644 --- a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_3.xml +++ b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_three_nodes_3.xml @@ -1,5 +1,7 @@ + false + 9181 3 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_1.xml b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_1.xml index 697986638d7..57585080e0f 100644 --- a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_1.xml +++ b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_1.xml @@ -1,5 +1,7 @@ + false + 9181 1 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_2.xml b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_2.xml index 967940e1e2b..35493e22270 100644 --- a/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_2.xml +++ b/tests/integration/test_keeper_nodes_add/configs/enable_keeper_two_nodes_2.xml @@ -1,5 +1,7 @@ + false + 9181 2 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_zookeeper_converter/configs/keeper_config.xml b/tests/integration/test_keeper_zookeeper_converter/configs/keeper_config.xml index 9b50f2c6c41..38ef4295b01 100644 --- a/tests/integration/test_keeper_zookeeper_converter/configs/keeper_config.xml +++ b/tests/integration/test_keeper_zookeeper_converter/configs/keeper_config.xml @@ -1,5 +1,7 @@ + false + 9181 1 /var/lib/clickhouse/coordination/logs diff --git a/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml b/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml index d3c62862002..6c413378524 100644 --- a/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml +++ b/tests/integration/test_zookeeper_config/configs/zookeeper_config_root_a.xml @@ -1,5 +1,5 @@ - + zoo1 2181 @@ -14,5 +14,5 @@ 3000 /root_a - + From d1a6c1991abc073b9320c3d6b529b596d416b2ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 27 Mar 2023 17:38:52 +0200 Subject: [PATCH 18/52] Only check MV on ALTER when necessary --- src/Storages/MergeTree/MergeTreeData.cpp | 7 +++++-- src/Storages/StorageBuffer.cpp | 6 ++++-- src/Storages/StorageDistributed.cpp | 6 ++++-- src/Storages/StorageMerge.cpp | 6 ++++-- src/Storages/StorageNull.cpp | 6 ++++-- .../02697_alter_dependencies.reference | 1 + .../0_stateless/02697_alter_dependencies.sql | 16 ++++++++++++++++ 7 files changed, 38 insertions(+), 10 deletions(-) create mode 100644 tests/queries/0_stateless/02697_alter_dependencies.reference create mode 100644 tests/queries/0_stateless/02697_alter_dependencies.sql diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index acb5ed248c8..52da00ca4a1 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -2930,7 +2930,8 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context old_types.emplace(column.name, column.type.get()); NamesAndTypesList columns_to_check_conversion; - auto name_deps = getDependentViewsByColumn(local_context); + + std::optional name_deps{}; for (const AlterCommand & command : commands) { /// Just validate partition expression @@ -3012,7 +3013,9 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context if (!command.clear) { - const auto & deps_mv = name_deps[command.column_name]; + if (!name_deps) + name_deps = getDependentViewsByColumn(local_context); + const auto & deps_mv = name_deps.value()[command.column_name]; if (!deps_mv.empty()) { throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN, diff --git a/src/Storages/StorageBuffer.cpp b/src/Storages/StorageBuffer.cpp index a69f26203e9..a4cb15d5711 100644 --- a/src/Storages/StorageBuffer.cpp +++ b/src/Storages/StorageBuffer.cpp @@ -1016,7 +1016,7 @@ void StorageBuffer::reschedule() void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, ContextPtr local_context) const { - auto name_deps = getDependentViewsByColumn(local_context); + std::optional name_deps{}; for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -1027,7 +1027,9 @@ void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, Context if (command.type == AlterCommand::Type::DROP_COLUMN && !command.clear) { - const auto & deps_mv = name_deps[command.column_name]; + if (!name_deps) + name_deps = getDependentViewsByColumn(local_context); + const auto & deps_mv = name_deps.value()[command.column_name]; if (!deps_mv.empty()) { throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN, diff --git a/src/Storages/StorageDistributed.cpp b/src/Storages/StorageDistributed.cpp index 0be4ae3a79f..ec2e8fed8ae 100644 --- a/src/Storages/StorageDistributed.cpp +++ b/src/Storages/StorageDistributed.cpp @@ -1394,7 +1394,7 @@ std::optional StorageDistributed::distributedWrite(const ASTInser void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, ContextPtr local_context) const { - auto name_deps = getDependentViewsByColumn(local_context); + std::optional name_deps{}; for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -1406,7 +1406,9 @@ void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, Co if (command.type == AlterCommand::DROP_COLUMN && !command.clear) { - const auto & deps_mv = name_deps[command.column_name]; + if (!name_deps) + name_deps = getDependentViewsByColumn(local_context); + const auto & deps_mv = name_deps.value()[command.column_name]; if (!deps_mv.empty()) { throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN, diff --git a/src/Storages/StorageMerge.cpp b/src/Storages/StorageMerge.cpp index 0f7b47255f6..96c40fc28d4 100644 --- a/src/Storages/StorageMerge.cpp +++ b/src/Storages/StorageMerge.cpp @@ -919,7 +919,7 @@ StorageMerge::DatabaseTablesIterators StorageMerge::getDatabaseIterators(Context void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, ContextPtr local_context) const { - auto name_deps = getDependentViewsByColumn(local_context); + std::optional name_deps{}; for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -930,7 +930,9 @@ void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, ContextP if (command.type == AlterCommand::Type::DROP_COLUMN && !command.clear) { - const auto & deps_mv = name_deps[command.column_name]; + if (!name_deps) + name_deps = getDependentViewsByColumn(local_context); + const auto & deps_mv = name_deps.value()[command.column_name]; if (!deps_mv.empty()) { throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN, diff --git a/src/Storages/StorageNull.cpp b/src/Storages/StorageNull.cpp index aa462e1a40c..0ced128c8ef 100644 --- a/src/Storages/StorageNull.cpp +++ b/src/Storages/StorageNull.cpp @@ -37,7 +37,7 @@ void registerStorageNull(StorageFactory & factory) void StorageNull::checkAlterIsPossible(const AlterCommands & commands, ContextPtr context) const { - auto name_deps = getDependentViewsByColumn(context); + std::optional name_deps{}; for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN @@ -50,7 +50,9 @@ void StorageNull::checkAlterIsPossible(const AlterCommands & commands, ContextPt if (command.type == AlterCommand::DROP_COLUMN && !command.clear) { - const auto & deps_mv = name_deps[command.column_name]; + if (!name_deps) + name_deps = getDependentViewsByColumn(context); + const auto & deps_mv = name_deps.value()[command.column_name]; if (!deps_mv.empty()) { throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN, diff --git a/tests/queries/0_stateless/02697_alter_dependencies.reference b/tests/queries/0_stateless/02697_alter_dependencies.reference new file mode 100644 index 00000000000..d05b1f927f4 --- /dev/null +++ b/tests/queries/0_stateless/02697_alter_dependencies.reference @@ -0,0 +1 @@ +0 0 diff --git a/tests/queries/0_stateless/02697_alter_dependencies.sql b/tests/queries/0_stateless/02697_alter_dependencies.sql new file mode 100644 index 00000000000..cf9b7551b5f --- /dev/null +++ b/tests/queries/0_stateless/02697_alter_dependencies.sql @@ -0,0 +1,16 @@ +CREATE TABLE mv_source (a Int64, insert_time DateTime) ENGINE = MergeTree() ORDER BY insert_time; +CREATE TABLE mv_target (a Int64, insert_time DateTime) ENGINE = MergeTree() ORDER BY insert_time; +CREATE MATERIALIZED VIEW source_to_target to mv_target as Select * from mv_source where a not in (Select sleepEachRow(0.1) from numbers(50)); + +ALTER TABLE mv_source MODIFY TTL insert_time + toIntervalDay(1); +SYSTEM FLUSH LOGS; +-- This is a fancy way to check that the MV hasn't been called (no functions executed by ALTER) +SELECT + ProfileEvents['FunctionExecute'], + ProfileEvents['TableFunctionExecute'] +FROM system.query_log +WHERE + type = 'QueryFinish' AND + query like '%ALTER TABLE mv_source%' AND + current_database = currentDatabase() AND + event_time > now() - INTERVAL 10 minute; From a0b6cd63bba2cf1cdee72b79b641f6a1a4880b25 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Tue, 28 Mar 2023 07:22:31 +0000 Subject: [PATCH 19/52] fix build --- src/Common/ZooKeeper/ZooKeeperArgs.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Common/ZooKeeper/ZooKeeperArgs.cpp b/src/Common/ZooKeeper/ZooKeeperArgs.cpp index 7cd85db931b..dc00d9a66f1 100644 --- a/src/Common/ZooKeeper/ZooKeeperArgs.cpp +++ b/src/Common/ZooKeeper/ZooKeeperArgs.cpp @@ -91,7 +91,7 @@ void ZooKeeperArgs::initFromKeeperServerSection(const Poco::Util::AbstractConfig if (auto session_timeout_key = coordination_key + ".session_timeout_ms"; config.has(session_timeout_key)) - session_timeout_key = config.getInt(session_timeout_key); + session_timeout_ms = config.getInt(session_timeout_key); } Poco::Util::AbstractConfiguration::Keys keys; From e01743a5cdd996d96f711ad5a9056b89ec474c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Tue, 28 Mar 2023 13:34:23 +0200 Subject: [PATCH 20/52] Try to fix unrelated flakiness --- tests/integration/test_version_update_after_mutation/test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_version_update_after_mutation/test.py b/tests/integration/test_version_update_after_mutation/test.py index 6b27c69462a..67f7ce47451 100644 --- a/tests/integration/test_version_update_after_mutation/test.py +++ b/tests/integration/test_version_update_after_mutation/test.py @@ -91,8 +91,8 @@ def test_mutate_and_upgrade(start_cluster): node2.query("OPTIMIZE TABLE mt FINAL") - assert node1.query("SELECT id FROM mt") == "1\n4\n" - assert node2.query("SELECT id FROM mt") == "1\n4\n" + assert node1.query("SELECT id FROM mt ORDER BY id") == "1\n4\n" + assert node2.query("SELECT id FROM mt ORDER BY id") == "1\n4\n" for node in [node1, node2]: node.query("DROP TABLE mt") From d1e874e95bb9062d6ca8be5190d061efa39c76ba Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Fri, 3 Mar 2023 18:22:55 +0100 Subject: [PATCH 21/52] Add a test for ClientInfo initial_query_start_time in inter-server mode Signed-off-by: Azat Khuzhin --- ...nt_info_initial_query_start_time.reference | 8 +++ ...de_client_info_initial_query_start_time.sh | 67 +++++++++++++++++++ 2 files changed, 75 insertions(+) create mode 100644 tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.reference create mode 100755 tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.sh diff --git a/tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.reference b/tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.reference new file mode 100644 index 00000000000..fbce8ae2026 --- /dev/null +++ b/tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.reference @@ -0,0 +1,8 @@ +SELECT +3 0 0 +3 0 0 +INSERT +CHECK +1 +2 +6 0 2 diff --git a/tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.sh b/tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.sh new file mode 100755 index 00000000000..5da643bd17b --- /dev/null +++ b/tests/queries/0_stateless/02590_interserver_mode_client_info_initial_query_start_time.sh @@ -0,0 +1,67 @@ +#!/usr/bin/env bash +# Tags: no-fasttest +# Tag no-fasttest: interserver mode requires SSL +# +# Test that checks that some of ClientInfo correctly passed in inter-server mode. +# NOTE: we need .sh test (.sql is not enough) because queries on remote nodes does not have current_database = currentDatabase() +# +# Check-style suppression: select * from system.query_log where current_database = currentDatabase(); + +CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CUR_DIR"/../shell_config.sh + +function get_query_id() { random_str 10; } + +$CLICKHOUSE_CLIENT -nm -q " + drop table if exists buf; + drop table if exists dist; + drop table if exists data; + + create table data (key Int) engine=Memory(); + create table dist as data engine=Distributed(test_cluster_interserver_secret, currentDatabase(), data, key); + create table dist_dist as data engine=Distributed(test_cluster_interserver_secret, currentDatabase(), dist, key); + system stop distributed sends dist; +" + +echo "SELECT" +query_id="$(get_query_id)" +# initialize connection, but actually if there are other tables that uses this +# cluster then, it will be created long time ago, but this is OK for this +# test, since we care about the difference between NOW() and there should +# not be any significant difference. +$CLICKHOUSE_CLIENT --prefer_localhost_replica=0 --query_id "$query_id" -q "select * from dist" +$CLICKHOUSE_CLIENT -nm --param_query_id "$query_id" -q " + system flush logs; + select count(), countIf(initial_query_start_time_microseconds != query_start_time_microseconds), countIf(event_time - initial_query_start_time > 3) from system.query_log where type = 'QueryFinish' and initial_query_id = {query_id:String}; +" + +sleep 6 + +query_id="$(get_query_id)" +# this query (and all subsequent) should reuse the previous connection (at least most of the time) +$CLICKHOUSE_CLIENT --prefer_localhost_replica=0 --query_id "$query_id" -q "select * from dist" + +$CLICKHOUSE_CLIENT -nm --param_query_id "$query_id" -q " + system flush logs; + select count(), countIf(initial_query_start_time_microseconds != query_start_time_microseconds), countIf(event_time - initial_query_start_time > 3) from system.query_log where type = 'QueryFinish' and initial_query_id = {query_id:String}; +" + +echo "INSERT" +query_id="$(get_query_id)" +$CLICKHOUSE_CLIENT --prefer_localhost_replica=0 --query_id "$query_id" -nm -q " + insert into dist_dist values (1),(2); + select * from data; +" + +sleep 3 +$CLICKHOUSE_CLIENT -nm --param_query_id "$query_id" -q "system flush distributed dist_dist" +sleep 1 +$CLICKHOUSE_CLIENT -nm --param_query_id "$query_id" -q "system flush distributed dist" + +echo "CHECK" +$CLICKHOUSE_CLIENT -nm --param_query_id "$query_id" -q " + select * from data order by key; + system flush logs; + select count(), countIf(initial_query_start_time_microseconds != query_start_time_microseconds), countIf(event_time - initial_query_start_time > 3) from system.query_log where type = 'QueryFinish' and initial_query_id = {query_id:String}; +" From cea631a4c267e96a90c97baa8710a18232b66fa6 Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Tue, 28 Mar 2023 23:24:07 +0800 Subject: [PATCH 22/52] throw exception while non-parametric functions having parameters --- .../UserDefined/UserDefinedSQLFunctionVisitor.cpp | 7 +++++++ src/Interpreters/ActionsVisitor.cpp | 7 +++++++ .../0_stateless/25403_non_parametric_function.reference | 0 .../queries/0_stateless/25403_non_parametric_function.sql | 5 +++++ 4 files changed, 19 insertions(+) create mode 100644 tests/queries/0_stateless/25403_non_parametric_function.reference create mode 100644 tests/queries/0_stateless/25403_non_parametric_function.sql diff --git a/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp b/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp index d78a8623a18..57cc45cc75d 100644 --- a/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp +++ b/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp @@ -20,6 +20,7 @@ namespace DB namespace ErrorCodes { extern const int UNSUPPORTED_METHOD; + extern const int FUNCTION_CANNOT_HAVE_PARAMETERS; } void UserDefinedSQLFunctionVisitor::visit(ASTPtr & ast) @@ -132,6 +133,12 @@ ASTPtr UserDefinedSQLFunctionVisitor::tryToReplaceFunction(const ASTFunction & f if (!user_defined_function) return nullptr; + /// All UDFs are not parametric for now. + if (function.parameters) + { + throw Exception(ErrorCodes::FUNCTION_CANNOT_HAVE_PARAMETERS, "Function {} is not parametric", function.name); + } + const auto & function_arguments_list = function.children.at(0)->as(); auto & function_arguments = function_arguments_list->children; diff --git a/src/Interpreters/ActionsVisitor.cpp b/src/Interpreters/ActionsVisitor.cpp index 8a5ea1205e7..1f4969a7f9a 100644 --- a/src/Interpreters/ActionsVisitor.cpp +++ b/src/Interpreters/ActionsVisitor.cpp @@ -75,6 +75,7 @@ namespace ErrorCodes extern const int LOGICAL_ERROR; extern const int TOO_FEW_ARGUMENTS_FOR_FUNCTION; extern const int TOO_MANY_ARGUMENTS_FOR_FUNCTION; + extern const int FUNCTION_CANNOT_HAVE_PARAMETERS; } static NamesAndTypesList::iterator findColumn(const String & name, NamesAndTypesList & cols) @@ -1109,6 +1110,12 @@ void ActionsMatcher::visit(const ASTFunction & node, const ASTPtr & ast, Data & } } + /// Normal functions are not parametric for now. + if (node.parameters) + { + throw Exception(ErrorCodes::FUNCTION_CANNOT_HAVE_PARAMETERS, "Function {} is not parametric", node.name); + } + Names argument_names; DataTypes argument_types; bool arguments_present = true; diff --git a/tests/queries/0_stateless/25403_non_parametric_function.reference b/tests/queries/0_stateless/25403_non_parametric_function.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/25403_non_parametric_function.sql b/tests/queries/0_stateless/25403_non_parametric_function.sql new file mode 100644 index 00000000000..9b7e2ae7a04 --- /dev/null +++ b/tests/queries/0_stateless/25403_non_parametric_function.sql @@ -0,0 +1,5 @@ +SELECT * FROM system.numbers WHERE number > toUInt64(10)(number) LIMIT 10; -- { serverError 309 } + +CREATE FUNCTION sum_udf as (x, y) -> (x + y); + +SELECT sum_udf(1)(1, 2); -- { serverError 309 } \ No newline at end of file From 2b84fb3fffe2c409407506bab719ff03620f6c91 Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 29 Mar 2023 11:21:30 +0800 Subject: [PATCH 23/52] fix tests --- ...function.reference => 02701_non_parametric_function.reference} | 0 ..._parametric_function.sql => 02701_non_parametric_function.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename tests/queries/0_stateless/{25403_non_parametric_function.reference => 02701_non_parametric_function.reference} (100%) rename tests/queries/0_stateless/{25403_non_parametric_function.sql => 02701_non_parametric_function.sql} (100%) diff --git a/tests/queries/0_stateless/25403_non_parametric_function.reference b/tests/queries/0_stateless/02701_non_parametric_function.reference similarity index 100% rename from tests/queries/0_stateless/25403_non_parametric_function.reference rename to tests/queries/0_stateless/02701_non_parametric_function.reference diff --git a/tests/queries/0_stateless/25403_non_parametric_function.sql b/tests/queries/0_stateless/02701_non_parametric_function.sql similarity index 100% rename from tests/queries/0_stateless/25403_non_parametric_function.sql rename to tests/queries/0_stateless/02701_non_parametric_function.sql From 7078576ee52f6834228be3d1bdb4e67d92afd745 Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 29 Mar 2023 12:58:35 +0800 Subject: [PATCH 24/52] fix tests --- tests/queries/0_stateless/02701_non_parametric_function.sql | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02701_non_parametric_function.sql b/tests/queries/0_stateless/02701_non_parametric_function.sql index 9b7e2ae7a04..9946b50a8c8 100644 --- a/tests/queries/0_stateless/02701_non_parametric_function.sql +++ b/tests/queries/0_stateless/02701_non_parametric_function.sql @@ -2,4 +2,6 @@ SELECT * FROM system.numbers WHERE number > toUInt64(10)(number) LIMIT 10; -- { CREATE FUNCTION sum_udf as (x, y) -> (x + y); -SELECT sum_udf(1)(1, 2); -- { serverError 309 } \ No newline at end of file +SELECT sum_udf(1)(1, 2); -- { serverError 309 } + +DROP FUNCTION sum_udf; \ No newline at end of file From 3507c4998e3a0c2eef6c88c89e06cf66202de1fd Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 29 Mar 2023 14:07:45 +0800 Subject: [PATCH 25/52] fix tests --- tests/queries/0_stateless/02701_non_parametric_function.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02701_non_parametric_function.sql b/tests/queries/0_stateless/02701_non_parametric_function.sql index 9946b50a8c8..1164a060323 100644 --- a/tests/queries/0_stateless/02701_non_parametric_function.sql +++ b/tests/queries/0_stateless/02701_non_parametric_function.sql @@ -1,6 +1,6 @@ SELECT * FROM system.numbers WHERE number > toUInt64(10)(number) LIMIT 10; -- { serverError 309 } -CREATE FUNCTION sum_udf as (x, y) -> (x + y); +CREATE FUNCTION IF NOT EXISTS sum_udf as (x, y) -> (x + y); SELECT sum_udf(1)(1, 2); -- { serverError 309 } From 6738be00298db5095a6b0837b58e4c0337dd1888 Mon Sep 17 00:00:00 2001 From: save-my-heart Date: Wed, 29 Mar 2023 16:14:51 +0800 Subject: [PATCH 26/52] fix tests --- tests/queries/0_stateless/02701_non_parametric_function.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02701_non_parametric_function.sql b/tests/queries/0_stateless/02701_non_parametric_function.sql index 1164a060323..a757343a624 100644 --- a/tests/queries/0_stateless/02701_non_parametric_function.sql +++ b/tests/queries/0_stateless/02701_non_parametric_function.sql @@ -4,4 +4,4 @@ CREATE FUNCTION IF NOT EXISTS sum_udf as (x, y) -> (x + y); SELECT sum_udf(1)(1, 2); -- { serverError 309 } -DROP FUNCTION sum_udf; \ No newline at end of file +DROP FUNCTION IF EXISTS sum_udf; \ No newline at end of file From f4b884a5a82de0c0164f8ad53b2f47fd766a3c93 Mon Sep 17 00:00:00 2001 From: Kruglov Pavel <48961922+Avogar@users.noreply.github.com> Date: Wed, 29 Mar 2023 13:55:29 +0200 Subject: [PATCH 27/52] Fix test flakiness --- tests/queries/0_stateless/02701_non_parametric_function.sql | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02701_non_parametric_function.sql b/tests/queries/0_stateless/02701_non_parametric_function.sql index a757343a624..5261fa7b082 100644 --- a/tests/queries/0_stateless/02701_non_parametric_function.sql +++ b/tests/queries/0_stateless/02701_non_parametric_function.sql @@ -1,7 +1,9 @@ +-- Tags: no-parallel + SELECT * FROM system.numbers WHERE number > toUInt64(10)(number) LIMIT 10; -- { serverError 309 } CREATE FUNCTION IF NOT EXISTS sum_udf as (x, y) -> (x + y); SELECT sum_udf(1)(1, 2); -- { serverError 309 } -DROP FUNCTION IF EXISTS sum_udf; \ No newline at end of file +DROP FUNCTION IF EXISTS sum_udf; From 42c2ccb7cc39e8c2812b1dc03368c55180c47266 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Thu, 30 Mar 2023 00:11:13 +0200 Subject: [PATCH 28/52] Support BACKUP ALL command. --- src/Parsers/ParserBackupQuery.cpp | 13 ++--- .../test_backup_restore_new/test.py | 47 +++++++++++++++++++ 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/src/Parsers/ParserBackupQuery.cpp b/src/Parsers/ParserBackupQuery.cpp index cead1a98c1a..cbe4567ee90 100644 --- a/src/Parsers/ParserBackupQuery.cpp +++ b/src/Parsers/ParserBackupQuery.cpp @@ -103,7 +103,7 @@ namespace }); } - bool parseElement(IParser::Pos & pos, Expected & expected, bool allow_all, Element & element) + bool parseElement(IParser::Pos & pos, Expected & expected, Element & element) { return IParserBase::wrapParseImpl(pos, [&] { @@ -169,7 +169,7 @@ namespace return true; } - if (allow_all && ParserKeyword{"ALL"}.ignore(pos, expected)) + if (ParserKeyword{"ALL"}.ignore(pos, expected)) { element.type = ElementType::ALL; parseExceptDatabases(pos, expected, element.except_databases); @@ -181,7 +181,7 @@ namespace }); } - bool parseElements(IParser::Pos & pos, Expected & expected, bool allow_all, std::vector & elements) + bool parseElements(IParser::Pos & pos, Expected & expected, std::vector & elements) { return IParserBase::wrapParseImpl(pos, [&] { @@ -190,7 +190,7 @@ namespace auto parse_element = [&] { Element element; - if (parseElement(pos, expected, allow_all, element)) + if (parseElement(pos, expected, element)) { result.emplace_back(std::move(element)); return true; @@ -334,11 +334,8 @@ bool ParserBackupQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) else return false; - /// Disable "ALL" if this is a RESTORE command. - bool allow_all = (kind == Kind::RESTORE); - std::vector elements; - if (!parseElements(pos, expected, allow_all, elements)) + if (!parseElements(pos, expected, elements)) return false; String cluster; diff --git a/tests/integration/test_backup_restore_new/test.py b/tests/integration/test_backup_restore_new/test.py index 3f67fe8e5f7..7c3374ffe7d 100644 --- a/tests/integration/test_backup_restore_new/test.py +++ b/tests/integration/test_backup_restore_new/test.py @@ -1184,6 +1184,53 @@ def test_restore_partition(): ) +def test_backup_all(): + create_and_fill_table() + + session_id = new_session_id() + instance.http_query( + "CREATE TEMPORARY TABLE temp_tbl(s String)", params={"session_id": session_id} + ) + instance.http_query( + "INSERT INTO temp_tbl VALUES ('q'), ('w'), ('e')", + params={"session_id": session_id}, + ) + + instance.query("CREATE FUNCTION two_and_half AS (x) -> x * 2.5") + + instance.query("CREATE USER u1 IDENTIFIED BY 'qwe123' SETTINGS custom_a = 1") + + backup_name = new_backup_name() + instance.http_query( + f"BACKUP ALL TO {backup_name}", + params={"session_id": session_id}, + ) + + instance.query("DROP TABLE test.table") + instance.query("DROP FUNCTION two_and_half") + instance.query("DROP USER u1") + + session_id = new_session_id() + instance.http_query( + f"RESTORE ALL FROM {backup_name}", + params={"session_id": session_id}, + method="POST", + ) + + assert instance.query("SELECT count(), sum(x) FROM test.table") == "100\t4950\n" + + assert instance.http_query( + "SELECT * FROM temp_tbl ORDER BY s", params={"session_id": session_id} + ) == TSV([["e"], ["q"], ["w"]]) + + assert instance.query("SELECT two_and_half(6)") == "15\n" + + assert ( + instance.query("SHOW CREATE USER u1") + == "CREATE USER u1 IDENTIFIED WITH sha256_password SETTINGS custom_a = 1\n" + ) + + def test_operation_id(): create_and_fill_table(n=30) From e43780326ec2319ea343fd68bbba1a2bbb1925e2 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Thu, 30 Mar 2023 11:09:07 +0800 Subject: [PATCH 29/52] wip support map args for map_from_arrays --- src/Functions/map.cpp | 56 +++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/src/Functions/map.cpp b/src/Functions/map.cpp index 549de200bea..e1dc58eb077 100644 --- a/src/Functions/map.cpp +++ b/src/Functions/map.cpp @@ -173,23 +173,31 @@ public: getName(), arguments.size()); - const auto * keys_type = checkAndGetDataType(arguments[0].get()); - if (!keys_type) - throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, "First argument for function {} must be an Array", getName()); + /// The first argument should always be Array. + /// Because key type can not be nested type of Map, which is Tuple + DataTypePtr key_type; + if (const auto * keys_type = checkAndGetDataType(arguments[0].get())) + key_type = keys_type->getNestedType(); + else + throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, "First argument for function {} must be Array or Map", getName()); - const auto * values_type = checkAndGetDataType(arguments[1].get()); - if (!values_type) - throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, "Second argument for function {} must be an Array", getName()); + DataTypePtr value_type; + if (const auto * value_array_type = checkAndGetDataType(arguments[1].get())) + value_type = value_array_type->getNestedType(); + else if (const auto * value_map_type = checkAndGetDataType(arguments[1].get())) + value_type = value_map_type->getValueType(); + else + throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, "Second argument for function {} must be Array or Map", getName()); - DataTypes key_value_types{keys_type->getNestedType(), values_type->getNestedType()}; + DataTypes key_value_types{key_type, value_type}; return std::make_shared(key_value_types); } ColumnPtr executeImpl( const ColumnsWithTypeAndName & arguments, const DataTypePtr & /* result_type */, size_t /* input_rows_count */) const override { - ColumnPtr holder_keys; bool is_keys_const = isColumnConst(*arguments[0].column); + ColumnPtr holder_keys; const ColumnArray * col_keys; if (is_keys_const) { @@ -201,24 +209,26 @@ public: col_keys = checkAndGetColumn(arguments[0].column.get()); } - ColumnPtr holder_values; - bool is_values_const = isColumnConst(*arguments[1].column); - const ColumnArray * col_values; - if (is_values_const) - { - holder_values = arguments[1].column->convertToFullColumnIfConst(); - col_values = checkAndGetColumn(holder_values.get()); - } - else - { - col_values = checkAndGetColumn(arguments[1].column.get()); - } + if (!col_keys) + throw Exception(ErrorCodes::ILLEGAL_COLUMN, "The first argument of function {} must be Array", getName()); - if (!col_keys || !col_values) - throw Exception(ErrorCodes::ILLEGAL_COLUMN, "Arguments of function {} must be array", getName()); + bool is_values_const = isColumnConst(*arguments[1].column); + ColumnPtr holder_values; + if (is_values_const) + holder_values = arguments[1].column->convertToFullColumnIfConst(); + else + holder_values = arguments[1].column; + + const ColumnArray * col_values; + if (const auto * col_values_array = checkAndGetColumn(holder_values.get())) + col_values = col_values_array; + else if (const auto * col_values_map = checkAndGetColumn(holder_values.get())) + col_values = &col_values_map->getNestedColumn(); + else + throw Exception(ErrorCodes::ILLEGAL_COLUMN, "The second arguments of function {} must be Array or Map", getName()); if (!col_keys->hasEqualOffsets(*col_values)) - throw Exception(ErrorCodes::SIZES_OF_ARRAYS_DONT_MATCH, "Array arguments for function {} must have equal sizes", getName()); + throw Exception(ErrorCodes::SIZES_OF_ARRAYS_DONT_MATCH, "Two arguments for function {} must have equal sizes", getName()); const auto & data_keys = col_keys->getDataPtr(); const auto & data_values = col_values->getDataPtr(); From e3f4089f3b6923d0281c0fe66cf186d759b07a02 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Thu, 30 Mar 2023 18:00:38 +0800 Subject: [PATCH 30/52] fix bugs --- src/Functions/map.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Functions/map.cpp b/src/Functions/map.cpp index e1dc58eb077..14453de0646 100644 --- a/src/Functions/map.cpp +++ b/src/Functions/map.cpp @@ -185,7 +185,7 @@ public: if (const auto * value_array_type = checkAndGetDataType(arguments[1].get())) value_type = value_array_type->getNestedType(); else if (const auto * value_map_type = checkAndGetDataType(arguments[1].get())) - value_type = value_map_type->getValueType(); + value_type = std::make_shared(value_map_type->getKeyValueTypes()); else throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, "Second argument for function {} must be Array or Map", getName()); From 2a35c189732fb36261dcf63b3885ab364600de79 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Thu, 30 Mar 2023 18:18:21 +0800 Subject: [PATCH 31/52] add docs and uts --- .../functions/tuple-map-functions.md | 20 +++++++++++-------- .../0_stateless/01651_map_functions.reference | 3 +++ .../0_stateless/01651_map_functions.sql | 5 +++++ 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/docs/en/sql-reference/functions/tuple-map-functions.md b/docs/en/sql-reference/functions/tuple-map-functions.md index 34c865e7752..cdecbbcc2e9 100644 --- a/docs/en/sql-reference/functions/tuple-map-functions.md +++ b/docs/en/sql-reference/functions/tuple-map-functions.md @@ -68,9 +68,9 @@ Result: ## mapFromArrays -Merges an [Array](../../sql-reference/data-types/array.md) of keys and an [Array](../../sql-reference/data-types/array.md) of values into a [Map(key, value)](../../sql-reference/data-types/map.md). +Merges an [Array](../../sql-reference/data-types/array.md) of keys and an [Array](../../sql-reference/data-types/array.md) of values into a [Map(key, value)](../../sql-reference/data-types/map.md). Notice that the second argument could also be a [Map](../../sql-reference/data-types/map.md), thus it is casted to an Array when executing. -The function is a more convenient alternative to `CAST((key_array, value_array), 'Map(key_type, value_type)')`. For example, instead of writing `CAST((['aa', 'bb'], [4, 5]), 'Map(String, UInt32)')`, you can write `mapFromArrays(['aa', 'bb'], [4, 5])`. +The function is a more convenient alternative to `CAST((key_array, value_array_or_map), 'Map(key_type, value_type)')`. For example, instead of writing `CAST((['aa', 'bb'], [4, 5]), 'Map(String, UInt32)')`, you can write `mapFromArrays(['aa', 'bb'], [4, 5])`. **Syntax** @@ -82,11 +82,11 @@ Alias: `MAP_FROM_ARRAYS(keys, values)` **Arguments** - `keys` — Given key array to create a map from. The nested type of array must be: [String](../../sql-reference/data-types/string.md), [Integer](../../sql-reference/data-types/int-uint.md), [LowCardinality](../../sql-reference/data-types/lowcardinality.md), [FixedString](../../sql-reference/data-types/fixedstring.md), [UUID](../../sql-reference/data-types/uuid.md), [Date](../../sql-reference/data-types/date.md), [DateTime](../../sql-reference/data-types/datetime.md), [Date32](../../sql-reference/data-types/date32.md), [Enum](../../sql-reference/data-types/enum.md) -- `values` - Given value array to create a map from. +- `values` - Given value array or map to create a map from. **Returned value** -- A map whose keys and values are constructed from the key and value arrays +- A map whose keys and values are constructed from the key array and value array/map. **Example** @@ -94,13 +94,17 @@ Query: ```sql select mapFromArrays(['a', 'b', 'c'], [1, 2, 3]) -``` - -```text + ┌─mapFromArrays(['a', 'b', 'c'], [1, 2, 3])─┐ │ {'a':1,'b':2,'c':3} │ └───────────────────────────────────────────┘ -``` + +SELECT mapFromArrays([1, 2, 3], map('a', 1, 'b', 2, 'c', 3)) + +┌─mapFromArrays([1, 2, 3], map('a', 1, 'b', 2, 'c', 3))─┐ +│ {1:('a',1),2:('b',2),3:('c',3)} │ +└───────────────────────────────────────────────────────┘ +``` ## mapAdd diff --git a/tests/queries/0_stateless/01651_map_functions.reference b/tests/queries/0_stateless/01651_map_functions.reference index f7fd3503327..60f1b6e0d0c 100644 --- a/tests/queries/0_stateless/01651_map_functions.reference +++ b/tests/queries/0_stateless/01651_map_functions.reference @@ -33,3 +33,6 @@ {'aa':4,'bb':5} {'aa':4,'bb':5} {'aa':4,'bb':5} +{'aa':('a',4),'bb':('b',5)} +{'aa':('a',4),'bb':('b',5)} +{'aa':('a',4),'bb':('b',5)} diff --git a/tests/queries/0_stateless/01651_map_functions.sql b/tests/queries/0_stateless/01651_map_functions.sql index 848b6932fe0..5942bf8b2c2 100644 --- a/tests/queries/0_stateless/01651_map_functions.sql +++ b/tests/queries/0_stateless/01651_map_functions.sql @@ -39,3 +39,8 @@ select mapFromArrays(['aa', 'bb'], 5); -- { serverError ILLEGAL_TYPE_OF_ARGUMENT select mapFromArrays(['aa', 'bb'], [4, 5], [6, 7]); -- { serverError NUMBER_OF_ARGUMENTS_DOESNT_MATCH } select mapFromArrays(['aa', 'bb'], [4, 5, 6]); -- { serverError SIZES_OF_ARRAYS_DONT_MATCH } select mapFromArrays([[1,2], [3,4]], [4, 5, 6]); -- { serverError BAD_ARGUMENTS } + +select mapFromArrays(['aa', 'bb'], map('a', 4, 'b', 5)); +select mapFromArrays(['aa', 'bb'], materialize(map('a', 4, 'b', 5))) from numbers(2); +select mapFromArrays(map('a', 4, 'b', 4), ['aa', 'bb']) from numbers(2); -- { serverError ILLEGAL_TYPE_OF_ARGUMENT } +select mapFromArrays(['aa', 'bb'], map('a', 4)); -- { serverError SIZES_OF_ARRAYS_DONT_MATCH } From f31f11dd67d8dbce08ba98c5ae85a61e9fb7ddaa Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Fri, 31 Mar 2023 09:12:41 +0000 Subject: [PATCH 32/52] Disable env credentials for stateless tests --- tests/config/config.d/disable_s3_env_credentials.xml | 5 +++++ tests/config/install.sh | 1 + 2 files changed, 6 insertions(+) create mode 100644 tests/config/config.d/disable_s3_env_credentials.xml diff --git a/tests/config/config.d/disable_s3_env_credentials.xml b/tests/config/config.d/disable_s3_env_credentials.xml new file mode 100644 index 00000000000..24a7e0f2f35 --- /dev/null +++ b/tests/config/config.d/disable_s3_env_credentials.xml @@ -0,0 +1,5 @@ + + + 0 + + diff --git a/tests/config/install.sh b/tests/config/install.sh index a6391f6f43f..44eab0e4db0 100755 --- a/tests/config/install.sh +++ b/tests/config/install.sh @@ -55,6 +55,7 @@ ln -sf $SRC_PATH/config.d/custom_disks_base_path.xml $DEST_SERVER_PATH/config.d/ ln -sf $SRC_PATH/config.d/display_name.xml $DEST_SERVER_PATH/config.d/ ln -sf $SRC_PATH/config.d/reverse_dns_query_function.xml $DEST_SERVER_PATH/config.d/ ln -sf $SRC_PATH/config.d/compressed_marks_and_index.xml $DEST_SERVER_PATH/config.d/ +ln -sf $SRC_PATH/config.d/disable_s3_env_credentials.xml $DEST_SERVER_PATH/config.d/ # Not supported with fasttest. if [ "${DEST_SERVER_PATH}" = "/etc/clickhouse-server" ] From aa8e5a107772b403cb3469e446d00194c448440e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 31 Mar 2023 14:09:21 +0200 Subject: [PATCH 33/52] Don't use CURRENT_WRITE_BUFFER_IS_EXHAUSTED for expected behaviour --- src/IO/CascadeWriteBuffer.cpp | 7 ++++--- src/IO/CascadeWriteBuffer.h | 2 +- src/IO/MemoryReadWriteBuffer.cpp | 8 +------- src/IO/MemoryReadWriteBuffer.h | 6 ++++++ 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/IO/CascadeWriteBuffer.cpp b/src/IO/CascadeWriteBuffer.cpp index 629cbff90af..f0d98027609 100644 --- a/src/IO/CascadeWriteBuffer.cpp +++ b/src/IO/CascadeWriteBuffer.cpp @@ -1,4 +1,5 @@ #include +#include #include namespace DB @@ -35,9 +36,9 @@ void CascadeWriteBuffer::nextImpl() curr_buffer->position() = position(); curr_buffer->next(); } - catch (const Exception & e) + catch (const MemoryWriteBuffer::CurrentBufferExhausted &) { - if (curr_buffer_num < num_sources && e.code() == ErrorCodes::CURRENT_WRITE_BUFFER_IS_EXHAUSTED) + if (curr_buffer_num < num_sources) { /// TODO: protocol should require set(position(), 0) before Exception @@ -46,7 +47,7 @@ void CascadeWriteBuffer::nextImpl() curr_buffer = setNextBuffer(); } else - throw; + throw Exception(ErrorCodes::CURRENT_WRITE_BUFFER_IS_EXHAUSTED, "MemoryWriteBuffer limit is exhausted"); } set(curr_buffer->position(), curr_buffer->buffer().end() - curr_buffer->position()); diff --git a/src/IO/CascadeWriteBuffer.h b/src/IO/CascadeWriteBuffer.h index ebd4f262aa2..1059c5b8ddb 100644 --- a/src/IO/CascadeWriteBuffer.h +++ b/src/IO/CascadeWriteBuffer.h @@ -16,7 +16,7 @@ namespace ErrorCodes * (lazy_sources contains not pointers themself, but their delayed constructors) * * Firtly, CascadeWriteBuffer redirects data to first buffer of the sequence - * If current WriteBuffer cannot receive data anymore, it throws special exception CURRENT_WRITE_BUFFER_IS_EXHAUSTED in nextImpl() body, + * If current WriteBuffer cannot receive data anymore, it throws special exception MemoryWriteBuffer::CurrentBufferExhausted in nextImpl() body, * CascadeWriteBuffer prepare next buffer and continuously redirects data to it. * If there are no buffers anymore CascadeWriteBuffer throws an exception. * diff --git a/src/IO/MemoryReadWriteBuffer.cpp b/src/IO/MemoryReadWriteBuffer.cpp index 93ce5ce7ce9..d6f89108561 100644 --- a/src/IO/MemoryReadWriteBuffer.cpp +++ b/src/IO/MemoryReadWriteBuffer.cpp @@ -5,12 +5,6 @@ namespace DB { -namespace ErrorCodes -{ - extern const int CURRENT_WRITE_BUFFER_IS_EXHAUSTED; -} - - class ReadBufferFromMemoryWriteBuffer : public ReadBuffer, boost::noncopyable, private Allocator { public: @@ -118,7 +112,7 @@ void MemoryWriteBuffer::addChunk() if (0 == next_chunk_size) { set(position(), 0); - throw Exception(ErrorCodes::CURRENT_WRITE_BUFFER_IS_EXHAUSTED, "MemoryWriteBuffer limit is exhausted"); + throw MemoryWriteBuffer::CurrentBufferExhausted(); } } diff --git a/src/IO/MemoryReadWriteBuffer.h b/src/IO/MemoryReadWriteBuffer.h index bcaf9a9a965..ee128c355c6 100644 --- a/src/IO/MemoryReadWriteBuffer.h +++ b/src/IO/MemoryReadWriteBuffer.h @@ -16,6 +16,12 @@ namespace DB class MemoryWriteBuffer : public WriteBuffer, public IReadableWriteBuffer, boost::noncopyable, private Allocator { public: + /// Special exception to throw when the current WriteBuffer cannot receive data + class CurrentBufferExhausted : public std::exception + { + public: + const char * what() const noexcept override { return "MemoryWriteBuffer limit is exhausted"; } + }; /// Use max_total_size_ = 0 for unlimited storage explicit MemoryWriteBuffer( From 1e3abc9e84cfd661f30232e29d200b20999a0d6e Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Fri, 31 Mar 2023 13:29:20 +0000 Subject: [PATCH 34/52] Add strict mode for KeeperMap --- .../table-engines/special/keepermap.md | 5 +- src/Core/Settings.h | 1 + src/Interpreters/MutationsInterpreter.cpp | 9 ++ src/Storages/StorageKeeperMap.cpp | 137 +++++++++++++++--- src/Storages/StorageKeeperMap.h | 4 +- .../02706_keeper_map_insert_strict.reference | 3 + .../02706_keeper_map_insert_strict.sql | 20 +++ 7 files changed, 158 insertions(+), 21 deletions(-) create mode 100644 tests/queries/0_stateless/02706_keeper_map_insert_strict.reference create mode 100644 tests/queries/0_stateless/02706_keeper_map_insert_strict.sql diff --git a/docs/en/engines/table-engines/special/keepermap.md b/docs/en/engines/table-engines/special/keepermap.md index 680413039e7..e5c4dea2339 100644 --- a/docs/en/engines/table-engines/special/keepermap.md +++ b/docs/en/engines/table-engines/special/keepermap.md @@ -78,7 +78,8 @@ Of course, it's possible to manually run `CREATE TABLE` with same path on nonrel ### Inserts -When new rows are inserted into `KeeperMap`, if the key already exists, the value will be updated, otherwise new key is created. +When new rows are inserted into `KeeperMap`, if the key does not exist, a new entry for the key is created. +If the key exists, and setting `keeper_map_strict_mode` is set to `true`, an exception is thrown, otherwise, the value for the key is overwritten. Example: @@ -89,6 +90,7 @@ INSERT INTO keeper_map_table VALUES ('some key', 1, 'value', 3.2); ### Deletes Rows can be deleted using `DELETE` query or `TRUNCATE`. +If the key exists, and setting `keeper_map_strict_mode` is set to `true`, fetching and deleting data will succeed only if it can be executed atomically. ```sql DELETE FROM keeper_map_table WHERE key LIKE 'some%' AND v1 > 1; @@ -105,6 +107,7 @@ TRUNCATE TABLE keeper_map_table; ### Updates Values can be updated using `ALTER TABLE` query. Primary key cannot be updated. +If setting `keeper_map_strict_mode` is set to `true`, fetching and updating data will succeed only if it's executed atomically. ```sql ALTER TABLE keeper_map_table UPDATE v1 = v1 * 10 + 2 WHERE key LIKE 'some%' AND v3 > 3.1; diff --git a/src/Core/Settings.h b/src/Core/Settings.h index e9db155fb12..d985f51eec0 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -722,6 +722,7 @@ class IColumn; M(Bool, force_aggregation_in_order, false, "Force use of aggregation in order on remote nodes during distributed aggregation. PLEASE, NEVER CHANGE THIS SETTING VALUE MANUALLY!", IMPORTANT) \ M(UInt64, http_max_request_param_data_size, 10_MiB, "Limit on size of request data used as a query parameter in predefined HTTP requests.", 0) \ M(Bool, allow_experimental_undrop_table_query, false, "Allow to use undrop query to restore dropped table in a limited time", 0) \ + M(Bool, keeper_map_strict_mode, false, "Enforce additional checks during operations on KeeperMap. E.g. throw an exception on an insert for already existing key", 0) \ // End of COMMON_SETTINGS // Please add settings related to formats into the FORMAT_FACTORY_SETTINGS and move obsolete settings to OBSOLETE_SETTINGS. diff --git a/src/Interpreters/MutationsInterpreter.cpp b/src/Interpreters/MutationsInterpreter.cpp index 0b52a1a51bc..d1fcc006ffb 100644 --- a/src/Interpreters/MutationsInterpreter.cpp +++ b/src/Interpreters/MutationsInterpreter.cpp @@ -550,6 +550,12 @@ void MutationsInterpreter::prepare(bool dry_run) if (source.hasLightweightDeleteMask()) all_columns.push_back({LightweightDeleteDescription::FILTER_COLUMN}); + if (return_all_columns) + { + for (const auto & column : source.getStorage()->getVirtuals()) + all_columns.push_back(column); + } + NameSet updated_columns; bool materialize_ttl_recalculate_only = source.materializeTTLRecalculateOnly(); @@ -906,6 +912,8 @@ void MutationsInterpreter::prepareMutationStages(std::vector & prepared_s { auto storage_snapshot = source.getStorageSnapshot(metadata_snapshot, context); auto options = GetColumnsOptions(GetColumnsOptions::AllPhysical).withExtendedObjects(); + if (return_all_columns) + options.withVirtuals(); auto all_columns = storage_snapshot->getColumns(options); /// Add _row_exists column if it is present in the part @@ -1256,6 +1264,7 @@ void MutationsInterpreter::validate() } QueryPlan plan; + initQueryPlan(stages.front(), plan); auto pipeline = addStreamsForLaterStages(stages, plan); } diff --git a/src/Storages/StorageKeeperMap.cpp b/src/Storages/StorageKeeperMap.cpp index f570f132463..58d02372f2a 100644 --- a/src/Storages/StorageKeeperMap.cpp +++ b/src/Storages/StorageKeeperMap.cpp @@ -59,6 +59,8 @@ namespace ErrorCodes namespace { +constexpr std::string_view version_column_name = "_version"; + std::string formattedAST(const ASTPtr & ast) { if (!ast) @@ -77,7 +79,6 @@ void verifyTableId(const StorageID & table_id) table_id.getDatabaseName(), database->getEngineName()); } - } } @@ -86,11 +87,13 @@ class StorageKeeperMapSink : public SinkToStorage { StorageKeeperMap & storage; std::unordered_map new_values; + std::unordered_map versions; size_t primary_key_pos; + ContextPtr context; public: - StorageKeeperMapSink(StorageKeeperMap & storage_, const StorageMetadataPtr & metadata_snapshot) - : SinkToStorage(metadata_snapshot->getSampleBlock()), storage(storage_) + StorageKeeperMapSink(StorageKeeperMap & storage_, Block header, ContextPtr context_) + : SinkToStorage(std::move(header)), storage(storage_), context(std::move(context_)) { auto primary_key = storage.getPrimaryKey(); assert(primary_key.size() == 1); @@ -113,18 +116,36 @@ public: wb_value.restart(); size_t idx = 0; + + int32_t version = -1; for (const auto & elem : block) { + if (elem.name == version_column_name) + { + version = assert_cast &>(*elem.column).getData()[i]; + continue; + } + elem.type->getDefaultSerialization()->serializeBinary(*elem.column, i, idx == primary_key_pos ? wb_key : wb_value, {}); ++idx; } auto key = base64Encode(wb_key.str(), /* url_encoding */ true); + + if (version != -1) + versions[key] = version; + new_values[std::move(key)] = std::move(wb_value.str()); } } void onFinish() override + { + finalize(/*strict*/ context->getSettingsRef().keeper_map_strict_mode); + } + + template + void finalize(bool strict) { auto zookeeper = storage.getClient(); @@ -147,21 +168,39 @@ public: for (const auto & [key, _] : new_values) key_paths.push_back(storage.fullPathForKey(key)); - auto results = zookeeper->exists(key_paths); + zkutil::ZooKeeper::MultiExistsResponse results; + + if constexpr (!for_update) + results = zookeeper->exists(key_paths); Coordination::Requests requests; requests.reserve(key_paths.size()); for (size_t i = 0; i < key_paths.size(); ++i) { auto key = fs::path(key_paths[i]).filename(); - if (results[i].error == Coordination::Error::ZOK) + + if constexpr (for_update) { - requests.push_back(zkutil::makeSetRequest(key_paths[i], new_values[key], -1)); + int32_t version = -1; + if (strict) + version = versions.at(key); + + requests.push_back(zkutil::makeSetRequest(key_paths[i], new_values[key], version)); } else { - requests.push_back(zkutil::makeCreateRequest(key_paths[i], new_values[key], zkutil::CreateMode::Persistent)); - ++new_keys_num; + if (results[i].error == Coordination::Error::ZOK) + { + if (strict) + throw Exception(ErrorCodes::KEEPER_EXCEPTION, "Value for key '{}' already exists", key); + + requests.push_back(zkutil::makeSetRequest(key_paths[i], new_values[key], -1)); + } + else + { + requests.push_back(zkutil::makeCreateRequest(key_paths[i], new_values[key], zkutil::CreateMode::Persistent)); + ++new_keys_num; + } } } @@ -193,6 +232,18 @@ class StorageKeeperMapSource : public ISource KeyContainerIter it; KeyContainerIter end; + bool with_version_column = false; + + static Block getHeader(Block header, bool with_version_column) + { + if (with_version_column) + header.insert( + {DataTypeInt32{}.createColumn(), + std::make_shared(), std::string{version_column_name}}); + + return header; + } + public: StorageKeeperMapSource( const StorageKeeperMap & storage_, @@ -200,8 +251,10 @@ public: size_t max_block_size_, KeyContainerPtr container_, KeyContainerIter begin_, - KeyContainerIter end_) - : ISource(header), storage(storage_), max_block_size(max_block_size_), container(std::move(container_)), it(begin_), end(end_) + KeyContainerIter end_, + bool with_version_column_) + : ISource(getHeader(header, with_version_column_)), storage(storage_), max_block_size(max_block_size_), container(std::move(container_)), it(begin_), end(end_) + , with_version_column(with_version_column_) { } @@ -225,12 +278,12 @@ public: for (auto & raw_key : raw_keys) raw_key = base64Encode(raw_key, /* url_encoding */ true); - return storage.getBySerializedKeys(raw_keys, nullptr); + return storage.getBySerializedKeys(raw_keys, nullptr, with_version_column); } else { size_t elem_num = std::min(max_block_size, static_cast(end - it)); - auto chunk = storage.getBySerializedKeys(std::span{it, it + elem_num}, nullptr); + auto chunk = storage.getBySerializedKeys(std::span{it, it + elem_num}, nullptr, with_version_column); it += elem_num; return chunk; } @@ -426,6 +479,16 @@ Pipe StorageKeeperMap::read( auto primary_key_type = sample_block.getByName(primary_key).type; std::tie(filtered_keys, all_scan) = getFilterKeys(primary_key, primary_key_type, query_info, context_); + bool with_version_column = false; + for (const auto & column : column_names) + { + if (column == version_column_name) + { + with_version_column = true; + break; + } + } + const auto process_keys = [&](KeyContainerPtr keys) -> Pipe { if (keys->empty()) @@ -449,7 +512,7 @@ Pipe StorageKeeperMap::read( using KeyContainer = typename KeyContainerPtr::element_type; pipes.emplace_back(std::make_shared>( - *this, sample_block, max_block_size, keys, keys->begin() + begin, keys->begin() + end)); + *this, sample_block, max_block_size, keys, keys->begin() + begin, keys->begin() + end, with_version_column)); } return Pipe::unitePipes(std::move(pipes)); }; @@ -461,10 +524,10 @@ Pipe StorageKeeperMap::read( return process_keys(std::move(filtered_keys)); } -SinkToStoragePtr StorageKeeperMap::write(const ASTPtr & /*query*/, const StorageMetadataPtr & metadata_snapshot, ContextPtr /*context*/) +SinkToStoragePtr StorageKeeperMap::write(const ASTPtr & /*query*/, const StorageMetadataPtr & metadata_snapshot, ContextPtr local_context) { checkTable(); - return std::make_shared(*this, metadata_snapshot); + return std::make_shared(*this, metadata_snapshot->getSampleBlock(), local_context); } void StorageKeeperMap::truncate(const ASTPtr &, const StorageMetadataPtr &, ContextPtr, TableExclusiveLockHolder &) @@ -554,6 +617,12 @@ void StorageKeeperMap::drop() dropTable(client, metadata_drop_lock); } +NamesAndTypesList StorageKeeperMap::getVirtuals() const +{ + return NamesAndTypesList{ + {std::string{version_column_name}, std::make_shared()}}; +} + zkutil::ZooKeeperPtr StorageKeeperMap::getClient() const { std::lock_guard lock{zookeeper_mutex}; @@ -670,13 +739,18 @@ Chunk StorageKeeperMap::getByKeys(const ColumnsWithTypeAndName & keys, PaddedPOD if (raw_keys.size() != keys[0].column->size()) throw Exception(ErrorCodes::LOGICAL_ERROR, "Assertion failed: {} != {}", raw_keys.size(), keys[0].column->size()); - return getBySerializedKeys(raw_keys, &null_map); + return getBySerializedKeys(raw_keys, &null_map, /* version_column */ false); } -Chunk StorageKeeperMap::getBySerializedKeys(const std::span keys, PaddedPODArray * null_map) const +Chunk StorageKeeperMap::getBySerializedKeys(const std::span keys, PaddedPODArray * null_map, bool with_version) const { Block sample_block = getInMemoryMetadataPtr()->getSampleBlock(); MutableColumns columns = sample_block.cloneEmptyColumns(); + MutableColumnPtr version_column = nullptr; + + if (with_version) + version_column = ColumnVector::create(); + size_t primary_key_pos = getPrimaryKeyPos(sample_block, getPrimaryKey()); if (null_map) @@ -706,6 +780,9 @@ Chunk StorageKeeperMap::getBySerializedKeys(const std::span k if (code == Coordination::Error::ZOK) { fillColumns(base64Decode(keys[i], true), response.data, primary_key_pos, sample_block, columns); + + if (version_column) + version_column->insert(response.stat.version); } else if (code == Coordination::Error::ZNONODE) { @@ -714,6 +791,9 @@ Chunk StorageKeeperMap::getBySerializedKeys(const std::span k (*null_map)[i] = 0; for (size_t col_idx = 0; col_idx < sample_block.columns(); ++col_idx) columns[col_idx]->insert(sample_block.getByPosition(col_idx).type->getDefault()); + + if (version_column) + version_column->insert(-1); } } else @@ -723,6 +803,10 @@ Chunk StorageKeeperMap::getBySerializedKeys(const std::span k } size_t num_rows = columns.at(0)->size(); + + if (version_column) + columns.push_back(std::move(version_column)); + return Chunk(std::move(columns), num_rows); } @@ -763,6 +847,8 @@ void StorageKeeperMap::mutate(const MutationCommands & commands, ContextPtr loca if (commands.empty()) return; + bool strict = local_context->getSettingsRef().keeper_map_strict_mode; + assert(commands.size() == 1); auto metadata_snapshot = getInMemoryMetadataPtr(); @@ -784,8 +870,10 @@ void StorageKeeperMap::mutate(const MutationCommands & commands, ContextPtr loca auto header = interpreter->getUpdatedHeader(); auto primary_key_pos = header.getPositionByName(primary_key); + auto version_position = header.getPositionByName(std::string{version_column_name}); auto client = getClient(); + Block block; while (executor.pull(block)) { @@ -793,14 +881,23 @@ void StorageKeeperMap::mutate(const MutationCommands & commands, ContextPtr loca auto column = column_type_name.column; auto size = column->size(); + WriteBufferFromOwnString wb_key; Coordination::Requests delete_requests; + for (size_t i = 0; i < size; ++i) { + int32_t version = -1; + if (strict) + { + const auto & version_column = block.getByPosition(version_position).column; + version = assert_cast &>(*version_column).getData()[i]; + } + wb_key.restart(); column_type_name.type->getDefaultSerialization()->serializeBinary(*column, i, wb_key, {}); - delete_requests.emplace_back(zkutil::makeRemoveRequest(fullPathForKey(base64Encode(wb_key.str(), true)), -1)); + delete_requests.emplace_back(zkutil::makeRemoveRequest(fullPathForKey(base64Encode(wb_key.str(), true)), version)); } Coordination::Responses responses; @@ -834,11 +931,13 @@ void StorageKeeperMap::mutate(const MutationCommands & commands, ContextPtr loca auto pipeline = QueryPipelineBuilder::getPipeline(interpreter->execute()); PullingPipelineExecutor executor(pipeline); - auto sink = std::make_shared(*this, metadata_snapshot); + auto sink = std::make_shared(*this, executor.getHeader(), local_context); Block block; while (executor.pull(block)) sink->consume(Chunk{block.getColumns(), block.rows()}); + + sink->finalize(local_context->getSettingsRef().keeper_map_strict_mode); sink->onFinish(); } diff --git a/src/Storages/StorageKeeperMap.h b/src/Storages/StorageKeeperMap.h index a16c662e547..f71ff3cc65a 100644 --- a/src/Storages/StorageKeeperMap.h +++ b/src/Storages/StorageKeeperMap.h @@ -46,11 +46,13 @@ public: void truncate(const ASTPtr &, const StorageMetadataPtr &, ContextPtr, TableExclusiveLockHolder &) override; void drop() override; + NamesAndTypesList getVirtuals() const override; + std::string getName() const override { return "KeeperMap"; } Names getPrimaryKey() const override { return {primary_key}; } Chunk getByKeys(const ColumnsWithTypeAndName & keys, PaddedPODArray & null_map, const Names &) const override; - Chunk getBySerializedKeys(std::span keys, PaddedPODArray * null_map) const; + Chunk getBySerializedKeys(std::span keys, PaddedPODArray * null_map, bool with_version) const; Block getSampleBlock(const Names &) const override; diff --git a/tests/queries/0_stateless/02706_keeper_map_insert_strict.reference b/tests/queries/0_stateless/02706_keeper_map_insert_strict.reference new file mode 100644 index 00000000000..a6bdbb192e4 --- /dev/null +++ b/tests/queries/0_stateless/02706_keeper_map_insert_strict.reference @@ -0,0 +1,3 @@ +1 1.1 +1 2.1 +1 2.1 diff --git a/tests/queries/0_stateless/02706_keeper_map_insert_strict.sql b/tests/queries/0_stateless/02706_keeper_map_insert_strict.sql new file mode 100644 index 00000000000..97c801ec46e --- /dev/null +++ b/tests/queries/0_stateless/02706_keeper_map_insert_strict.sql @@ -0,0 +1,20 @@ +-- Tags: no-ordinary-database, no-fasttest + +DROP TABLE IF EXISTS 02706_keeper_map_insert_strict SYNC; + +CREATE TABLE 02706_keeper_map_insert_strict (key UInt64, value Float64) Engine=KeeperMap('/' || currentDatabase() || '/test_02706_keeper_map_insert_strict') PRIMARY KEY(key); + +INSERT INTO 02706_keeper_map_insert_strict VALUES (1, 1.1), (2, 2.2); +SELECT * FROM 02706_keeper_map_insert_strict WHERE key = 1; + +SET keeper_map_strict_mode = false; + +INSERT INTO 02706_keeper_map_insert_strict VALUES (1, 2.1); +SELECT * FROM 02706_keeper_map_insert_strict WHERE key = 1; + +SET keeper_map_strict_mode = true; + +INSERT INTO 02706_keeper_map_insert_strict VALUES (1, 2.1); -- { serverError KEEPER_EXCEPTION } +SELECT * FROM 02706_keeper_map_insert_strict WHERE key = 1; + +DROP TABLE 02706_keeper_map_insert_strict; From 3f4aadfe7dc0a1795ce55d256696b6756704dc34 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 31 Mar 2023 23:50:35 +0200 Subject: [PATCH 35/52] Add logging for concurrency checks for backups. --- src/Backups/BackupCoordinationLocal.cpp | 10 ++++++++-- src/Backups/BackupCoordinationLocal.h | 2 ++ src/Backups/BackupCoordinationRemote.cpp | 8 ++++++-- src/Backups/BackupCoordinationRemote.h | 1 + src/Backups/RestoreCoordinationLocal.cpp | 13 +++++++++++-- src/Backups/RestoreCoordinationLocal.h | 2 ++ src/Backups/RestoreCoordinationRemote.cpp | 6 +++++- src/Backups/RestoreCoordinationRemote.h | 1 + 8 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/Backups/BackupCoordinationLocal.cpp b/src/Backups/BackupCoordinationLocal.cpp index a96ed443b9c..f3a6c02d228 100644 --- a/src/Backups/BackupCoordinationLocal.cpp +++ b/src/Backups/BackupCoordinationLocal.cpp @@ -8,7 +8,8 @@ namespace DB { -BackupCoordinationLocal::BackupCoordinationLocal(bool plain_backup_) : file_infos(plain_backup_) +BackupCoordinationLocal::BackupCoordinationLocal(bool plain_backup_) + : log(&Poco::Logger::get("BackupCoordinationLocal")), file_infos(plain_backup_) { } @@ -125,7 +126,12 @@ bool BackupCoordinationLocal::startWritingFile(size_t data_file_index) bool BackupCoordinationLocal::hasConcurrentBackups(const std::atomic & num_active_backups) const { - return (num_active_backups > 1); + if (num_active_backups > 1) + { + LOG_WARNING(log, "Found concurrent backups: num_active_backups={}", num_active_backups); + return true; + } + return false; } } diff --git a/src/Backups/BackupCoordinationLocal.h b/src/Backups/BackupCoordinationLocal.h index db2070fa891..60fcc014720 100644 --- a/src/Backups/BackupCoordinationLocal.h +++ b/src/Backups/BackupCoordinationLocal.h @@ -52,6 +52,8 @@ public: bool hasConcurrentBackups(const std::atomic & num_active_backups) const override; private: + Poco::Logger * const log; + BackupCoordinationReplicatedTables TSA_GUARDED_BY(replicated_tables_mutex) replicated_tables; BackupCoordinationReplicatedAccess TSA_GUARDED_BY(replicated_access_mutex) replicated_access; BackupCoordinationReplicatedSQLObjects TSA_GUARDED_BY(replicated_sql_objects_mutex) replicated_sql_objects; diff --git a/src/Backups/BackupCoordinationRemote.cpp b/src/Backups/BackupCoordinationRemote.cpp index 9b4343a1d3b..d6463d08909 100644 --- a/src/Backups/BackupCoordinationRemote.cpp +++ b/src/Backups/BackupCoordinationRemote.cpp @@ -164,17 +164,18 @@ BackupCoordinationRemote::BackupCoordinationRemote( , current_host_index(findCurrentHostIndex(all_hosts, current_host)) , plain_backup(plain_backup_) , is_internal(is_internal_) + , log(&Poco::Logger::get("BackupCoordinationRemote")) { zookeeper_retries_info = ZooKeeperRetriesInfo( "BackupCoordinationRemote", - &Poco::Logger::get("BackupCoordinationRemote"), + log, keeper_settings.keeper_max_retries, keeper_settings.keeper_retry_initial_backoff_ms, keeper_settings.keeper_retry_max_backoff_ms); createRootNodes(); stage_sync.emplace( - zookeeper_path + "/stage", [this] { return getZooKeeper(); }, &Poco::Logger::get("BackupCoordination")); + zookeeper_path + "/stage", [this] { return getZooKeeper(); }, log); } BackupCoordinationRemote::~BackupCoordinationRemote() @@ -664,7 +665,10 @@ bool BackupCoordinationRemote::hasConcurrentBackups(const std::atomic &) const auto status = zk->get(root_zookeeper_path + "/" + existing_backup_path + "/stage"); if (status != Stage::COMPLETED) + { + LOG_WARNING(log, "Found a concurrent backup: {}, current backup: {}", existing_backup_uuid, toString(backup_uuid)); return true; + } } zk->createIfNotExists(backup_stage_path, ""); diff --git a/src/Backups/BackupCoordinationRemote.h b/src/Backups/BackupCoordinationRemote.h index e7f5ff3a211..5155f21c27a 100644 --- a/src/Backups/BackupCoordinationRemote.h +++ b/src/Backups/BackupCoordinationRemote.h @@ -104,6 +104,7 @@ private: const size_t current_host_index; const bool plain_backup; const bool is_internal; + Poco::Logger * const log; mutable ZooKeeperRetriesInfo zookeeper_retries_info; std::optional stage_sync; diff --git a/src/Backups/RestoreCoordinationLocal.cpp b/src/Backups/RestoreCoordinationLocal.cpp index 191cde40aa1..068c4fe7e52 100644 --- a/src/Backups/RestoreCoordinationLocal.cpp +++ b/src/Backups/RestoreCoordinationLocal.cpp @@ -1,10 +1,14 @@ #include +#include namespace DB { -RestoreCoordinationLocal::RestoreCoordinationLocal() = default; +RestoreCoordinationLocal::RestoreCoordinationLocal() : log(&Poco::Logger::get("RestoreCoordinationLocal")) +{ +} + RestoreCoordinationLocal::~RestoreCoordinationLocal() = default; void RestoreCoordinationLocal::setStage(const String &, const String &) @@ -49,7 +53,12 @@ bool RestoreCoordinationLocal::acquireReplicatedSQLObjects(const String &, UserD bool RestoreCoordinationLocal::hasConcurrentRestores(const std::atomic & num_active_restores) const { - return (num_active_restores > 1); + if (num_active_restores > 1) + { + LOG_WARNING(log, "Found concurrent backups: num_active_restores={}", num_active_restores); + return true; + } + return false; } } diff --git a/src/Backups/RestoreCoordinationLocal.h b/src/Backups/RestoreCoordinationLocal.h index bbe76cdf5fd..e27f0d1ef88 100644 --- a/src/Backups/RestoreCoordinationLocal.h +++ b/src/Backups/RestoreCoordinationLocal.h @@ -42,6 +42,8 @@ public: bool hasConcurrentRestores(const std::atomic & num_active_restores) const override; private: + Poco::Logger * const log; + std::set> acquired_tables_in_replicated_databases; std::unordered_set acquired_data_in_replicated_tables; mutable std::mutex mutex; diff --git a/src/Backups/RestoreCoordinationRemote.cpp b/src/Backups/RestoreCoordinationRemote.cpp index 10d085a696a..d93f99a3f2a 100644 --- a/src/Backups/RestoreCoordinationRemote.cpp +++ b/src/Backups/RestoreCoordinationRemote.cpp @@ -25,11 +25,12 @@ RestoreCoordinationRemote::RestoreCoordinationRemote( , current_host(current_host_) , current_host_index(BackupCoordinationRemote::findCurrentHostIndex(all_hosts, current_host)) , is_internal(is_internal_) + , log(&Poco::Logger::get("RestoreCoordinationRemote")) { createRootNodes(); stage_sync.emplace( - zookeeper_path + "/stage", [this] { return getZooKeeper(); }, &Poco::Logger::get("RestoreCoordination")); + zookeeper_path + "/stage", [this] { return getZooKeeper(); }, log); } RestoreCoordinationRemote::~RestoreCoordinationRemote() @@ -197,7 +198,10 @@ bool RestoreCoordinationRemote::hasConcurrentRestores(const std::atomic const auto status = zk->get(root_zookeeper_path + "/" + existing_restore_path + "/stage"); if (status != Stage::COMPLETED) + { + LOG_WARNING(log, "Found a concurrent restore: {}, current restore: {}", existing_restore_uuid, toString(restore_uuid)); return true; + } } zk->createIfNotExists(path, ""); diff --git a/src/Backups/RestoreCoordinationRemote.h b/src/Backups/RestoreCoordinationRemote.h index b78c2e96f9e..e524e42c440 100644 --- a/src/Backups/RestoreCoordinationRemote.h +++ b/src/Backups/RestoreCoordinationRemote.h @@ -59,6 +59,7 @@ private: const String current_host; const size_t current_host_index; const bool is_internal; + Poco::Logger * const log; std::optional stage_sync; From 225fee16dc82a079bee7517948742c04c396b656 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Sat, 1 Apr 2023 19:46:53 +0200 Subject: [PATCH 36/52] Fix test "test_backup_all" and add more test cases. --- .../test_backup_restore_new/test.py | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/tests/integration/test_backup_restore_new/test.py b/tests/integration/test_backup_restore_new/test.py index 7c3374ffe7d..424799e2402 100644 --- a/tests/integration/test_backup_restore_new/test.py +++ b/tests/integration/test_backup_restore_new/test.py @@ -1184,7 +1184,8 @@ def test_restore_partition(): ) -def test_backup_all(): +@pytest.mark.parametrize("exclude_system_log_tables", [False, True]) +def test_backup_all(exclude_system_log_tables): create_and_fill_table() session_id = new_session_id() @@ -1201,20 +1202,34 @@ def test_backup_all(): instance.query("CREATE USER u1 IDENTIFIED BY 'qwe123' SETTINGS custom_a = 1") backup_name = new_backup_name() - instance.http_query( - f"BACKUP ALL TO {backup_name}", - params={"session_id": session_id}, - ) + + exclude_from_backup = [] + if exclude_system_log_tables: + system_log_tables = ( + instance.query( + "SELECT concat('system.', table) FROM system.tables WHERE (database = 'system') AND (table LIKE '%_log')" + ) + .rstrip("\n") + .split("\n") + ) + exclude_from_backup += system_log_tables + + backup_command = f"BACKUP ALL {'EXCEPT TABLES ' + ','.join(exclude_from_backup) if exclude_from_backup else ''} TO {backup_name}" + + instance.http_query(backup_command, params={"session_id": session_id}) instance.query("DROP TABLE test.table") instance.query("DROP FUNCTION two_and_half") instance.query("DROP USER u1") + restore_settings = [] + if not exclude_system_log_tables: + restore_settings.append("allow_non_empty_tables=true") + restore_command = f"RESTORE ALL FROM {backup_name} {'SETTINGS '+ ', '.join(restore_settings) if restore_settings else ''}" + session_id = new_session_id() instance.http_query( - f"RESTORE ALL FROM {backup_name}", - params={"session_id": session_id}, - method="POST", + restore_command, params={"session_id": session_id}, method="POST" ) assert instance.query("SELECT count(), sum(x) FROM test.table") == "100\t4950\n" @@ -1230,6 +1245,10 @@ def test_backup_all(): == "CREATE USER u1 IDENTIFIED WITH sha256_password SETTINGS custom_a = 1\n" ) + instance.query("DROP TABLE test.table") + instance.query("DROP FUNCTION two_and_half") + instance.query("DROP USER u1") + def test_operation_id(): create_and_fill_table(n=30) From 9a051bc27ec20213e351216bc8c32c9c91d76adc Mon Sep 17 00:00:00 2001 From: Denny Crane Date: Sun, 2 Apr 2023 12:30:36 -0300 Subject: [PATCH 37/52] some complex query (it fails with analyzer enabled --- ...707_complex_query_fails_analyzer.reference | 10 ++ .../02707_complex_query_fails_analyzer.sql | 117 ++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 tests/queries/0_stateless/02707_complex_query_fails_analyzer.reference create mode 100644 tests/queries/0_stateless/02707_complex_query_fails_analyzer.sql diff --git a/tests/queries/0_stateless/02707_complex_query_fails_analyzer.reference b/tests/queries/0_stateless/02707_complex_query_fails_analyzer.reference new file mode 100644 index 00000000000..192f8aa904a --- /dev/null +++ b/tests/queries/0_stateless/02707_complex_query_fails_analyzer.reference @@ -0,0 +1,10 @@ +1 1 -59.952 +1 2 59.952 +1 3 -100 +2 1 -93.7611 +2 2 93.7611 +3 1 0 +3 2 0 +--------- +0 +0 diff --git a/tests/queries/0_stateless/02707_complex_query_fails_analyzer.sql b/tests/queries/0_stateless/02707_complex_query_fails_analyzer.sql new file mode 100644 index 00000000000..a9d83479d50 --- /dev/null +++ b/tests/queries/0_stateless/02707_complex_query_fails_analyzer.sql @@ -0,0 +1,117 @@ +DROP TABLE IF EXISTS srv_account_parts; +DROP TABLE IF EXISTS etl_batch; + +CREATE TABLE srv_account_parts( + shard_num UInt16, + account_ids Array(Int64) +)ENGINE = ReplacingMergeTree +ORDER BY shard_num +as select * from values ((0,[]),(1,[1,2,3]),(2,[1,2,3]),(3,[1])); + +CREATE TABLE etl_batch( + batch_id UInt64, + batch_start DateTime, + batch_start_day Date DEFAULT toDate(batch_start), + batch_load DateTime, + total_num_records UInt32, + etl_server_id Int32, + account_id UInt64, + shard_num UInt16 +)ENGINE = ReplacingMergeTree +PARTITION BY toYYYYMM(batch_start_day) +ORDER BY (batch_id, etl_server_id, account_id); + +insert into etl_batch(batch_id, batch_start, batch_load, total_num_records, etl_server_id, account_id, shard_num) +select number batch_id, + toDateTime('2022-01-01') + INTERVAL 23 HOUR batch_start, + batch_start batch_load, + 333 total_num_records, + 1 etl_server_id, + number%3+1 account_id, + 1 shard_num +from numbers(1000); + +insert into etl_batch(batch_id, batch_start, batch_load, total_num_records, etl_server_id, account_id, shard_num) +select number+2000 batch_id, + toDateTime('2022-01-01') + INTERVAL 23 HOUR batch_start, + batch_start batch_load, + 333 total_num_records, + 1 etl_server_id, + number%3+1 account_id, + 2 shard_num +from numbers(1000); + +insert into etl_batch(batch_id, batch_start, batch_load, total_num_records, etl_server_id, account_id, shard_num) +select number+4000 batch_id, + toDateTime('2022-01-01') + INTERVAL 3 HOUR batch_start, + batch_start batch_load, + 3333 total_num_records, + 1 etl_server_id, + 2 account_id, + 2 shard_num +from numbers(1000); + +insert into etl_batch(batch_id, batch_start, batch_load, total_num_records, etl_server_id, account_id, shard_num) +select number+6000 batch_id, + toDateTime('2022-01-01') + INTERVAL 23 HOUR batch_start, + batch_start batch_load, + 333 total_num_records, + 1 etl_server_id, + 1 account_id, + 2 shard_num +from numbers(1000); + +insert into etl_batch(batch_id, batch_start, batch_load, total_num_records, etl_server_id, account_id, shard_num) +select number+8000 batch_id, + toDateTime('2022-01-01') + INTERVAL 23 HOUR batch_start, + batch_start batch_load, + 1000 total_num_records, + 1 etl_server_id, + 3 account_id, + 3 shard_num +from numbers(1000); + +CREATE OR REPLACE VIEW v_num_records_by_node_bias_acc as +SELECT shard_num, + arrayJoin(account_ids) AS account_id, + records_24h, + records_12h, + IF (b = '',-100,xbias) AS bias, + IF (bias > 10,0,IF (bias > 0,1,IF (bias < -10,301,300))) AS sbias +FROM srv_account_parts + LEFT JOIN (SELECT account_id, + shard_num, + records_24h, + records_12h, + xbias, + 'b' AS b + FROM (SELECT account_id, + groupArray((shard_num,records_24h,records_12h)) AS ga, + arraySum(ga.2) AS tot24, + arraySum(ga.3) AS tot12, + arrayMap(i ->(((((i.2)*LENGTH(ga))*100) / tot24) - 100),ga) AS bias24, + arrayMap(i ->(((((i.3)*LENGTH(ga))*100) / tot12) - 100),ga) AS bias12, + arrayMap((i,j,k) ->(i,IF (tot12 = 0,0,IF (ABS(j) > ABS(k),j,k))),ga,bias24,bias12) AS a_bias + FROM (SELECT shard_num, + toInt64(account_id) AS account_id, + SUM(total_num_records) AS records_24h, + sumIf(total_num_records,batch_load >(toDateTime('2022-01-02') -(3600*12))) AS records_12h + FROM etl_batch FINAL PREWHERE (batch_start_day >= (toDate('2022-01-02') - 2)) AND (batch_load > (toDateTime('2022-01-02') - (3600*24))) + where (shard_num, account_id) in (select shard_num, arrayJoin(account_ids) from srv_account_parts) + GROUP BY shard_num,account_id) + GROUP BY account_id) + ARRAY JOIN (a_bias.1).1 AS shard_num,a_bias.2 AS xbias, (a_bias.1).2 AS records_24h, (a_bias.1).3 AS records_12h + ) s USING (shard_num,account_id); + +select account_id, shard_num, round(bias,4) +from v_num_records_by_node_bias_acc +order by account_id, shard_num, bias; + +select '---------'; + +SELECT a AS b FROM (SELECT 0 a) s LEFT JOIN (SELECT 0 b) t USING (b); + +SELECT arrayJoin(a) AS b FROM (SELECT [0] a) s LEFT JOIN (SELECT 0 b) t USING (b); + +DROP TABLE srv_account_parts; +DROP TABLE etl_batch; From b4ea2268ca38603d12d4b0ade370924be3fb1930 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 3 Apr 2023 10:54:47 +0200 Subject: [PATCH 38/52] Adapt unit tests to the new exception --- src/IO/tests/gtest_cascade_and_memory_write_buffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/IO/tests/gtest_cascade_and_memory_write_buffer.cpp b/src/IO/tests/gtest_cascade_and_memory_write_buffer.cpp index 36b4ec10de0..583ba9c97de 100644 --- a/src/IO/tests/gtest_cascade_and_memory_write_buffer.cpp +++ b/src/IO/tests/gtest_cascade_and_memory_write_buffer.cpp @@ -198,7 +198,7 @@ TEST(MemoryWriteBuffer, WriteAndReread) if (s > 1) { MemoryWriteBuffer buf(s - 1); - EXPECT_THROW(buf.write(data.data(), data.size()), DB::Exception); + EXPECT_THROW(buf.write(data.data(), data.size()), MemoryWriteBuffer::CurrentBufferExhausted); } } From 40a0ecf66a35f23addcf9211642ff301eb4c897a Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 3 Apr 2023 09:30:39 +0000 Subject: [PATCH 39/52] Fix --- src/Storages/StorageKeeperMap.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/Storages/StorageKeeperMap.cpp b/src/Storages/StorageKeeperMap.cpp index 58d02372f2a..aeb206f1e05 100644 --- a/src/Storages/StorageKeeperMap.cpp +++ b/src/Storages/StorageKeeperMap.cpp @@ -93,7 +93,7 @@ class StorageKeeperMapSink : public SinkToStorage public: StorageKeeperMapSink(StorageKeeperMap & storage_, Block header, ContextPtr context_) - : SinkToStorage(std::move(header)), storage(storage_), context(std::move(context_)) + : SinkToStorage(header), storage(storage_), context(std::move(context_)) { auto primary_key = storage.getPrimaryKey(); assert(primary_key.size() == 1); @@ -171,7 +171,10 @@ public: zkutil::ZooKeeper::MultiExistsResponse results; if constexpr (!for_update) - results = zookeeper->exists(key_paths); + { + if (!strict) + results = zookeeper->exists(key_paths); + } Coordination::Requests requests; requests.reserve(key_paths.size()); @@ -189,11 +192,8 @@ public: } else { - if (results[i].error == Coordination::Error::ZOK) + if (!strict && results[i].error == Coordination::Error::ZOK) { - if (strict) - throw Exception(ErrorCodes::KEEPER_EXCEPTION, "Value for key '{}' already exists", key); - requests.push_back(zkutil::makeSetRequest(key_paths[i], new_values[key], -1)); } else @@ -937,8 +937,7 @@ void StorageKeeperMap::mutate(const MutationCommands & commands, ContextPtr loca while (executor.pull(block)) sink->consume(Chunk{block.getColumns(), block.rows()}); - sink->finalize(local_context->getSettingsRef().keeper_map_strict_mode); - sink->onFinish(); + sink->finalize(strict); } namespace From 95661c13bcf3fb5576b80d3afe9b760e508ccf41 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 3 Apr 2023 09:55:11 +0000 Subject: [PATCH 40/52] Add tests for strict update/delete --- ..._keeper_map_delete_update_strict.reference | 32 ++++++++++++++ .../02707_keeper_map_delete_update_strict.sql | 44 +++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 tests/queries/0_stateless/02707_keeper_map_delete_update_strict.reference create mode 100644 tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql diff --git a/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.reference b/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.reference new file mode 100644 index 00000000000..8ca8c0ca5a2 --- /dev/null +++ b/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.reference @@ -0,0 +1,32 @@ +1 Some string 0 +2 Some other string 0 +3 random 0 +4 random2 0 +----------- +3 random 0 +4 random2 0 +----------- +3 random 0 +----------- +0 +----------- +1 String 10 +2 String 20 +3 String 30 +4 String 40 +----------- +1 String 10 +2 String 20 +3 Another 30 +4 Another 40 +----------- +1 String 10 +2 String 20 +3 Another 30 +4 Another 40 +----------- +1 String 102 +2 String 202 +3 Another 302 +4 Another 402 +----------- diff --git a/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql b/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql new file mode 100644 index 00000000000..1e14675d353 --- /dev/null +++ b/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql @@ -0,0 +1,44 @@ +-- Tags: no-ordinary-database, no-fasttest + +DROP TABLE IF EXISTS 02661_keepermap_delete_update; + +SET keeper_map_strict_mode = 1; + +CREATE TABLE 02661_keepermap_delete_update (key UInt64, value String, value2 UInt64) ENGINE=KeeperMap('/' || currentDatabase() || '/test02661_keepermap_delete_update') PRIMARY KEY(key); + +INSERT INTO 02661_keepermap_delete_update VALUES (1, 'Some string', 0), (2, 'Some other string', 0), (3, 'random', 0), (4, 'random2', 0); + +SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT '-----------'; + +DELETE FROM 02661_keepermap_delete_update WHERE value LIKE 'Some%string'; + +SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT '-----------'; + +ALTER TABLE 02661_keepermap_delete_update DELETE WHERE key >= 4; + +SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT '-----------'; + +DELETE FROM 02661_keepermap_delete_update WHERE 1 = 1; +SELECT count() FROM 02661_keepermap_delete_update; +SELECT '-----------'; + +INSERT INTO 02661_keepermap_delete_update VALUES (1, 'String', 10), (2, 'String', 20), (3, 'String', 30), (4, 'String', 40); +SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT '-----------'; + +ALTER TABLE 02661_keepermap_delete_update UPDATE value = 'Another' WHERE key > 2; +SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT '-----------'; + +ALTER TABLE 02661_keepermap_delete_update UPDATE key = key * 10 WHERE 1 = 1; -- { serverError 36 } +SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT '-----------'; + +ALTER TABLE 02661_keepermap_delete_update UPDATE value2 = value2 * 10 + 2 WHERE value2 < 100; +SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT '-----------'; + +DROP TABLE IF EXISTS 02661_keepermap_delete_update; From adfedf692a68cf9e93a4d6103c22aea19d7c019f Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 3 Apr 2023 13:13:19 +0200 Subject: [PATCH 41/52] Update table name --- .../02707_keeper_map_delete_update_strict.sql | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql b/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql index 1e14675d353..aaf4f2fd838 100644 --- a/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql +++ b/tests/queries/0_stateless/02707_keeper_map_delete_update_strict.sql @@ -1,44 +1,44 @@ -- Tags: no-ordinary-database, no-fasttest -DROP TABLE IF EXISTS 02661_keepermap_delete_update; +DROP TABLE IF EXISTS 02707_keepermap_delete_update; SET keeper_map_strict_mode = 1; -CREATE TABLE 02661_keepermap_delete_update (key UInt64, value String, value2 UInt64) ENGINE=KeeperMap('/' || currentDatabase() || '/test02661_keepermap_delete_update') PRIMARY KEY(key); +CREATE TABLE 02707_keepermap_delete_update (key UInt64, value String, value2 UInt64) ENGINE=KeeperMap('/' || currentDatabase() || '/test02707_keepermap_delete_update') PRIMARY KEY(key); -INSERT INTO 02661_keepermap_delete_update VALUES (1, 'Some string', 0), (2, 'Some other string', 0), (3, 'random', 0), (4, 'random2', 0); +INSERT INTO 02707_keepermap_delete_update VALUES (1, 'Some string', 0), (2, 'Some other string', 0), (3, 'random', 0), (4, 'random2', 0); -SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT * FROM 02707_keepermap_delete_update ORDER BY key; SELECT '-----------'; -DELETE FROM 02661_keepermap_delete_update WHERE value LIKE 'Some%string'; +DELETE FROM 02707_keepermap_delete_update WHERE value LIKE 'Some%string'; -SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT * FROM 02707_keepermap_delete_update ORDER BY key; SELECT '-----------'; -ALTER TABLE 02661_keepermap_delete_update DELETE WHERE key >= 4; +ALTER TABLE 02707_keepermap_delete_update DELETE WHERE key >= 4; -SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +SELECT * FROM 02707_keepermap_delete_update ORDER BY key; SELECT '-----------'; -DELETE FROM 02661_keepermap_delete_update WHERE 1 = 1; -SELECT count() FROM 02661_keepermap_delete_update; +DELETE FROM 02707_keepermap_delete_update WHERE 1 = 1; +SELECT count() FROM 02707_keepermap_delete_update; SELECT '-----------'; -INSERT INTO 02661_keepermap_delete_update VALUES (1, 'String', 10), (2, 'String', 20), (3, 'String', 30), (4, 'String', 40); -SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +INSERT INTO 02707_keepermap_delete_update VALUES (1, 'String', 10), (2, 'String', 20), (3, 'String', 30), (4, 'String', 40); +SELECT * FROM 02707_keepermap_delete_update ORDER BY key; SELECT '-----------'; -ALTER TABLE 02661_keepermap_delete_update UPDATE value = 'Another' WHERE key > 2; -SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +ALTER TABLE 02707_keepermap_delete_update UPDATE value = 'Another' WHERE key > 2; +SELECT * FROM 02707_keepermap_delete_update ORDER BY key; SELECT '-----------'; -ALTER TABLE 02661_keepermap_delete_update UPDATE key = key * 10 WHERE 1 = 1; -- { serverError 36 } -SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +ALTER TABLE 02707_keepermap_delete_update UPDATE key = key * 10 WHERE 1 = 1; -- { serverError 36 } +SELECT * FROM 02707_keepermap_delete_update ORDER BY key; SELECT '-----------'; -ALTER TABLE 02661_keepermap_delete_update UPDATE value2 = value2 * 10 + 2 WHERE value2 < 100; -SELECT * FROM 02661_keepermap_delete_update ORDER BY key; +ALTER TABLE 02707_keepermap_delete_update UPDATE value2 = value2 * 10 + 2 WHERE value2 < 100; +SELECT * FROM 02707_keepermap_delete_update ORDER BY key; SELECT '-----------'; -DROP TABLE IF EXISTS 02661_keepermap_delete_update; +DROP TABLE IF EXISTS 02707_keepermap_delete_update; From 00e335530e690f996a26f566ecee032fe9489bbc Mon Sep 17 00:00:00 2001 From: Yakov Olkhovskiy <99031427+yakov-olkhovskiy@users.noreply.github.com> Date: Mon, 3 Apr 2023 08:15:18 -0400 Subject: [PATCH 42/52] close client --- tests/clickhouse-test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/clickhouse-test b/tests/clickhouse-test index 9067a8142bc..d0dfdd783f1 100755 --- a/tests/clickhouse-test +++ b/tests/clickhouse-test @@ -144,7 +144,7 @@ def clickhouse_execute_http( except Exception as ex: if i == max_http_retries - 1: raise ex - + client.close() sleep(i + 1) if res.status != 200: From e7d00c8f340497dde2a84325d3f1baf8616f74c0 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 3 Apr 2023 13:56:03 +0000 Subject: [PATCH 43/52] Don't replicate mutations for KeeperMap tables --- src/Databases/DatabaseReplicated.cpp | 8 ++++++++ src/Interpreters/InterpreterAlterQuery.cpp | 12 +++++++++--- .../0_stateless/02577_keepermap_delete_update.sql | 4 +++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Databases/DatabaseReplicated.cpp b/src/Databases/DatabaseReplicated.cpp index 76eea059174..efac04d9e15 100644 --- a/src/Databases/DatabaseReplicated.cpp +++ b/src/Databases/DatabaseReplicated.cpp @@ -34,6 +34,7 @@ #include #include #include +#include namespace DB { @@ -1390,6 +1391,13 @@ bool DatabaseReplicated::shouldReplicateQuery(const ContextPtr & query_context, /// Some ALTERs are not replicated on database level if (const auto * alter = query_ptr->as()) { + auto table_id = query_context->resolveStorageID(*alter, Context::ResolveOrdinary); + StoragePtr table = DatabaseCatalog::instance().getTable(table_id, query_context); + + /// we never replicate KeeperMap operations because it doesn't make sense + if (auto * keeper_map = table->as()) + return false; + return !alter->isAttachAlter() && !alter->isFetchAlter() && !alter->isDropPartitionAlter(); } diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index fabcc6844e5..3dfa29fbb01 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -39,6 +40,7 @@ namespace ErrorCodes extern const int INCORRECT_QUERY; extern const int NOT_IMPLEMENTED; extern const int TABLE_IS_READ_ONLY; + extern const int BAD_ARGUMENTS; } @@ -72,16 +74,21 @@ BlockIO InterpreterAlterQuery::executeToTable(const ASTAlterQuery & alter) if (!UserDefinedSQLFunctionFactory::instance().empty()) UserDefinedSQLFunctionVisitor::visit(query_ptr); + auto table_id = getContext()->resolveStorageID(alter, Context::ResolveOrdinary); + query_ptr->as().setDatabase(table_id.database_name); + StoragePtr table = DatabaseCatalog::instance().getTable(table_id, getContext()); + if (!alter.cluster.empty() && !maybeRemoveOnCluster(query_ptr, getContext())) { + if (table->as()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Mutations with ON CLUSTER are not allowed for KeeperMap tables"); + DDLQueryOnClusterParams params; params.access_to_check = getRequiredAccess(); return executeDDLQueryOnCluster(query_ptr, getContext(), params); } getContext()->checkAccess(getRequiredAccess()); - auto table_id = getContext()->resolveStorageID(alter, Context::ResolveOrdinary); - query_ptr->as().setDatabase(table_id.database_name); DatabasePtr database = DatabaseCatalog::instance().getDatabase(table_id.database_name); if (database->shouldReplicateQuery(getContext(), query_ptr)) @@ -91,7 +98,6 @@ BlockIO InterpreterAlterQuery::executeToTable(const ASTAlterQuery & alter) return database->tryEnqueueReplicatedDDL(query_ptr, getContext()); } - StoragePtr table = DatabaseCatalog::instance().getTable(table_id, getContext()); checkStorageSupportsTransactionsIfNeeded(table, getContext()); if (table->isStaticStorage()) throw Exception(ErrorCodes::TABLE_IS_READ_ONLY, "Table is read-only"); diff --git a/tests/queries/0_stateless/02577_keepermap_delete_update.sql b/tests/queries/0_stateless/02577_keepermap_delete_update.sql index 199a653822c..942dd28cd46 100644 --- a/tests/queries/0_stateless/02577_keepermap_delete_update.sql +++ b/tests/queries/0_stateless/02577_keepermap_delete_update.sql @@ -31,7 +31,7 @@ ALTER TABLE 02661_keepermap_delete_update UPDATE value = 'Another' WHERE key > 2 SELECT * FROM 02661_keepermap_delete_update ORDER BY key; SELECT '-----------'; -ALTER TABLE 02661_keepermap_delete_update UPDATE key = key * 10 WHERE 1 = 1; -- { serverError 36 } +ALTER TABLE 02661_keepermap_delete_update UPDATE key = key * 10 WHERE 1 = 1; -- { serverError BAD_ARGUMENTS } SELECT * FROM 02661_keepermap_delete_update ORDER BY key; SELECT '-----------'; @@ -39,4 +39,6 @@ ALTER TABLE 02661_keepermap_delete_update UPDATE value2 = value2 * 10 + 2 WHERE SELECT * FROM 02661_keepermap_delete_update ORDER BY key; SELECT '-----------'; +ALTER TABLE 02661_keepermap_delete_update ON CLUSTER test_shard_localhost UPDATE value2 = value2 * 10 + 2 WHERE value2 < 100; -- { serverError BAD_ARGUMENTS } + DROP TABLE IF EXISTS 02661_keepermap_delete_update; From 12bee0573fe3dcde656942181a0d234994930c48 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 3 Apr 2023 14:54:14 +0000 Subject: [PATCH 44/52] Correctly check table --- src/Interpreters/InterpreterAlterQuery.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index 3dfa29fbb01..c683296a2ba 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -76,11 +76,11 @@ BlockIO InterpreterAlterQuery::executeToTable(const ASTAlterQuery & alter) auto table_id = getContext()->resolveStorageID(alter, Context::ResolveOrdinary); query_ptr->as().setDatabase(table_id.database_name); - StoragePtr table = DatabaseCatalog::instance().getTable(table_id, getContext()); + StoragePtr table = DatabaseCatalog::instance().tryGetTable(table_id, getContext()); if (!alter.cluster.empty() && !maybeRemoveOnCluster(query_ptr, getContext())) { - if (table->as()) + if (table && table->as()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Mutations with ON CLUSTER are not allowed for KeeperMap tables"); DDLQueryOnClusterParams params; @@ -98,6 +98,9 @@ BlockIO InterpreterAlterQuery::executeToTable(const ASTAlterQuery & alter) return database->tryEnqueueReplicatedDDL(query_ptr, getContext()); } + if (!table) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Could not find table: {}", table_id.table_name); + checkStorageSupportsTransactionsIfNeeded(table, getContext()); if (table->isStaticStorage()) throw Exception(ErrorCodes::TABLE_IS_READ_ONLY, "Table is read-only"); From e81c2999a3ffe0733be88ced07456fca67119c4b Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 3 Apr 2023 16:56:45 +0200 Subject: [PATCH 45/52] Update src/Interpreters/InterpreterAlterQuery.cpp Co-authored-by: Alexander Tokmakov --- src/Interpreters/InterpreterAlterQuery.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index c683296a2ba..49bc18534a8 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -99,7 +99,7 @@ BlockIO InterpreterAlterQuery::executeToTable(const ASTAlterQuery & alter) } if (!table) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Could not find table: {}", table_id.table_name); + throw Exception(ErrorCodes::UNKNOWN_TABLE, "Could not find table: {}", table_id.table_name); checkStorageSupportsTransactionsIfNeeded(table, getContext()); if (table->isStaticStorage()) From f21c664744897be6da191b9e62d4768abc098194 Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 3 Apr 2023 15:17:06 +0000 Subject: [PATCH 46/52] Add error code --- src/Interpreters/InterpreterAlterQuery.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index 49bc18534a8..21f0fbadd09 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -41,6 +41,7 @@ namespace ErrorCodes extern const int NOT_IMPLEMENTED; extern const int TABLE_IS_READ_ONLY; extern const int BAD_ARGUMENTS; + extern const int UNKNOWN_TABLE; } From fea5fae5b00294fa0b0729cc7ee109a572510180 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Mon, 3 Apr 2023 17:42:57 +0000 Subject: [PATCH 47/52] Fix 02494_query_cache_drop.sql Failing with high rate in master after #45912 was merged --- tests/queries/0_stateless/02494_query_cache_drop_cache.sql | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/queries/0_stateless/02494_query_cache_drop_cache.sql b/tests/queries/0_stateless/02494_query_cache_drop_cache.sql index 1f61472fcb0..078057a834f 100644 --- a/tests/queries/0_stateless/02494_query_cache_drop_cache.sql +++ b/tests/queries/0_stateless/02494_query_cache_drop_cache.sql @@ -3,6 +3,9 @@ SET allow_experimental_query_cache = true; +-- (it's silly to use what will be tested below but we have to assume other tests cluttered the query cache) +SYSTEM DROP QUERY CACHE; + -- Cache query result in query cache SELECT 1 SETTINGS use_query_cache = true; SELECT count(*) FROM system.query_cache; From 1cb8a7c45cd5894facc0abeb17e7e89c9a4b5821 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Tue, 4 Apr 2023 00:46:25 +0300 Subject: [PATCH 48/52] Update tips.md See Telegram. --- docs/en/operations/tips.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/operations/tips.md b/docs/en/operations/tips.md index 693dcd0d21b..8f6cf6ad147 100644 --- a/docs/en/operations/tips.md +++ b/docs/en/operations/tips.md @@ -74,7 +74,7 @@ Never set the block size too small or too large. You can use RAID-0 on SSD. Regardless of RAID use, always use replication for data security. -Enable NCQ with a long queue. For HDD, choose the CFQ scheduler, and for SSD, choose noop. Don’t reduce the ‘readahead’ setting. +Enable NCQ with a long queue. For HDD, choose the mq-deadline or CFQ scheduler, and for SSD, choose noop. Don’t reduce the ‘readahead’ setting. For HDD, enable the write cache. Make sure that [`fstrim`](https://en.wikipedia.org/wiki/Trim_(computing)) is enabled for NVME and SSD disks in your OS (usually it's implemented using a cronjob or systemd service). From 558b1eb37226b3307d866abc47f7b1abfc665cff Mon Sep 17 00:00:00 2001 From: rfraposa Date: Mon, 3 Apr 2023 19:35:23 -0600 Subject: [PATCH 49/52] Update clickhouse-local.md --- .../operations/utilities/clickhouse-local.md | 146 +++++++++++++++++- 1 file changed, 144 insertions(+), 2 deletions(-) diff --git a/docs/en/operations/utilities/clickhouse-local.md b/docs/en/operations/utilities/clickhouse-local.md index 6bf1269c1d9..a23e0745dec 100644 --- a/docs/en/operations/utilities/clickhouse-local.md +++ b/docs/en/operations/utilities/clickhouse-local.md @@ -8,10 +8,150 @@ sidebar_label: clickhouse-local The `clickhouse-local` program enables you to perform fast processing on local files, without having to deploy and configure the ClickHouse server. It accepts data that represent tables and queries them using [ClickHouse SQL dialect](../../sql-reference/index.md). `clickhouse-local` uses the same core as ClickHouse server, so it supports most of the features and the same set of formats and table engines. -By default `clickhouse-local` has access to data on the same host, and it does not depend on the server's configuration. It also supports loading server configuration using `--config-file` argument. For temporary data, a unique temporary data directory is created by default. +## Download clickhouse-local + +`clickhouse-local` is executed using the same `clickhouse` binary that runs the ClickHouse server and `clickhouse-client`. The easiest way to download the latest version is with the following command: + +```bash +curl https://clickhouse.com/ | sh +``` + +:::note +The binary you just downloaded can run all sorts of ClickHouse tools and utilities. If you want to run ClickHouse as a database server, check out the [Quick Start](../../quick-start.mdx). +::: + +## Query data in a CSV file using SQL + +A common use of `clickhouse-local` is to run ad-hoc queries on files: where you don't have to insert the data into a table. `clickhouse-local` can stream the data from a file into a temporary table and execute your SQL. + +If the file is sitting on the same machine as `clickhouse-local`, use the `file` table engine. The following `reviews.tsv` file contains a sampling of Amazon product reviews: + +```bash +./clickhouse local -q "SELECT * FROM file('reviews.tsv')" +``` + +ClickHouse knows the file uses a tab-separated format from filename extension. If you need to explicitly specify the format, simply add one of the [many ClickHouse input formats](../../interfaces/formats.md): + ```bash + ./clickhouse local -q "SELECT * FROM file('reviews.tsv', 'TabSeparated')" + ``` + +The `file` table function creates a table, and you can use `DESCRIBE` to see the inferred schema: + +```bash +./clickhouse local -q "DESCRIBE file('reviews.tsv')" +``` + +```response +marketplace Nullable(String) +customer_id Nullable(Int64) +review_id Nullable(String) +product_id Nullable(String) +product_parent Nullable(Int64) +product_title Nullable(String) +product_category Nullable(String) +star_rating Nullable(Int64) +helpful_votes Nullable(Int64) +total_votes Nullable(Int64) +vine Nullable(String) +verified_purchase Nullable(String) +review_headline Nullable(String) +review_body Nullable(String) +review_date Nullable(Date) +``` + +Let's find a product with the highest rating: + +```bash +./clickhouse local -q "SELECT + argMax(product_title,star_rating), + max(star_rating) +FROM file('reviews.tsv')" +``` + +```response +Monopoly Junior Board Game 5 +``` + +## Query data in a Parquet file in AWS S3 + +If you have a file in S3, use `clickhouse-local` and the `s3` table function to query the file in place (without inserting the data into a ClickHouse table). We have a file named `house_0.parquet` in a public bucket that contains home prices of property sold in the United Kingdom. Let's see how many rows it has: + +```bash +./clickhouse local -q " +SELECT count() +FROM s3('https://datasets-documentation.s3.eu-west-3.amazonaws.com/house_parquet/house_0.parquet')" +``` + +The file has 2.7M rows: + +```response +2772030 +``` + +It's always useful to see what the inferred schema that ClickHouse determines from the file: + +```bash +./clickhouse local -q "DESCRIBE s3('https://datasets-documentation.s3.eu-west-3.amazonaws.com/house_parquet/house_0.parquet')" +``` + +```response +price Nullable(Int64) +date Nullable(UInt16) +postcode1 Nullable(String) +postcode2 Nullable(String) +type Nullable(String) +is_new Nullable(UInt8) +duration Nullable(String) +addr1 Nullable(String) +addr2 Nullable(String) +street Nullable(String) +locality Nullable(String) +town Nullable(String) +district Nullable(String) +county Nullable(String) +``` + +Let's see what the most expensive neighborhoods are: + +```bash +./clickhouse local -q " +SELECT + town, + district, + count() AS c, + round(avg(price)) AS price, + bar(price, 0, 5000000, 100) +FROM s3('https://datasets-documentation.s3.eu-west-3.amazonaws.com/house_parquet/house_0.parquet') +GROUP BY + town, + district +HAVING c >= 100 +ORDER BY price DESC +LIMIT 10" +``` + +```response +LONDON CITY OF LONDON 886 2271305 █████████████████████████████████████████████▍ +LEATHERHEAD ELMBRIDGE 206 1176680 ███████████████████████▌ +LONDON CITY OF WESTMINSTER 12577 1108221 ██████████████████████▏ +LONDON KENSINGTON AND CHELSEA 8728 1094496 █████████████████████▉ +HYTHE FOLKESTONE AND HYTHE 130 1023980 ████████████████████▍ +CHALFONT ST GILES CHILTERN 113 835754 ████████████████▋ +AMERSHAM BUCKINGHAMSHIRE 113 799596 ███████████████▉ +VIRGINIA WATER RUNNYMEDE 356 789301 ███████████████▊ +BARNET ENFIELD 282 740514 ██████████████▊ +NORTHWOOD THREE RIVERS 184 731609 ██████████████▋ +``` + +:::tip +When you are ready to insert your files into ClickHouse, startup a ClickHouse server and insert the results of your `file` and `s3` table functions into a `MergeTree` table. View the [Quick Start](../../quick-start.mdx) for more details. +::: + ## Usage {#usage} +By default `clickhouse-local` has access to data of a ClickHouse server on the same host, and it does not depend on the server's configuration. It also supports loading server configuration using `--config-file` argument. For temporary data, a unique temporary data directory is created by default. + Basic usage (Linux): ``` bash @@ -24,7 +164,9 @@ Basic usage (Mac): $ ./clickhouse local --structure "table_structure" --input-format "format_of_incoming_data" --query "query" ``` -Also supported on Windows through WSL2. +:::note +`clickhouse-local` is also supported on Windows through WSL2. +::: Arguments: From 3bd29f0aa978a4d20f26a7593b8da0d05614be19 Mon Sep 17 00:00:00 2001 From: taiyang-li <654010905@qq.com> Date: Tue, 4 Apr 2023 10:34:33 +0800 Subject: [PATCH 50/52] fix exception message --- src/Functions/map.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Functions/map.cpp b/src/Functions/map.cpp index 14453de0646..21a4ab8de7d 100644 --- a/src/Functions/map.cpp +++ b/src/Functions/map.cpp @@ -179,7 +179,7 @@ public: if (const auto * keys_type = checkAndGetDataType(arguments[0].get())) key_type = keys_type->getNestedType(); else - throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, "First argument for function {} must be Array or Map", getName()); + throw Exception(ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, "First argument for function {} must be an Array", getName()); DataTypePtr value_type; if (const auto * value_array_type = checkAndGetDataType(arguments[1].get())) From 57a412745d39074cd3e3740706da0e12760eab8c Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Tue, 4 Apr 2023 11:35:29 +0000 Subject: [PATCH 51/52] Update version_date.tsv and changelogs after v22.8.16.32-lts --- docs/changelogs/v22.8.16.32-lts.md | 29 ++++++++++++++++++++++++++++ utils/list-versions/version_date.tsv | 1 + 2 files changed, 30 insertions(+) create mode 100644 docs/changelogs/v22.8.16.32-lts.md diff --git a/docs/changelogs/v22.8.16.32-lts.md b/docs/changelogs/v22.8.16.32-lts.md new file mode 100644 index 00000000000..27ec1f1f145 --- /dev/null +++ b/docs/changelogs/v22.8.16.32-lts.md @@ -0,0 +1,29 @@ +--- +sidebar_position: 1 +sidebar_label: 2023 +--- + +# 2023 Changelog + +### ClickHouse release v22.8.16.32-lts (7c4be737bd0) FIXME as compared to v22.8.15.23-lts (d36fa168bbf) + +#### Build/Testing/Packaging Improvement +* Backported in [#48344](https://github.com/ClickHouse/ClickHouse/issues/48344): Use sccache as a replacement for ccache and using S3 as cache backend. [#46240](https://github.com/ClickHouse/ClickHouse/pull/46240) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Backported in [#48250](https://github.com/ClickHouse/ClickHouse/issues/48250): The `clickhouse/clickhouse-keeper` image used to be pushed only with tags `-alpine`, e.g. `latest-alpine`. As it was suggested in https://github.com/ClickHouse/examples/pull/2, now it will be pushed as suffixless too. [#48236](https://github.com/ClickHouse/ClickHouse/pull/48236) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). + +#### Bug Fix (user-visible misbehavior in an official stable release) + +* Fix bug in zero-copy replication disk choice during fetch [#47010](https://github.com/ClickHouse/ClickHouse/pull/47010) ([alesapin](https://github.com/alesapin)). +* Fix query parameters [#47488](https://github.com/ClickHouse/ClickHouse/pull/47488) ([Alexey Milovidov](https://github.com/alexey-milovidov)). +* Fix wait for zero copy lock during move [#47631](https://github.com/ClickHouse/ClickHouse/pull/47631) ([alesapin](https://github.com/alesapin)). +* Fix crash in polygonsSymDifferenceCartesian [#47702](https://github.com/ClickHouse/ClickHouse/pull/47702) ([pufit](https://github.com/pufit)). +* Backport to 22.8: Fix moving broken parts to the detached for the object storage disk on startup [#48273](https://github.com/ClickHouse/ClickHouse/pull/48273) ([Aleksei Filatov](https://github.com/aalexfvk)). + +#### NOT FOR CHANGELOG / INSIGNIFICANT + +* Add a fuse for backport branches w/o a created PR [#47760](https://github.com/ClickHouse/ClickHouse/pull/47760) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Only valid Reviews.STATES overwrite existing reviews [#47789](https://github.com/ClickHouse/ClickHouse/pull/47789) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Place short return before big block, improve logging [#47822](https://github.com/ClickHouse/ClickHouse/pull/47822) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Artifacts s3 prefix [#47945](https://github.com/ClickHouse/ClickHouse/pull/47945) ([Mikhail f. Shiryaev](https://github.com/Felixoid)). +* Fix tsan error lock-order-inversion [#47953](https://github.com/ClickHouse/ClickHouse/pull/47953) ([Kruglov Pavel](https://github.com/Avogar)). + diff --git a/utils/list-versions/version_date.tsv b/utils/list-versions/version_date.tsv index 24af79eef2f..16ae3007938 100644 --- a/utils/list-versions/version_date.tsv +++ b/utils/list-versions/version_date.tsv @@ -36,6 +36,7 @@ v22.9.4.32-stable 2022-10-26 v22.9.3.18-stable 2022-09-30 v22.9.2.7-stable 2022-09-23 v22.9.1.2603-stable 2022-09-22 +v22.8.16.32-lts 2023-04-04 v22.8.15.23-lts 2023-03-10 v22.8.14.53-lts 2023-02-27 v22.8.13.20-lts 2023-01-29 From 2d4fbdf4b0cd60063f6b0eceb25ad2369d0d3607 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Tue, 4 Apr 2023 14:27:58 +0200 Subject: [PATCH 52/52] add script for a slack bot --- utils/ci-slack-bot/ci-slack-bot.py | 179 +++++++++++++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100755 utils/ci-slack-bot/ci-slack-bot.py diff --git a/utils/ci-slack-bot/ci-slack-bot.py b/utils/ci-slack-bot/ci-slack-bot.py new file mode 100755 index 00000000000..83b6d8aad6b --- /dev/null +++ b/utils/ci-slack-bot/ci-slack-bot.py @@ -0,0 +1,179 @@ +#!/usr/bin/env python3 + +# A trivial stateless slack bot that notifies about new broken tests in ClickHouse CI. +# It checks what happened to our CI during the last check_period hours (1 hour) and notifies us in slack if necessary. +# This script should be executed once each check_period hours (1 hour). +# It will post duplicate messages if you run it more often; it will lose some messages if you run it less often. +# +# You can run it locally with no arguments, it will work in a dry-run mode. Or you can set your own SLACK_URL_DEFAULT. +# Feel free to add more checks, more details to messages, or better heuristics. +# NOTE There's no deployment automation for now, +# an AWS Lambda (slack-ci-bot-test lambda in CI-CD) has to be updated manually after changing this script. +# +# See also: https://aretestsgreenyet.com/ + +import os +import json +import base64 + +if os.environ.get("AWS_LAMBDA_ENV", "0") == "1": + # For AWS labmda (python 3.7) + from botocore.vendored import requests +else: + # For running locally + import requests + +DRY_RUN_MARK = "" + +MAX_FAILURES_DEFAULT = 50 +SLACK_URL_DEFAULT = DRY_RUN_MARK + +# Find tests that failed in master during the last check_period hours, +# but did not fail during the last 2 weeks. Assuming these tests were broken recently. +# NOTE: It may report flaky tests that fail too rarely. +NEW_BROKEN_TESTS_QUERY = """ +WITH + 1 AS check_period, + now() as now +SELECT test_name, any(report_url) +FROM checks +WHERE 1 + AND check_start_time >= now - INTERVAL 1 WEEK + AND (check_start_time + check_duration_ms / 1000) >= now - INTERVAL check_period HOUR + AND pull_request_number = 0 + AND test_status LIKE 'F%' + AND check_status != 'success' + AND test_name NOT IN ( + SELECT test_name FROM checks WHERE 1 + AND check_start_time >= now - INTERVAL 1 MONTH + AND (check_start_time + check_duration_ms / 1000) BETWEEN now - INTERVAL 2 WEEK AND now - INTERVAL check_period HOUR + AND pull_request_number = 0 + AND check_status != 'success' + AND test_status LIKE 'F%') + AND test_context_raw NOT LIKE '%CannotSendRequest%' and test_context_raw NOT LIKE '%Server does not respond to health check%' +GROUP BY test_name +""" + +# Returns total number of failed checks during the last 24 hours +# and previous value of that metric (check_period hours ago) +COUNT_FAILURES_QUERY = """ +WITH + 1 AS check_period, + '%' AS check_name_pattern, + now() as now +SELECT + countIf((check_start_time + check_duration_ms / 1000) >= now - INTERVAL 24 HOUR) AS new_val, + countIf((check_start_time + check_duration_ms / 1000) <= now - INTERVAL check_period HOUR) AS prev_val +FROM checks +WHERE 1 + AND check_start_time >= now - INTERVAL 1 WEEK + AND (check_start_time + check_duration_ms / 1000) >= now - INTERVAL 24 + check_period HOUR + AND pull_request_number = 0 + AND test_status LIKE 'F%' + AND check_status != 'success' + AND check_name ILIKE check_name_pattern +""" + +SLACK_MESSAGE_JSON = {"type": "mrkdwn", "text": None} + + +def get_play_url(query): + return ( + "https://play.clickhouse.com/play?user=play#" + + base64.b64encode(query.encode()).decode() + ) + + +def run_clickhouse_query(query): + url = "https://play.clickhouse.com/?user=play&query=" + requests.utils.quote(query) + res = requests.get(url) + if res.status_code != 200: + print("Failed to execute query: ", res.status_code, res.content) + raise Exception( + "Failed to execute query: {}: {}".format(res.status_code, res.content) + ) + + lines = res.text.strip().splitlines() + return [x.split("\t") for x in lines] + + +def get_new_broken_tests_message(broken_tests): + if not broken_tests: + return None + msg = "There are {} new broken tests in master:\n".format(len(broken_tests)) + for name, report in broken_tests: + msg += " - *{}* - <{}|Report>\n".format(name, report) + return msg + + +def get_too_many_failures_message(failures_count): + MAX_FAILURES = int(os.environ.get("MAX_FAILURES", MAX_FAILURES_DEFAULT)) + curr_failures = int(failures_count[0][0]) + prev_failures = int(failures_count[0][1]) + if curr_failures == 0: + return ( + "Looks like CI is completely broken: there are *no failures* at all... 0_o" + ) + if curr_failures < MAX_FAILURES: + return None + if prev_failures < MAX_FAILURES: + return "*CI is broken: there are {} failures during the last 24 hours*".format( + curr_failures + ) + if curr_failures < prev_failures: + return None + if (curr_failures - prev_failures) / prev_failures < 0.2: + return None + return "CI is broken and it's getting worse: there are {} failures during the last 24 hours".format( + curr_failures + ) + + +def send_to_slack(message): + SLACK_URL = os.environ.get("SLACK_URL", SLACK_URL_DEFAULT) + if SLACK_URL == DRY_RUN_MARK: + return + + payload = SLACK_MESSAGE_JSON.copy() + payload["text"] = message + res = requests.post(SLACK_URL, json.dumps(payload)) + if res.status_code != 200: + print("Failed to send a message to Slack: ", res.status_code, res.content) + raise Exception( + "Failed to send a message to Slack: {}: {}".format( + res.status_code, res.content + ) + ) + + +def query_and_alert_if_needed(query, get_message_func): + query_res = run_clickhouse_query(query) + print("Got result {} for query {}", query_res, query) + msg = get_message_func(query_res) + if msg is None: + return + + msg += "\nCI DB query: <{}|link>".format(get_play_url(query)) + print("Sending message to slack:", msg) + send_to_slack(msg) + + +def check_and_alert(): + query_and_alert_if_needed(NEW_BROKEN_TESTS_QUERY, get_new_broken_tests_message) + query_and_alert_if_needed(COUNT_FAILURES_QUERY, get_too_many_failures_message) + + +def lambda_handler(event, context): + try: + check_and_alert() + return {"statusCode": 200, "body": "OK"} + except Exception as e: + send_to_slack( + "I failed, please help me (see ClickHouse/utils/ci-slack-bot/ci-slack-bot.py): " + + str(e) + ) + return {"statusCode": 200, "body": "FAIL"} + + +if __name__ == "__main__": + check_and_alert()