Skip to content

fix(planner): apply shared cleanup thresholds - #786

Open
milindsrivastava1997 wants to merge 1 commit into
mainfrom
760-retention-take-max-across-shared-aggregation-refs-sliding-depth-n-1ws+1
Open

milindsrivastava1997 wants to merge 1 commit into
mainfrom
760-retention-take-max-across-shared-aggregation-refs-sliding-depth-n-1ws+1

Conversation

@milindsrivastava1997

@milindsrivastava1997 milindsrivastava1997 commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • retain the maximum circular-buffer depth across shared aggregation references
  • keep read-based cleanup thresholds policy-specific and sum them across shared references
  • make optimizer output use active read-based cleanup for value and key aggregations

Decision

Optimizer output intentionally uses ReadBased cleanup. It configures read_count_threshold, not CircularBuffer's num_aggregates_to_retain; therefore the CircularBuffer-specific sliding depth formula (n−1)·W/S + 1 is not applicable to this path. A future optimizer-selectable CircularBuffer mode should handle that calculation in a separate change.

Verification

  • cargo fmt --check
  • cargo test -p asap_types
  • cargo test -p asap_planner

Closes #760

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three issues with the per-assignment thresholds. Before this PR they were unused, because the optimizer emitted NoCleanup. Now it emits ReadBased and sets num_aggregates_to_retain = None, so these counts alone decide when windows are deleted. A window is deleted after it has been read threshold times, and a window that is never read is never deleted.

use asap_types::enums::{CleanupPolicy, QueryLanguage};

let mut inference = InferenceConfig::new(QueryLanguage::promql, CleanupPolicy::NoCleanup);
let mut inference = InferenceConfig::new(QueryLanguage::promql, CleanupPolicy::ReadBased);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subtract windows are deleted while queries still need them (this comment is about Subtract => 2 at line 89, which takes effect because of the switch to ReadBased here). Subtract => 2 assumes the engine reads only two prefix-sum checkpoints. The engine has no subtract path (IncrementalMerger is listed as future work in window_merger), so Subtract runs as a normal range query that reads all n windows on every run.

Example: sum_over_time(m[5m]) with t_repeat = 60s and scrape = 60s. This picks tumbling W = 60s with n = 5, and Sum is subtractable, so the method is Subtract with threshold 2. Each window is needed by 5 consecutive runs but is deleted after the 2nd read. Runs 3–5 merge only about 2 of the 5 windows, so the sum comes back too low and no error is raised.

pub fn cleanup_count_for_assignment(query_method: &QueryMethod) -> u64 {
match query_method {
QueryMethod::Direct => 1,
QueryMethod::Merge { num_windows } => *num_windows,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory grows without limit when queries run less often than the window slides. Merge { num_windows } => n assumes each window is read n times. That only holds when t_repeat equals the slide. window_candidates allows W and S smaller than t_repeat.

  • range = 300s, t_repeat = 300s, W = 60s gives Merge{5}. Each window is read once per run, so its count stays at 1 and never reaches 5.
  • sliding W = 120s, S = 60s, t_repeat = 120s: every other window is never read.

In both cases those windows are never deleted. num_aggregates_to_retain used to cap retention, and it is now None, so store memory grows forever. The rule-based planner avoids the first case by using ceil(lookback / effective_repeat).

let key_ref = assignment.key_aggregation_id.map(|key_id| {
let key_retain = solution.deployed_configs()[&key_id].num_aggregates_to_retain;
AggregationReference::new(key_id, key_retain)
AggregationReference::with_read_count_threshold(key_id, Some(cleanup_count))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key tracker gets the value aggregation's threshold, which doesn't match how often its panes are read. build_key_config makes the DeltaSet tracker tumble at the value's slide S and keep ceil(range / S) panes. Each pane is read about range/S times when t_repeat = S. The threshold passed here is the value's count instead: range/W for sliding Merge, or 2 for Subtract.

Example: W = 120s, S = 60s, range = 240s gives a key threshold of 2. Each key pane is deleted after 2 of the 4 runs that need it. Later runs then build the key set from missing deltas, and keys drop out of the query output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Retention: take max across shared aggregation refs; sliding depth (n-1)*W/S+1

1 participant