From 58e415c7d42066ff478c1409e9bd7b84c34d044e Mon Sep 17 00:00:00 2001 From: Pavel Kruglov Date: Sat, 13 Mar 2021 00:17:19 +0300 Subject: [PATCH 1/3] Update clusters only if their configs were updated --- .../AbstractConfigurationComparison.cpp | 4 + .../Config/AbstractConfigurationComparison.h | 5 + src/Interpreters/Cluster.cpp | 38 ++- src/Interpreters/Cluster.h | 2 +- src/Interpreters/Context.cpp | 9 +- .../configs/remote_servers.xml | 30 +++ .../test_reload_clusters_config/test.py | 235 ++++++++++++++++++ 7 files changed, 315 insertions(+), 8 deletions(-) create mode 100644 tests/integration/test_reload_clusters_config/configs/remote_servers.xml create mode 100644 tests/integration/test_reload_clusters_config/test.py diff --git a/src/Common/Config/AbstractConfigurationComparison.cpp b/src/Common/Config/AbstractConfigurationComparison.cpp index 0e603cb1056..59c0c895a89 100644 --- a/src/Common/Config/AbstractConfigurationComparison.cpp +++ b/src/Common/Config/AbstractConfigurationComparison.cpp @@ -26,6 +26,10 @@ bool isSameConfiguration(const Poco::Util::AbstractConfiguration & left, const P return isSameConfiguration(left, String(), right, String()); } +bool isSameConfiguration(const Poco::Util::AbstractConfiguration & left, const Poco::Util::AbstractConfiguration & right, const String & key) +{ + return isSameConfiguration(left, key, right, key); +} bool isSameConfiguration(const Poco::Util::AbstractConfiguration & left, const String & left_key, const Poco::Util::AbstractConfiguration & right, const String & right_key) diff --git a/src/Common/Config/AbstractConfigurationComparison.h b/src/Common/Config/AbstractConfigurationComparison.h index f825ad4e53d..795fca2af8e 100644 --- a/src/Common/Config/AbstractConfigurationComparison.h +++ b/src/Common/Config/AbstractConfigurationComparison.h @@ -13,6 +13,11 @@ namespace DB bool isSameConfiguration(const Poco::Util::AbstractConfiguration & left, const Poco::Util::AbstractConfiguration & right); + /// Returns true if the specified subview of the two configurations contains the same keys and values. + bool isSameConfiguration(const Poco::Util::AbstractConfiguration & left, + const Poco::Util::AbstractConfiguration & right, + const String & key); + /// Returns true if specified subviews of the two configurations contains the same keys and values. bool isSameConfiguration(const Poco::Util::AbstractConfiguration & left, const String & left_key, const Poco::Util::AbstractConfiguration & right, const String & right_key); diff --git a/src/Interpreters/Cluster.cpp b/src/Interpreters/Cluster.cpp index fb9788e84c4..b77d5019d48 100644 --- a/src/Interpreters/Cluster.cpp +++ b/src/Interpreters/Cluster.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -265,20 +266,45 @@ void Clusters::setCluster(const String & cluster_name, const std::shared_ptrkeys(config_prefix, old_config_keys); + std::sort(old_config_keys.begin(), old_config_keys.end()); + + std::set_difference( + old_config_keys.begin(), old_config_keys.end(), new_config_keys.begin(), new_config_keys.end(), std::back_inserter(deleted_keys)); + } std::lock_guard lock(mutex); - impl.clear(); - for (const auto & key : config_keys) + /// If old congig is set, remove deleted clusters from impl, otherwise just clear it. + if (old_config) + { + for (const auto & key : deleted_keys) + impl.erase(key); + } + else + impl.clear(); + + for (const auto & key : new_config_keys) { if (key.find('.') != String::npos) throw Exception("Cluster names with dots are not supported: '" + key + "'", ErrorCodes::SYNTAX_ERROR); - impl.emplace(key, std::make_shared(config, settings, config_prefix, key)); + /// If old config is set and cluster config wasn't changed, don't update this cluster. + if (!old_config || !isSameConfiguration(new_config, *old_config, config_prefix + "." + key)) + impl[key] = std::make_shared(new_config, settings, config_prefix, key); } } diff --git a/src/Interpreters/Cluster.h b/src/Interpreters/Cluster.h index c64d52724e5..a047f199204 100644 --- a/src/Interpreters/Cluster.h +++ b/src/Interpreters/Cluster.h @@ -276,7 +276,7 @@ public: ClusterPtr getCluster(const std::string & cluster_name) const; void setCluster(const String & cluster_name, const ClusterPtr & cluster); - void updateClusters(const Poco::Util::AbstractConfiguration & config, const Settings & settings, const String & config_prefix); + void updateClusters(const Poco::Util::AbstractConfiguration & new_config, const Settings & settings, const String & config_prefix, Poco::Util::AbstractConfiguration * old_config = nullptr); public: using Impl = std::map; diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 8615cf70343..bb2d553b8e8 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -60,6 +60,7 @@ #include #include #include +#include #include #include #include @@ -1833,12 +1834,18 @@ void Context::setClustersConfig(const ConfigurationPtr & config, const String & { std::lock_guard lock(shared->clusters_mutex); + /// Do not update clusters if this part of config wasn't changed. + if (shared->clusters && isSameConfiguration(*config, *shared->clusters_config, config_name)) { + return; + } + + auto old_clusters_config = shared->clusters_config; shared->clusters_config = config; if (!shared->clusters) shared->clusters = std::make_unique(*shared->clusters_config, settings, config_name); else - shared->clusters->updateClusters(*shared->clusters_config, settings, config_name); + shared->clusters->updateClusters(*shared->clusters_config, settings, config_name, old_clusters_config); } diff --git a/tests/integration/test_reload_clusters_config/configs/remote_servers.xml b/tests/integration/test_reload_clusters_config/configs/remote_servers.xml new file mode 100644 index 00000000000..b827fce02be --- /dev/null +++ b/tests/integration/test_reload_clusters_config/configs/remote_servers.xml @@ -0,0 +1,30 @@ + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + diff --git a/tests/integration/test_reload_clusters_config/test.py b/tests/integration/test_reload_clusters_config/test.py new file mode 100644 index 00000000000..f1fb0d820d4 --- /dev/null +++ b/tests/integration/test_reload_clusters_config/test.py @@ -0,0 +1,235 @@ +import os +import sys +import time + +import pytest + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +from helpers.cluster import ClickHouseCluster +from helpers.network import PartitionManager +from helpers.test_tools import TSV + +cluster = ClickHouseCluster(__file__) +node = cluster.add_instance('node', with_zookeeper=True, main_configs=['configs/remote_servers.xml']) +node_1 = cluster.add_instance('node_1', with_zookeeper=True) +node_2 = cluster.add_instance('node_2', with_zookeeper=True) + +@pytest.fixture(scope="module") +def started_cluster(): + try: + cluster.start() + + node.query('''CREATE TABLE distributed (id UInt32) ENGINE = + Distributed('test_cluster', 'default', 'replicated')''') + + node.query('''CREATE TABLE distributed2 (id UInt32) ENGINE = + Distributed('test_cluster2', 'default', 'replicated')''') + + cluster.pause_container('node_1') + cluster.pause_container('node_2') + + yield cluster + + finally: + cluster.shutdown() + + +base_config = ''' + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + +''' + +test_config1 = ''' + + + + + true + + node_1 + 9000 + + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + +''' + +test_config2 = ''' + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + +''' + +test_config3 = ''' + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + + true + + node_1 + 9000 + + + node_2 + 9000 + + + + + + true + + node_1 + 9000 + + + + + +''' + + +def send_repeated_query(table, count=5): + for i in range(count): + node.query_and_get_error("SELECT count() FROM {} SETTINGS receive_timeout=1".format(table)) + + +def get_errors_count(cluster, host_name="node_1"): + return int(node.query("SELECT errors_count FROM system.clusters WHERE cluster='{}' and host_name='{}'".format(cluster, host_name))) + + +def set_config(config): + node.replace_config("/etc/clickhouse-server/config.d/remote_servers.xml", config) + node.query("SYSTEM RELOAD CONFIG") + + +def test_simple_reload(started_cluster): + send_repeated_query("distributed") + + assert get_errors_count("test_cluster") > 0 + + node.query("SYSTEM RELOAD CONFIG") + + assert get_errors_count("test_cluster") > 0 + + +def test_update_one_cluster(started_cluster): + send_repeated_query("distributed") + send_repeated_query("distributed2") + + assert get_errors_count("test_cluster") > 0 + assert get_errors_count("test_cluster2") > 0 + + set_config(test_config1) + + assert get_errors_count("test_cluster") == 0 + assert get_errors_count("test_cluster2") > 0 + + set_config(base_config) + + +def test_delete_cluster(started_cluster): + send_repeated_query("distributed") + send_repeated_query("distributed2") + + assert get_errors_count("test_cluster") > 0 + assert get_errors_count("test_cluster2") > 0 + + set_config(test_config2) + + assert get_errors_count("test_cluster") > 0 + + result = node.query("SELECT * FROM system.clusters WHERE cluster='test_cluster2'") + assert result == '' + + set_config(base_config) + + +def test_add_cluster(started_cluster): + send_repeated_query("distributed") + send_repeated_query("distributed2") + + assert get_errors_count("test_cluster") > 0 + assert get_errors_count("test_cluster2") > 0 + + set_config(test_config3) + + assert get_errors_count("test_cluster") > 0 + assert get_errors_count("test_cluster2") > 0 + + result = node.query("SELECT * FROM system.clusters WHERE cluster='test_cluster3'") + assert result != '' + + set_config(base_config) + From ee483d89e5887d6e8494c37f8896082d9af048d0 Mon Sep 17 00:00:00 2001 From: Pavel Kruglov Date: Sat, 13 Mar 2021 23:16:24 +0300 Subject: [PATCH 2/3] Fix style --- src/Interpreters/Context.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index bb2d553b8e8..1f2da5c3946 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -1835,9 +1835,8 @@ void Context::setClustersConfig(const ConfigurationPtr & config, const String & std::lock_guard lock(shared->clusters_mutex); /// Do not update clusters if this part of config wasn't changed. - if (shared->clusters && isSameConfiguration(*config, *shared->clusters_config, config_name)) { + if (shared->clusters && isSameConfiguration(*config, *shared->clusters_config, config_name)) return; - } auto old_clusters_config = shared->clusters_config; shared->clusters_config = config; From 3e3b5c64bf33c6777d4df0113fda0d9beeaac6f4 Mon Sep 17 00:00:00 2001 From: Pavel Kruglov Date: Sun, 14 Mar 2021 02:34:32 +0300 Subject: [PATCH 3/3] add init file --- tests/integration/test_reload_clusters_config/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/integration/test_reload_clusters_config/__init__.py diff --git a/tests/integration/test_reload_clusters_config/__init__.py b/tests/integration/test_reload_clusters_config/__init__.py new file mode 100644 index 00000000000..e69de29bb2d