Autodiscovery dynamic clusters#76001
Merged
vdimir merged 10 commits intoClickHouse:masterfrom Mar 12, 2025
Merged
Conversation
ianton-ru
force-pushed
the
dynamic_clusters_autodicovery
branch
from
090ef8f to
df89516
Compare
vdimir
added
the
can be tested
Contributor
ianton-ru
force-pushed
the
dynamic_clusters_autodicovery
branch
from
3df8f59 to
04371dd
Compare
ianton-ru
force-pushed
the
dynamic_clusters_autodicovery
branch
from
04371dd to
03df132
Compare
ianton-ru
force-pushed
the
dynamic_clusters_autodicovery
branch
2 times, most recently
from
a32ee4c to
31ebcaa
Compare
ianton-ru
force-pushed
the
dynamic_clusters_autodicovery
branch
from
31ebcaa to
98f9943
Compare
vdimir
reviewed
Feb 25, 2025
Member
vdimir
left a comment
vdimir
left a comment
There was a problem hiding this comment.
Overall, this looks good, thanks for your contribution! The main suggestion is to update the documentation and review some of the other comments.
Comment on lines
-56
to
-59
| /* | ||
| * Holds boolean flags for fixed set of keys. | ||
| * Flags can be concurrently set from different threads, and consumer can wait for it. | ||
| */ |
Member
There was a problem hiding this comment.
Consider updating a comment rather than removing
| /// Reads data from zookeeper and tries to update cluster. | ||
| /// Returns true on success (or no update required). | ||
| bool ClusterDiscovery::updateCluster(ClusterInfo & cluster_info) | ||
| bool ClusterDiscovery::upsertCluster(ClusterInfo & cluster_info) |
Member
There was a problem hiding this comment.
Can we also update a comment that this method can also create new cluster or remove it
Comment on lines
+427
to
+432
| { | ||
| std::lock_guard lock(mutex); | ||
| cluster_impls.erase(name); | ||
| } | ||
| clusters_to_update->remove(name); | ||
| LOG_DEBUG(log, "Dynamic cluster '{}' removed successfully", name); |
Member
There was a problem hiding this comment.
If we put clusters_to_update->remove(name); before cluster_impls.erase(name); would it solve issue with passing cluster_info.name to removeCluster ? If yes, are there other strong reason to order operations or not to like this?
Contributor
Author
There was a problem hiding this comment.
No, it can be in any order.
I did not remove record from clusters_info map, it's possible that at some moment we get cluster without nodes, when all nodes goes off, but we need to keep watch on this node in Keeper to restore cluster when nodes come back online. name is taken from clusters_info.
| String zk_root = zkutil::extractZooKeeperPath(zk_name_and_root, true); | ||
| String zk_name = zkutil::extractZooKeeperName(zk_name_and_root); | ||
| String zk_name_and_root = config.getString(cluster_config_prefix + ".path", ""); | ||
| String zk_multicluster_name_and_root = config.getString(cluster_config_prefix + ".multicluster_root_path", ""); |
Member
There was a problem hiding this comment.
The option names multicluster_root_path and path seem a bit inconsistent to me. I would expect both to be either path/multicluster_path or root_path/multicluster_root_path. Or is it that multicluster_root_path corresponds to multiple clusters, meaning we would need several <path> entries to define them? If so, probably makes sense.
Member
There was a problem hiding this comment.
And could you also update documentation https://github.com/ClickHouse/ClickHouse/blob/master/docs/en/operations/cluster-discovery.md accordingly with the new feature, please?
Contributor
Author
There was a problem hiding this comment.
<path> contains Keeper node for specific cluster, like /clickhouse/discovery/my_cluster.
<multicluster_root_path > contains parent node, root for multiple clusters, like /clickhouse/discovery, and each child subnode contains separate cluster.
So Keeper can have nodes
/clickhouse/discovery/my_cluster
/clickhouse/discovery/my_cluster_2
/clickhouse/discovery/my_cluster_3
and all clusters my_cluster, my_cluster_2, my_cluster_3 will be discovered.
| my_discovery_paths = multicluster_discovery_paths | ||
| ](auto) | ||
| { | ||
| (*my_discovery_paths)[zk_root_index - 1]->need_update = true; |
Member
There was a problem hiding this comment.
Suggested change
| (*my_discovery_paths)[zk_root_index - 1]->need_update = true; | |
| my_discovery_paths->at(zk_root_index - 1)->need_update = true; |
ianton-ru
force-pushed
the
dynamic_clusters_autodicovery
branch
2 times, most recently
from
b166f38 to
c5e7391
Compare
ianton-ru
force-pushed
the
dynamic_clusters_autodicovery
branch
from
c5e7391 to
1428036
Compare
vdimir
approved these changes
Mar 11, 2025
Merged
via the queue into
ClickHouse:master
with commit Mar 12, 2025
45ef349
124 of 125 checks passed
robot-ch-test-poll1
added
the
pr-synced-to-cloud
Member
|
@ianton-ru Hi, there is a data-race - https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=78123&sha=99e652c97c30e00334a988387f6fa21f4711bce0&name_0=PR&name_1=Integration%20tests%20%28tsan%2C%201%2F6%29 Can you please take a look? |
Contributor
Author
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
<multicluster_root_path>.Documentation entry for user-facing changes
ClickHouse has a feature to discovery nodes in cluster (see setting
allow_experimental_cluster_discovery).This PR add ability to discover new clusters.
Nodes in new clusters keep the same way to register in Keeper
Added ability to discover all clusters in Keeper node