Antalya 26.1: Remote initiator improvements#1577
Antalya 26.1: Remote initiator improvements#1577ianton-ru wants to merge 9 commits intoantalya-26.1from
Conversation
|
@codex review |
Audit: PR #1577 — Antalya 26.1: Remote initiator improvementsSource: Altinity/ClickHouse#1577 Confirmed defectsNone. No confirmed defects in reviewed scope. (Earlier revision briefly introduced an unused Coverage summary
Audit metadata
AI-generated audit note. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0d1972000
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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); |
There was a problem hiding this comment.
Strip initiator-only cluster setting before forwarding query
When object_storage_remote_initiator_cluster is set, the query forwarded via remote(...) still carries that setting even though convertToRemote only clears object_storage_remote_initiator. This makes remote execution depend on remote nodes understanding a setting that is only needed on the initiator; in mixed-version/rolling-upgrade clusters, older remote hosts can fail with unknown-setting errors before execution. The forwarded AST settings should drop object_storage_remote_initiator_cluster together with object_storage_remote_initiator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@ianton-ru does it make sense? it looks like it does
There was a problem hiding this comment.
object_storage_remote_initiator_cluster does nothing without ``object_storage_remote_initiator`. Make sense to remove just for less garbage in sub-query.
| Prewhere filter | ||
| Prewhere filter column: less(multiply(2, b), 100) | ||
| Filter column: and(indexHint(greater(plus(i, 40), 0)), equals(a, 0)) (removed) | ||
| Filter column: and(equals(a, 0), indexHint(greater(plus(i, 40), 0))) (removed) |
There was a problem hiding this comment.
Argument order depends on hash, hash was changes (see FunctionNode::updateTreeHashImpl)
| 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; |
There was a problem hiding this comment.
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:
/// In case the current node is not supposed to initiate the clustered query
/// Sends this query to a remote initiator using the `remote` table function
if (settings[Setting::object_storage_remote_initiator])
{
/// Re-writes queries in the form of:
/// Input: SELECT * FROM iceberg(...) SETTINGS object_storage_cluster='swarm', object_storage_remote_initiator=1
/// Output: SELECT * FROM remote('remote_host', icebergCluster('swarm', ...)
/// Where `remote_host` is a random host from the cluster which will execute the query
/// This means the initiator node belongs to the same cluster that will execute the query
/// In case remote_initiator_cluster_name is set, the initiator might be set to a different cluster
}
| if (shard_addresses.size() != 1) | ||
| throw Exception(ErrorCodes::LOGICAL_ERROR, "Size of shard {} in cluster {} is not equal 1", shard_num, cluster_name_from_settings); | ||
| auto host_name = shard_addresses[0].toString(); | ||
| std::string host_name; |
There was a problem hiding this comment.
Address here in encoded format foo%2Ebar instead of foo.bar. This wasn't catched in tests before I add a host with dot in name.
| 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) |
| 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', ...)) |
There was a problem hiding this comment.
What if the query is not remote? Can't we check for that?
/// If cluster not exists, query falls later
Where and with which exception? It would be good to avoid any network calls before failing
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
@ianton-ru does it make sense? it looks like it does
|
The changes look ok, but I think it needs more documentation. I also wonder if we can keep the 'cluster exists' check by wrapping that check in a `if (!is_remote0' |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Different improvement for remote initiator
Documentation entry for user-facing changes
With remote initiator feature queries like
rewrites as
'remote_host' is a random host from 'swarm' cluster
See #756
Current PR introduces the next improvements:
object_storage_remote_initiatorauth works incorrectly #1570 - uses username and password if access to cluster requires it. Throws exception if cluster uses common secret, this should be solved in future PRs.object_storage_remote_initiatorwith different cluster name #1571 - new settingobject_storage_remote_initiator_clusterallows to chooseremote_hostfrom different cluster, not only fromswarmremotequery did not work with additional setting inside function, likeremote('remote_host', iceberg(..., SETTINGS iceberg_metadata_file_path='path/to/metadata.json')). Now must work correctly.remote('remote_host', icebergCluster('remote_cluster', ...))clusterremote_clustercan be defined only onremote_hostand unknown on current initial host. Removed cluster check on early stage, this allows to execute such queries.CI/CD Options
Exclude tests:
Regression jobs to run: