New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip unavailable replicas in parallel distributed insert select #58931
Conversation
This is an automated comment for commit 21e089b with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
Not sure if we should add another setting (I don't like using |
Although, look like we already use |
Well, we explicitly set ClickHouse/src/Storages/IStorageCluster.cpp Lines 218 to 219 in 67f74b5
So, we if user changes skip_unavailable_shards it doesn't affect *Cluster functions. Also, I think we can change the code in IStorageCluster and don't copy settings with changed skip_unavailable_shards=true but just use optional skip_unavailable_endpoints argument of ConnectionPoolWithFailover::getMany .
But I guess we still use |
It's fine because |
Integration tests (tsan) [3/6] - test_parallel_replicas_custom_key_failover was broken in master, reverted |
{ | ||
/// Skip unavailable hosts if necessary | ||
auto try_results = replicas.pool->getMany(timeouts, current_settings, PoolMode::GET_MANY, /*async_callback*/ {}, /*skip_unavailable_endpoints*/ true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the comment at line 1152 is correct. Then PoolMode::GET_ONE
is sufficient here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the comment is either incorrect or misleading (because we get the cluster from StorageDistributed::getCluster()
), we can ask @nikitamikhaylov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's copy-paste from
ClickHouse/src/Storages/StorageReplicatedMergeTree.cpp
Lines 5562 to 5565 in 4315d44
for (const auto & replicas : src_cluster->getShardsAddresses()) | |
{ | |
/// There will be only one replica, because we consider each replica as a shard | |
for (const auto & node : replicas) |
And looks like we have exactly the same issue in StorageReplicatedMergeTree::distributedWriteFromClusterStorage
(and that's why we should avoid copy-paste when possible)
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Skip unavailable replicas when executing parallel distributed
INSERT SELECT