planner: MILP plan → InferenceConfig should use ReadBased cleanup instead of NoCleanup #800

Activity

1. milindsrivastava1997

   self-assigned this

   on Oct 7, 2026
2. milindsrivastava1997

   added

   asap-planner

   on Oct 7, 2026
3. milindsrivastava1997 commented on Oct 7, 2026

   

   milindsrivastava1997

   on Oct 7, 2026

   ContributorAuthor

   More actions

   Note from A3: under NoCleanup, the planner's YAML carries no retention at all. build_queries_yaml writes num_aggregates_to_retain only under CircularBuffer and read_count_threshold only under ReadBased, and StreamingConfig::from_yaml_data derives each aggregation's retention from those. So until this issue lands, MILP-planned aggregations keep every window, while the MILP charged memory for only retained_instance_count windows. The ReadBased threshold should come from the S8 plan (PlannedRaqe::merged_instance_count, PlannedDeployment::retained_instance_count).
4. milindsrivastava1997 commented on Oct 7, 2026

   

   milindsrivastava1997

   on Oct 7, 2026

   ContributorAuthor

   More actions

   Blocked: is ReadBased the right policy for MILP plans, or should we use CircularBuffer?

   ReadBased evicts a window after threshold reads, so it is correct only if every window instance gets the same number of reads. S8 eligibility only requires T % y == 0. When T > y, some windows are read less often or never. Example: tumbling 60s windows, T = 120s, L = 60s. The odd-minute windows are never read, never evicted, and memory grows without bound. ReadBased has two other weaknesses:

   • Ad-hoc reads of the aggregation count toward the threshold and can evict a window before its scheduled reader gets to it.
   • A skipped scheduled evaluation means the window is never evicted.

   CircularBuffer with num_aggregates_to_retain taken from PlannedDeployment::retained_instance_count keeps exactly what the MILP charged memory for. It doesn't depend on read alignment, and a shared deployment is already sized for its longest-lookback reader.

   Proposal: emit CircularBuffer from the MILP output and retitle this issue. If ReadBased is required, the fallback is to emit it only when every reader of a deployment has T == y, and to error otherwise.

   Related (separate fix): ReadBased thresholds never reach the engine today. StreamingConfig::from_yaml_data (asap_types/src/streaming_config.rs) sums each query's num_aggregates_to_retain into read_count_threshold. Under ReadBased, InferenceConfig sets only read_count_threshold, so the sum is empty and nothing is evicted. Legacy ReadBased plans therefore behave as NoCleanup in the engine.
5. milindsrivastava1997

   added a commit that references this issue on Oct 8, 2026

   feat(planner): add asap-planner --planner legacy|milp (#808)

   ...

   fe74d3c
6. zzylol commented on Oct 13, 2026

   

   zzylol

   on Oct 13, 2026

   Contributor

   More actions

   @milindsrivastava1997 Agree on CircularBuffer, with one addition: evicted windows go to disk instead of being dropped.

   Proposal

   • Memory: CircularBuffer with num_aggregates_to_retain = PlannedDeployment::retained_instance_count. That's exactly what the MILP charges memory for, it doesn't depend on read alignment (your T > y example), and a shared deployment is already sized for its longest-lookback reader. --planner saved goes through the same milp_output path, so one fix covers both.
   • Disk: spill instead of drop. When a window leaves the in-memory buffer, write its serialized accumulator to disk (e.g. an append-only file per aggregation, indexed by window start) and keep it for a configured retention, instead of deleting it. A query whose range reaches past the in-memory windows reads the older ones from disk. Scheduled RQE reads never need the disk, since the buffer holds their whole lookback; only ad-hoc or longer-range queries do. This keeps history the way Prometheus, VictoriaMetrics and ClickHouse do, so the comparison isn't "ASAPQuery drops old data".
   • Cost model: unchanged. Memory is still retained_instance_count windows; disk isn't charged, matching §6.3's "CPU + retained memory, no disk". In the end-to-end runs, spill writes count in ASAPQuery's measured CPU, and disk bytes are reported separately.

   Why it's urgent: the end-to-end evaluation (plan in #862) feeds each system for ~25 h to fill the 24 h windows. Under NoCleanup, ASAPQuery would keep every window in memory, so its memory would grow for the whole run and stop matching the plan.

   Suggested order: CircularBuffer first; it unblocks the runs on its own. The disk tier is a separate PR, ideally in before the runs too, so ASAPQuery keeps history like the baselines. If it can't land in time, we run with CircularBuffer and say so in the paper.
7. milindsrivastava1997 commented on Oct 14, 2026

   

   milindsrivastava1997

   on Oct 14, 2026

   ContributorAuthor

   More actions

   Scope decided for this issue:

   1. Retention. MILP output emits CircularBuffer. Each query's num_aggregates_to_retain = Deployment::closed_instance_count(lookback), i.e. (L − x)/y + 1. Only closed windows, since open ones live in the precompute engine, so store + precompute together match retained_instance_count. Shared aggregations take the max across queries (ported from #760, which this closes; it also fixes ReadBased thresholds summing the wrong field).

   The engine store kept num_aggregates_to_retain × max_epochs windows with max_epochs = 4 hardcoded, i.e. 4× the plan. max_epochs becomes 1 everywhere, so the store keeps exactly N windows. Legacy CircularBuffer N already covers the full lookback, so the 4× was just slack.

   2. Write-only disk spill (second commit, same PR). The engine flag --spill-dir <path> (off by default) makes every evicted window, under both CircularBuffer and ReadBased, append its serialized accumulator to <dir>/agg_<id>.bin. Writes are inline and buffered. A write failure is logged and counted in a metric, and ingestion continues. No disk retention. Spilled windows aren't queryable yet; a follow-up issue will add the read path (needs a byte deserializer per accumulator type).

   For #862: pass --spill-dir, report the directory size as disk bytes, and fail the run if the spill-error metric is non-zero.
8. milindsrivastava1997 commented on Oct 14, 2026

   

   milindsrivastava1997

   on Oct 14, 2026

   ContributorAuthor

   More actions

   E2E check of this branch: retention ignores how far in the past a query evaluates.

   Run: generated2 workload 1 with num_ports_per_server: 10 (80,000 series), legacy planner, new default aggregate_cleanup.policy: circular_buffer, query_engine.spill=true, 1 node pair on Clemson.

   8 of the 10 queries were served by ASAPQuery. Both instant quantiles, quantile by (label_0) (0.65|0.7, fake_metric_total), were forwarded to Prometheus on every repetition (1122/1122, Ok(None) path).

   Cause, confirmed from the spill files:

   • Both read only agg 4 (KLL, 1s tumbling, num_aggregates_to_retain: 1), so the store keeps just the newest closed window.
   • The client evaluates each query at T and sends it at T + 10.0–10.5 s (query_time_offset: 10).
   • 1258/1258 windows those queries needed (end == T) are in agg_4.bin: each was evicted ~1 s after the next window closed, ~9 s before it was asked for.
   • Control: sum by (label_0) (fake_metric_total) has the same offset and also gets retain: 1, but its aggregation is shared with the [15m] queries (max rule → 900), and all 561 requests were served by ASAPQuery.

   Retention covers only the lookback (ceil((lookback + range)/repeat) legacy, (L − x)/y + 1 MILP). Before this branch read_based never evicted anything, so this was hidden.

   Decision: engine-side slack, given as a time span.

   • New engine config store.retention_slack_ms (default 0, so retention matches the plan exactly). Each aggregation keeps N + ceil(retention_slack_ms / slide_ms) windows.
   • asap-tools derives it from the workload: max query_time_offset across query groups, plus one window of margin.
   • Memory then exceeds the plan by that slack per aggregation. For this run: +11 windows of 1 s, +1 window of 60 s.

   Other numbers from the run (sketchdb mode, ~45 min): engine memory flat at 7.0–7.5 GiB; spill ~1.9 GB, dominated by the per-series 60 s KLL (agg 8, 80k keys × 536 B per window) and MultipleIncrease (agg 7, ~15 MB per window).