Antalya 25.8: Remote initiator improvements 2#1638
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
arthurpassos
left a comment
There was a problem hiding this comment.
To the limits of my knowledge, it looks ok
| if (cluster_name.empty()) | ||
| { | ||
| throw Exception( | ||
| ErrorCodes::LOGICAL_ERROR, |
There was a problem hiding this comment.
I know it was already here, but is LOGICAL_ERROR a good pick? Wouldn't BAD_ARGUMENTS be better? I mean, if the error / empty cluster is a code error, then ok. But if it's a missing argument from the user, then we should change
There was a problem hiding this comment.
When user did not set cluster name, updateQueryForDistributedEngineIfNeeded must be called with make_cluster_function=false, this check is not executed in this case. It is a technical check to be sure I did not make mistake somewhere in code.
Verification report: Altinity/ClickHouse PR #1638ConclusionPR is verified. CI on head
|
| Check | Test FAIL on head | Class |
|---|---|---|
Integration tests (amd_asan, flaky check) |
test_s3_cluster::test_graceful_shutdown[3-5] |
Pre-existing flake — exists since 2025-11-05; test_graceful_shutdown family has 52+ FAILs / 24 PRs / 90d. Caught by the dedicated "flaky check" job, expected behavior. |
Stateless tests (amd_debug, parallel) |
03211_nested_json_merges |
Pre-existing flake — 16 FAILs / 7 PRs / 90d, unrelated to PR diff (JSON merges). |
Stateless tests (amd_asan, distributed plan, parallel, 1/2) |
(no test FAIL recorded) | Job-level error; rerun candidate. |
Stateless tests (amd_asan, distributed plan, parallel, 2/2) |
n/a — in progress / passing per user | — |
Regression workflow (4 failed jobs)
| Check | Top failure on PR-1638 builds | Baseline (30d, all PRs) | Class |
|---|---|---|---|
RegressionTestsRelease / Common (settings) / settings |
8 settings missing snapshots; 1 of them — object_storage_remote_initiator_cluster — is the new setting introduced by this PR |
object_storage_remote_initiator_cluster: 144 OK / 57 Fail across all 25.8 builds |
**PR-cause: need to update snapshot |
RegressionTestsAarch64 / Common (settings) / settings |
same | same | same |
RegressionTestsRelease / Swarms / swarms |
/swarms/feature/node failure/check restart swarm node, /swarms/feature/swarm joins |
well-known broken/flaky swarms scenarios | Pre-existing |
RegressionTestsAarch64 / Swarms / swarms |
same | same | same |
RegressionTestsAarch64 / SSLServer (3) / ssl_server_3 |
/ssl server/part 3/zookeeper/fips/* (FIPS / ZK ciphers) |
recurrent across PRs | Pre-existing infra (FIPS ZK) |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Use
object_storage_remote_initiatorwithoutobject_storage_clusteron initial node.Documentation entry for user-facing changes
Solved #1607
Query with
object_storage_remote_initiatorandobject_storage_remote_initiator_clustercan be executed, whenobject_storage_clustersetting is defined only on remote node.converted to
Remote node
random_node_from_swarm_external_nameexecutesas simple table function or as cluster table function depend on
object_storage_clusterdefined only on this node.CI/CD Options
Exclude tests:
Regression jobs to run: