Repository navigation
fix(asap-tools): probe the ClickHouse adapter's readiness endpoint - #905
Conversation
A ClickHouse-backed query engine serves /clickhouse/ping and no Prometheus API, so the readiness check on /api/v1/status/runtimeinfo never passed and every ClickHouse sketchdb run timed out after 60 s. The endpoint now follows the backend type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
milindsrivastava1997
left a comment
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Automated code review (Claude Code). Main issue: Elasticsearch backends still hit the readiness timeout this PR fixes for ClickHouse.
| return 8088 | ||
|
|
||
|
|
||
| def runtime_info_endpoint(backend_type: str) -> str: |
There was a problem hiding this comment.
runtime_info_endpoint() maps only "clickhouse"; every other backend falls through to the Prometheus path. elastic_querydsl / elastic_sql use ElasticHttpAdapter, which only routes runtime info at /_cluster/health (elastic_http.rs:232), so the probe 404s and wait_until_ready still times out after 60 s — the same failure this PR fixes for ClickHouse.
|
|
||
|
|
||
| def runtime_info_endpoint(backend_type: str) -> str: | ||
| """The readiness endpoint of the engine's query adapter for `backend_type`: |
There was a problem hiding this comment.
Altitude: this duplicates get_runtime_info_path() from the Rust adapters in Python, and it has already drifted (Elastic missing). servers/http.rs registers /metrics on the same router for every adapter — probing that (or having the engine report its path) would remove the mapping entirely and not break when adapters are added/renamed.
| @@ -652,7 +663,7 @@ def _is_runtime_info_ready(self) -> bool: | |||
| cmd = ( | |||
| f"curl -fsS --max-time {constants.HTTP_READINESS_CURL_TIMEOUT_SECONDS} " | |||
| f"http://127.0.0.1:{self.get_http_port()}" | |||
There was a problem hiding this comment.
The probe uses get_http_port() (hard-coded 8088) rather than the http_port passed to start(); self.http_port is only set when spill=True. If mode_server_urls[sketchdb] (e.g. experiment_run_clickhouse.py:746) uses a non-8088 port, readiness never succeeds regardless of the endpoint fix.
| remote_write_port: Port the precompute engine listens on for Prometheus remote | ||
| write; should match streaming.remote_write.base_port (default 8080) | ||
| """ | ||
| self.runtime_info_endpoint = runtime_info_endpoint(backend_config["type"]) |
There was a problem hiding this comment.
Minor: start() now indexes backend_config["type"] as its first statement, so a backend dict without "type" raises a bare KeyError before the config-generation error path. (resolve_backend_config already indexes it, so only direct callers of start() are affected.)
| self.remote_spill_dir: Optional[str] = None | ||
| self.local_spill_report: Optional[str] = None | ||
| self.http_port: Optional[int] = None | ||
| # The started backend's adapter's; set by start(). |
There was a problem hiding this comment.
Nit: the attribute self.runtime_info_endpoint shadows the module-level function runtime_info_endpoint() (line 285 reads like a self-reference), and the comment is missing a noun ("The started backend's adapter's; set by start()."). Consider renaming e.g. readiness_path.
| args="curl", returncode=0, stdout="", stderr="" | ||
| ) | ||
| service = QueryEngineRustService(provider, use_container=False, node_offset=0) | ||
| service.runtime_info_endpoint = runtime_info_endpoint("clickhouse") |
There was a problem hiding this comment.
This sets service.runtime_info_endpoint by hand, so the actual fix — start() picking the endpoint from backend_config["type"] — is untested. Deleting the new line in start() leaves this test passing. Driving start() with a mocked provider + clickhouse backend_config and asserting on the probe command would cover it.
| provider.execute_command.call_args_list[1].kwargs["cmd"], | ||
| ) | ||
|
|
||
| def test_clickhouse_backed_query_engine_probes_its_adapters_endpoint(self): |
There was a problem hiding this comment.
Nit: a ClickHouse query-engine readiness test lives in test_victoriametrics_readiness.py, and the Prometheus mapping check is appended to the end of the ClickHouse test — if the ClickHouse asserts fail, the Prometheus check never runs. Consider a separate test (and file).
1 participant
Problem:
QueryEngineRustServicechecks readiness by curling/api/v1/status/runtimeinfo, the Prometheus adapter's endpoint. A ClickHouse-backed engine serves/clickhouse/pinginstead (clickhouse_http.rs:178) and returns 404 on the Prometheus path, sowait_until_readytimed out after 60 s on every ClickHouse sketchdb run. Seen on CloudLab while testing #874: the engine was up (/clickhouse/ping200,/api/v1/status/runtimeinfo404).Fix:
runtime_info_endpoint(backend_type)picks the adapter's endpoint;start()stores it and the readiness check probes it.Tests:
tests/test_victoriametrics_readiness.py.🤖 Generated with Claude Code