When the configuration file changes, reload the connection pool of the distributed table#25768
Closed
ljcui wants to merge 2 commits intoClickHouse:masterfrom
Closed
When the configuration file changes, reload the connection pool of the distributed table#25768ljcui wants to merge 2 commits intoClickHouse:masterfrom
ljcui wants to merge 2 commits intoClickHouse:masterfrom
Conversation
robot-clickhouse
added
the
pr-improvement
azat
reviewed
Jun 28, 2021
src/Storages/StorageDistributed.cpp
Outdated
Comment on lines
939
to
941
Member
There was a problem hiding this comment.
Suggested change
| std::lock_guard lock(cluster_nodes_mutex); | |
| for (auto & node : cluster_nodes_data) | |
| node.second.directory_monitor->requestUpdatePool(); | |
| std::lock_guard lock(cluster_nodes_mutex); | |
| for (auto & node : cluster_nodes_data) | |
| node.second.directory_monitor->requestUpdatePool(); |
Lots of coding style issues here and in other hunks of the patch.
Author
There was a problem hiding this comment.
Thanks for pointing out the problem, I will modify it later
Member
There was a problem hiding this comment.
Suggested change
| need_update_pool= true; | |
| need_update_pool = true; |
Member
There was a problem hiding this comment.
The major comment is that this patch will reload the pool only for use_compact_format_in_distributed_parts_names=1 and internal_replication=true, so it will reload only replicas (I'm not sure that it is OK, plus at least description does not say anything about this).
Member
There was a problem hiding this comment.
Suggested change
| if (need_update_pool) { | |
| if (need_update_pool) | |
| { |
programs/server/Server.cpp
Outdated
Member
There was a problem hiding this comment.
So update will happen on any config reload regardless was cluster definition had been changed or not, right?
Author
There was a problem hiding this comment.
seems that the isSameConfiguration function can be used to judge in advance, and update only when the remote_servers changes
alexey-milovidov
changed the title
…e distributed table
ljcui
force-pushed
the
fix_distributed_table
branch
from
f5fe2fc to
fd4ab25
Compare
Member
|
It fails CI checks, some NULL derefence: Details |
azat
reviewed
Jun 30, 2021
Comment on lines
+431
to
+442
| if (!pool) | ||
| { | ||
| pool = StorageDistributedDirectoryMonitor::createPool(name, storage); | ||
| } | ||
| else | ||
| { | ||
| if (need_update_pool) | ||
| { | ||
| need_update_pool = false; | ||
| pool = StorageDistributedDirectoryMonitor::createPool(name, storage); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
This can be written in a single branch, i.e.
Suggested change
| if (!pool) | |
| { | |
| pool = StorageDistributedDirectoryMonitor::createPool(name, storage); | |
| } | |
| else | |
| { | |
| if (need_update_pool) | |
| { | |
| need_update_pool = false; | |
| pool = StorageDistributedDirectoryMonitor::createPool(name, storage); | |
| } | |
| } | |
| if (!pool || need_update_pool) | |
| { | |
| pool = StorageDistributedDirectoryMonitor::createPool(name, storage); | |
| need_update_pool = false; | |
| } |
Or if it was splitted, worth adding some log.
| auto distributed_table = dynamic_cast<StorageDistributed *>(table.get()); | ||
| if (!distributed_table) | ||
| continue; | ||
| distributed_table->updateConnectionPool(); |
Member
There was a problem hiding this comment.
Actually this is a bit uncontrolled, when I though about this my concern was that cluster definition can be changed at any time, and once it was changed it should atomically sync all existing pending blocks for distributed send and then re-create all directory monitors.
Not sure, but maybe making this asynchronously is enough...
| void StorageDistributedDirectoryMonitor::requestUpdatePool() | ||
| { | ||
| const char * user_pw_end = strchr(name.data(), '@'); | ||
| if (!user_pw_end && startsWith(name, "shard")) |
Member
There was a problem hiding this comment.
I don't think that re-creating pool is enough, since changing the cluster definition can change the destination server, but you may still have pending blocks.
So suppose you have the following cluster:
<remote_servers>
<main_cluster>
<shard>
<internal_replication>true</internal_replication>
<replica>
<host>foo-1</host>
<port>9000</port>
</replica>
<replica>
<host>foo-2</host>
<port>9000</port>
</replica>
</shard>
<shard>
<internal_replication>true</internal_replication>
<replica>
<host>bar-1</host>
<port>9000</port>
</replica>
<replica>
<host>bar-2</host>
<port>9000</port>
</replica>
</shard>
</main_cluster>
</remote_servers>And the foo* shard removed from the cluster definition.
In this case shard1 will became bar* not foo*, but all pending blocks should be sent to foo* not bar*, and during this time any INSERT should be blocked since you cannot write new blocks until you sync old one.
And also there are some trickery with internal_replication=false.
So that said that it is not that simple, and it looks to me that it is better to re-create DirectryMonitor objects, plus it requires good integration test, that will cover all possible variants.
|
ZhiYong Wang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Member
|
Please resubmit. |
Member
|
@azat is was abandoned. |
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
7 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.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
When the address of remote_servers in the configuration file changes, reload the connection pool of the distributed table using the new compact format
Detailed description / Documentation draft:
Use configuration item
use_compact_format_in_distributed_parts_names, set to 1.The directory name of distributed table uses the new compact format，like this
shard1_replica1orshard1_all_replicas.When the address of remote_servers in the configuration file changes, the address in the connection pool of distributed table is still old and must be restarted at this time to take effect.
This pr enables the connection pool to be automatically updated when the configuration file is reloaded.