Repository navigation
feat(asap-tools): feed every metric table of a sketch-bench workload into ClickHouse - #875
Conversation
…ce (#868) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ced feeder (#868) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…able (#868) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e2e-runner job Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Code review: 3 bugs in stopping the feeders between modes (no_teardown, killed before stats are written, no wait for exit), plus robustness and cleanup notes inline.
| monitor_interval_seconds=monitor_interval_seconds, | ||
| ) | ||
| # Before the next mode's init SQL drops the tables it inserts into. | ||
| if paced_feeder_service is not None and not no_teardown: |
There was a problem hiding this comment.
Bug: with flow.no_teardown=true, a mode's feeders are never stopped before the next mode starts. The next mode's init SQL drops and recreates the tables while the old feeders keep inserting, and PacedFeederService.start then sees 8 processes instead of 4 and raises RuntimeError. experiment_run_e2e.py stops the feeder at the top of every mode whatever no_teardown says; do the same before _start_clickhouse_feed.
There was a problem hiding this comment.
Fixed in 6e482c5. _start_clickhouse_feed now stops any feeders and waits until pgrep finds none (PacedFeederService.wait_for_exit) before the mode's init SQL, whatever no_teardown says, and raises if one outlives pkill by 30 s. Test: StartClickHouseFeedTest (order stop → wait → init SQL → start; feeders that survive stop the run before the init SQL).
| ) | ||
| # Before the next mode's init SQL drops the tables it inserts into. | ||
| if paced_feeder_service is not None and not no_teardown: | ||
| paced_feeder_service.stop() |
There was a problem hiding this comment.
Bug: two problems with stopping here.
pkillreaches each feeder about 60 s before itsduration_secsends. The feeder writes--stats-outonly when it finishes, so the per-metric<sink>_<metric>_stats.jsonfiles (tick overruns and achieved rate) are never produced for ClickHouse runs.stop()returns without waiting for the processes to exit. An INSERT already in flight can finish after the next mode'sCREATE TABLEand put the previous arm's rows, at overlapping timestamps (same seed), into the new arm's tables. Wait untilpgrepfinds no feeders before running the init SQL.
There was a problem hiding this comment.
Both fixed in 6e482c5.
- After the queries,
_finish_clickhouse_feedwaits for the feeders to reach the end ofduration_secsand exit on their own, so they write--stats-out; it stops any still running 30 s later. With the generator's 60 s slack this adds about a minute per mode. (experiment_run_e2e.pyhas the same kill-before-stats behaviour; left for a separate change.) - The next mode stops and then waits for no feeders to be running before its init SQL (see the reply above).
Tests:FinishClickHouseFeedTest,WaitForExitTest.
| ) | ||
| check_paced_feeder_feed( | ||
| feeder_config, | ||
| paced_feeder_min_duration_secs(0, minimum_experiment_running_time), |
There was a problem hiding this comment.
The feed-duration check allows only the 2 s startup check before queries start. In this runner the query client starts 10–20 s later, after remote_monitor and the container launch, so with a hand-written config at the minimum duration_secs, groups without align_to_interval keep querying after the feed has stopped. The generator's 60 s slack hides this only for generated configs.
There was a problem hiding this comment.
Fixed in 6e482c5: the minimum now adds QUERY_CLIENT_STARTUP_SECS = 30 (remote_monitor and the client container start after the feed; 10–20 s on CloudLab). The generator's 60 s slack still covers it. Test: test_feed_must_outlast_the_query_client_startup.
| if not local_data_file or local_data_file == "???": | ||
| if feeder is not None: | ||
| # Fed live instead of bulk-loaded. | ||
| if not os.path.exists(feeder.workload_table): |
There was a problem hiding this comment.
For fed configs, validation should also require dataset.init_sql_file, which is now the only thing that creates the metric tables; without it the run fails only after setup. A missing workload_table also raises an unhelpful ConfigAttributeError here instead of a clear ValueError.
There was a problem hiding this comment.
Fixed in 6e482c5: fed configs now require an existing dataset.init_sql_file, and a missing workload_table raises a ValueError naming it. Tests: FedClickHouseConfigValidationTest.
| ) -> None: | ||
| """Recreate the metric tables, start one feeder per metric, and point the | ||
| mode's aligned query groups at the feed's start.""" | ||
| # Before any data arrives: views only see rows inserted after they exist. |
There was a problem hiding this comment.
run_init_sql is passed the hard-coded string "custom" instead of the configured dataset name, yet validation accepts feeders for clickbench and h2o. Either pass dataset_name or have validation reject feeders for non-custom datasets.
There was a problem hiding this comment.
Fixed in 6e482c5: validation now rejects paced_feeder unless dataset.name is custom (the feeder only produces sketch-bench tables), and the runner passes dataset_name through.
| def run_init_sql(self, dataset_name: str, init_sql_file: str) -> None: | ||
| """Run a local DDL SQL file on the node, loading no data.""" | ||
| url = f"http://localhost:{self.clickhouse_http_port}/" | ||
| self._ensure_clickhouse_http_ready() |
There was a problem hiding this comment.
run_init_sql calls _ensure_clickhouse_http_ready again, so the bulk-load path, which already ran it in start(), now checks readiness twice. Keep the check in the callers, or split out a helper without it.
There was a problem hiding this comment.
Fixed in 6e482c5: the copy-and-execute steps are now a private _exec_local_sql_file with no readiness check. start() (one check) and the built-in DDL branch use it, and only the public run_init_sql checks readiness.
| return aligned_interval_lcm(client_config) | ||
|
|
||
|
|
||
| def _start_clickhouse_feed( |
There was a problem hiding this comment.
13 positional parameters, several of them derivable (the experiment and local dirs can be built from the root dirs plus experiment_mode). Neighbouring str arguments can be swapped silently at the call site. Compute the derivable ones inside, or call it with keyword arguments.
There was a problem hiding this comment.
Fixed in 6e482c5: the parameters are keyword-only, the per-mode output dirs are built inside from the root dirs and experiment_mode, and the call site uses keywords.
| feeder_config, | ||
| paced_feeder_min_duration_secs(0, minimum_experiment_running_time), | ||
| ) | ||
| return aligned_interval_lcm(client_config) |
There was a problem hiding this comment.
aligned_interval_lcm enforces ASAPQuery's 5 s allowed-lateness rule on query_time_offset. That rule doesn't apply here, because this runner refuses sketchdb mode, so a ClickHouse-only config with query_time_offset=2 is rejected for no reason.
There was a problem hiding this comment.
Leaving this as is. It only rejects hand-written configs with query_time_offset < 5 s; the generator uses 10 s. ClickHouse needs about 2 s anyway: a tick's rows are inserted after its second ends, so 5 s is a reasonable floor here too. Relaxing it would mean a parameter on the helper shared with experiment_run_e2e.py. Happy to revisit if a ClickHouse-only config needs a smaller offset.
…review fixes - Stop feeders and wait for them to exit before each mode's init SQL, whatever no_teardown says, so no old INSERT lands in the new tables. - After the queries, let feeders run to the end of their feed so they write their stats; stop any that overstay. - Feed duration check allows for the query client's startup. - Fed configs need dataset.name custom, init_sql_file and an existing workload_table. - One ClickHouse readiness check per load; keyword-only feed helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes #868. Closes #873.
Summary
sketch_bench_workloads.py(#844) writes one ClickHouse table per metric, but the ClickHouse runner only bulk-loadeddataset.local_data_fileinto one table, so the others stayed empty. And since #841 the query client refusesalign_to_intervalgroups withoutdata_start_time, so #844's configs failed at client startup.With
dataset.paced_feederset,experiment_run_clickhouse.pynow feeds every metric table live, reusing #841's feeder plumbing:paced_feeder.py): aclickhousesink (--sink clickhouse, one table per metric, records metrics too).PacedFeederServicetakes its target nodes directly; the table/type/duration checks are split intocheck_paced_feeder_feedfor reuse.baseline_mv's views exist before data), starts one feeder per metric on the ClickHouse node at an aligned start (aligned_interval_lcm/aligned_start_ms), writesdata_start_timeinto the mode's client config (set_data_start) and rsyncs it, runs the queries, then stops the feeders. Configs withoutpaced_feederbulk-load as before.sketchdbmode is refused with a feeder (its engine ingests a data file); follow-up in asap-tools: feed the sketchdb arm in the ClickHouse runner #874.paced_feeder.workload_tableinstead oflocal_data_file.sketch_bench_workloads.pytakes--seed(and optional--samples-per-sec/--records-per-sec) instead of--data-file, and emits the feeder block for all metrics.data-sources/paced-feeder/installation/install.sh, listed incomponents.conf), soonly_install_internal_components.shrebuilds it after a code sync; previously only the one-timecommon/setup_dependencies.shbuilt it.Each ClickHouse arm gets its own feed; with the same seed and table, repetition k covers the same samples in every arm because each arm's start is aligned and its
data_start_timematches its feed.Testing
tests/test_paced_feeder.py(ClickHouse sink command lines),tests/test_experiment_run_clickhouse.py(feeder config checks; init SQL before the feed, aligned start,data_start_time == start_ms // 1000, sink URL),tests/test_sketch_bench_workloads.py(feeder block for every metric; generated config passesvalidate_experiment_config, which failed before the fix). Experiments suite: 156 passed. black/flake8 clean.e2e-rate10/synthetic-templatesall-shared1(gauge, counter, two records metrics; ~200k rows/s + 300k records/s), seed 42, 600 s query phase:baseline: 4 feeders, one shared start, all ran to the end without errors; queries on all four tables returned non-empty results (4500 results, 0 empty, 0 errors).baseline_mv, run ~17 min later: all 4500 answers (96 queries x repetitions) equalbaseline's within 1e-6 relative, so the aligned starts line up across runs.flows/httpoverran some ticks (up to 2.5 s) in bursts while the 60 s query group (1-day windows) ran on the same node, then caught up. Scale with--records-per-secor move the feeder off the ClickHouse node if that matters.🤖 Generated with Claude Code