-
Notifications
You must be signed in to change notification settings - Fork 17
Antalya 26.1: Remote initiator improvements #1577
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
base: antalya-26.1
Are you sure you want to change the base?
Changes from 7 commits
000a737
d4b850d
0c95253
8dab965
a0d1972
d7c4bee
df2595a
d8b6b82
259fb32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,11 +49,10 @@ namespace Setting | |
| extern const SettingsBool async_query_sending_for_remote; | ||
| extern const SettingsBool async_socket_for_remote; | ||
| extern const SettingsBool skip_unavailable_shards; | ||
| extern const SettingsBool parallel_replicas_local_plan; | ||
| extern const SettingsString cluster_for_parallel_replicas; | ||
| extern const SettingsNonZeroUInt64 max_parallel_replicas; | ||
| extern const SettingsUInt64 object_storage_max_nodes; | ||
| extern const SettingsBool object_storage_remote_initiator; | ||
| extern const SettingsString object_storage_remote_initiator_cluster; | ||
| extern const SettingsObjectStorageClusterJoinMode object_storage_cluster_join_mode; | ||
| } | ||
|
|
||
|
|
@@ -330,8 +329,6 @@ void IStorageCluster::read( | |
|
|
||
| const auto & settings = context->getSettingsRef(); | ||
|
|
||
| auto cluster = getClusterImpl(context, cluster_name_from_settings, isObjectStorage() ? settings[Setting::object_storage_max_nodes] : 0); | ||
|
|
||
| /// Calculate the header. This is significant, because some columns could be thrown away in some cases like query with count(*) | ||
|
|
||
| SharedHeader sample_block; | ||
|
|
@@ -354,7 +351,11 @@ void IStorageCluster::read( | |
|
|
||
| if (settings[Setting::object_storage_remote_initiator]) | ||
| { | ||
| auto storage_and_context = convertToRemote(cluster, context, cluster_name_from_settings, query_to_send); | ||
| auto remote_initiator_cluster_name = settings[Setting::object_storage_remote_initiator_cluster].value; | ||
| if (remote_initiator_cluster_name.empty()) | ||
| remote_initiator_cluster_name = cluster_name_from_settings; | ||
| auto remote_initiator_cluster = getClusterImpl(context, remote_initiator_cluster_name); | ||
| auto storage_and_context = convertToRemote(remote_initiator_cluster, context, remote_initiator_cluster_name, query_to_send); | ||
|
Comment on lines
+362
to
+366
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ianton-ru does it make sense? it looks like it does
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| auto src_distributed = std::dynamic_pointer_cast<StorageDistributed>(storage_and_context.storage); | ||
| auto modified_query_info = query_info; | ||
| modified_query_info.cluster = src_distributed->getCluster(); | ||
|
|
@@ -363,6 +364,8 @@ void IStorageCluster::read( | |
| return; | ||
| } | ||
|
|
||
| auto cluster = getClusterImpl(context, cluster_name_from_settings, isObjectStorage() ? settings[Setting::object_storage_max_nodes] : 0); | ||
|
|
||
| RestoreQualifiedNamesVisitor::Data data; | ||
| data.distributed_table = DatabaseAndTableWithAlias(*getTableExpression(query_to_send->as<ASTSelectQuery &>(), 0)); | ||
| data.remote_table.database = context->getCurrentDatabase(); | ||
|
|
@@ -396,6 +399,10 @@ IStorageCluster::RemoteCallVariables IStorageCluster::convertToRemote( | |
| const std::string & cluster_name_from_settings, | ||
| ASTPtr query_to_send) | ||
| { | ||
| /// TODO: Allow to use secret for remote queries | ||
| if (!cluster->getSecret().empty()) | ||
| throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Can't convert query to remote when cluster uses secret"); | ||
|
|
||
| auto host_addresses = cluster->getShardsAddresses(); | ||
| if (host_addresses.empty()) | ||
| throw Exception(ErrorCodes::LOGICAL_ERROR, "Empty cluster {}", cluster_name_from_settings); | ||
|
|
@@ -436,7 +443,18 @@ IStorageCluster::RemoteCallVariables IStorageCluster::convertToRemote( | |
| if (!table_expression) | ||
| throw Exception(ErrorCodes::LOGICAL_ERROR, "Can't find table expression"); | ||
|
|
||
| auto remote_query = makeASTFunction(remote_function_name, make_intrusive<ASTLiteral>(host_name), table_expression->table_function); | ||
| boost::intrusive_ptr<ASTFunction> remote_query; | ||
|
|
||
| if (shard_addresses[0].user_specified) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment please |
||
| { | ||
| remote_query = makeASTFunction(remote_function_name, | ||
| make_intrusive<ASTLiteral>(host_name), | ||
| table_expression->table_function, | ||
| make_intrusive<ASTLiteral>(shard_addresses[0].user), | ||
| make_intrusive<ASTLiteral>(shard_addresses[0].password)); | ||
| } | ||
| else | ||
| remote_query = makeASTFunction(remote_function_name, make_intrusive<ASTLiteral>(host_name), table_expression->table_function); | ||
|
|
||
| table_expression->table_function = remote_query; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,9 +75,11 @@ class ITableFunctionCluster : public Base | |
|
|
||
| /// Cluster name is always the first | ||
| cluster_name = checkAndGetLiteralArgument<String>(args[0], "cluster_name"); | ||
|
|
||
| if (!context->tryGetCluster(cluster_name)) | ||
| throw Exception(ErrorCodes::CLUSTER_DOESNT_EXIST, "Requested cluster '{}' not found", cluster_name); | ||
| /// Remove check cluster existing here | ||
| /// In query like | ||
| /// remote('remote_host', xxxCluster('remote_cluster', ...)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the query is not
Where and with which exception? It would be good to avoid any network calls before failing
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cluster name is not a network node name, it's an internal ClickHouse name. Query falls later when tries to get hosts from cluster. Network calls can't be made without hosts. But it's hard to understand here is cluster function inside 'remote' or not. |
||
| /// 'remote_cluster' can be defined only on 'remote_host' | ||
| /// If cluster not exists, query falls later | ||
|
|
||
| /// Just cut the first arg (cluster_name) and try to parse other table function arguments as is | ||
| args.erase(args.begin()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| <clickhouse> | ||
| <remote_servers> | ||
| <hidden_cluster_with_username_and_password> | ||
| <shard> | ||
| <replica> | ||
| <host>s0_0_1</host> | ||
| <port>9000</port> | ||
| <user>foo</user> | ||
| <password>bar</password> | ||
| </replica> | ||
| <replica> | ||
| <host>s0_1_0</host> | ||
| <port>9000</port> | ||
| <user>foo</user> | ||
| <password>bar</password> | ||
| </replica> | ||
| </shard> | ||
| </hidden_cluster_with_username_and_password> | ||
| </remote_servers> | ||
| </clickhouse> |
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.
Please add a comment explaining what this code block does. It took me a while to understand it by just reading the code.
I suggest something like: