Skip to content

feat(sketch-db): create_checked retention validation (Method B) - #40

Merged
zzylol merged 1 commit into
mainfrom
sketchdb/backfill-retention-check
Apr 18, 2026
Merged

feat(sketch-db): create_checked retention validation (Method B)#40
zzylol merged 1 commit into
mainfrom
sketchdb/backfill-retention-check

Conversation

@zzylol

@zzylol zzylol commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Extends BackfillRegistry::create_checked with Method B from the design discussion: reject a job up-front when its start_ms is outside the SimpleMapStore retention horizon, so operators get a clear error instead of the retention sweep silently wiping freshly-backfilled windows.

  • New CreateError::OutOfRetention { agg_id, requested_start_ms, earliest_retained_ms } variant.
  • create_checked gains trailing data_retention_ms: Option<u64> — None skips the check (tests / disabled retention).
  • 3 existing callsites updated to pass None (no behavior change).

Test plan

  • 3 new tests: out-of-retention reject, None skips, within-retention accept.
  • 3 existing create_checked tests still green with new signature.
  • 713 lib tests pass (up from 710).
  • clippy + fmt clean.

Next (PR 4)

SchemaEvictionService tokio task. Polls schema registry, cancels in-flight backfills for Expired schemas, calls drop_agg_id, cleans registry. Flags: --enable-schema-eviction, --schema-eviction-dry-run. Default retention → 24h. Startup warn if persistence_delete_older_than < retirement_retention.

🤖 Generated with Claude Code

@zzylol @claude
feat(sketch-db): create_checked retention validation (Method B)
a944de8 
Extends `BackfillRegistry::create_checked` with the Method-B check
from the design discussion: if the requested backfill `start_ms` is
older than `persistence_delete_older_than_ms` (the SimpleMapStore
retention horizon), reject up-front with
`CreateError::OutOfRetention` instead of silently letting the
backfill produce windows the retention sweep will immediately
evict.

## What's landed

New `CreateError::OutOfRetention { agg_id, requested_start_ms,
earliest_retained_ms }` variant alongside existing `UnknownAgg`
and `Overlap`. `Display` includes a specific actionable message:
"extend persistence_delete_older_than before creating this job".

`create_checked` signature gains a trailing
`data_retention_ms: Option<u64>` parameter:
- `Some(N)`: reject if `now - start_ms > N`.
- `None`: skip the check (testing / retention-disabled deployments).

Existing tests that called `create_checked` updated to pass
`None`; behavior unchanged there. No other production callsites
exist yet — the HTTP endpoint in `handle_post_backfill_job` calls
the unchecked `create()` today, so this PR doesn't break any
production flow. A follow-up PR will wire the HTTP endpoint
through `create_checked` with the deployment's retention value.

## Test plan

- [x] 3 new tests:
  * `create_checked_rejects_start_older_than_data_retention` —
    start=0 + retention=1h → `OutOfRetention` with correct fields.
  * `create_checked_none_retention_skips_check` — `None` accepts
    a start that would otherwise fail.
  * `create_checked_accepts_start_within_retention` — start=now-30m,
    retention=1h → accepted (guard against off-by-one at boundary).
- [x] 3 existing tests (Overlap, boundary-at-created_at,
  UnknownAgg) pass with new signature.
- [x] 713 lib tests total (up from 710).
- [x] clippy `--workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

## Next

PR 4: `SchemaEvictionService` tokio task consuming the primitives
from PRs 2 + 3 — polls schema registry, cancels in-flight backfills
for Expired schemas, drops their agg_id, removes from registry.
`--enable-schema-eviction` + `--schema-eviction-dry-run` flags.
Default retention 24h. Startup warn if
`persistence_delete_older_than < retirement_retention`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit d4d8d3e into main Apr 18, 2026
@zzylol
zzylol deleted the sketchdb/backfill-retention-check branch April 18, 2026 19:58
zzylol added a commit that referenced this pull request Apr 18, 2026
@zzylol @claude
feat(sketch-db): SchemaEvictionService — background cleanup of Expire…
76f32fb 
…d schemas

The last piece of the Phase 5 eviction chain. Consumes the
primitives from PRs 2 + 3 (drop_agg_id, create_checked) to make
retired schemas actually reclaim space when they pass expires_at_ms,
instead of accumulating forever in the store.

## What's landed

`src/stores/sketch_db/schema_eviction.rs`:

* `SchemaEvictionConfig { poll_interval, dry_run }` — service
  tuning, independent from `SchemaRegistry::retirement_retention`
  which stays a data-model property.
* `SchemaEvictionService::new(schemas, backfill, store, config)`
  and `.spawn() -> SchemaEvictionHandle` for the tokio task.
* `SchemaEvictionHandle::shutdown().await` for clean ctrl-c.
* `SchemaEvictionService::run_once()` — pub-crate synchronous
  sweep, exposed so tests can drive deterministically.
* `warn_if_retention_inverted(data_retention, retirement_retention)`
  — startup check. If `persistence_delete_older_than <
  retirement_retention`, the two retention layers race each other
  on expired data; log a `warn!` with actionable message.

Sweep behaviour (per `run_once`):

1. List all `AggStatus::Expired` schemas.
2. For each Expired schema's agg_id:
   a. Find any `Running` backfill jobs targeting that agg_id
      → cancel them (writing to data about to drop = wasted work).
      Conform to the user's Phase 5 direction: "留着 backfill 也没
      用,直接 cancel 然后删掉".
   b. `store.drop_agg_id(agg_id)` → evict the windows.
   c. `schema_registry.remove_schema(agg_id)` → drop the registry
      entry so next tick doesn't re-evict.
3. Log audit line with `agg_id`, `metric`, `retired_at_ms`,
   `evicted_windows`, `running_backfills_cancelled`.

`--schema-eviction-dry-run` skips steps 2b/2c — every other step
runs including logging and running-backfill discovery — so the
operator can validate a new retention value before letting it
delete anything.

## Schema module changes

* `DEFAULT_RETIREMENT_RETENTION`: **1h → 24h**. Old default was
  too short for typical analyst workflows. Per the user's rule
  of thumb `persistence_delete_older_than > retirement_retention`,
  24h leaves room for day-over-day comparisons without fighting
  data retention.
* `SchemaRegistry::set_retention(duration)` — non-test method so
  `main.rs` can override at startup (previously only
  `set_retention_for_testing` existed).
* `SchemaRegistry::retirement_retention() -> Duration` — getter
  so the eviction service can log the effective value and the
  retention-inversion check can read it.
* `SchemaRegistry::remove_schema(agg_id) -> Option<AggSchema>`
  — idempotent removal, triggers persist-to-disk if configured
  so restart doesn't reintroduce the evicted entry.

## main.rs wiring

* New CLI flags:
  * `--enable-schema-eviction` (off by default)
  * `--schema-eviction-poll-secs` (default 300)
  * `--schema-eviction-dry-run` (off by default)
* On startup, if enabled AND precompute is running: spawns the
  service with the schema registry from `precompute_ingest_state`.
* Invokes `warn_if_retention_inverted` before spawn so operators
  see the ordering violation immediately on restart.
* `schema_eviction_handle.shutdown().await` slotted into the
  existing graceful-shutdown chain.

## Test plan

- [x] 5 new unit tests:
  * `run_once_drops_expired_agg_data` — fixture with Expired
    agg 1 + Active agg 2 → sweep drops agg 1's data, removes
    agg 1 from registry, leaves agg 2 alone.
  * `run_once_is_noop_with_no_expired_schemas` — Active-only
    registry → no writes.
  * `dry_run_logs_but_does_not_drop` — data + registry entry
    survive under `dry_run: true`.
  * `cancels_running_backfill_for_expired_agg` — Running job on
    agg_id=1 → sweep cancels before dropping data.
  * `retention_inverted_warning_fires_only_when_inverted`
    (doesn't panic on any of the three branches).
- [x] 718 lib tests pass (up from 713).
- [x] clippy `--workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

## What this completes

The Phase 5 eviction story end-to-end:

1. PR #39 — `Store::drop_agg_id` primitive.
2. PR #40 — `create_checked` rejects backfills outside retention.
3. **This PR** — background service consumes both to actually reclaim
   space when schemas retire + expire.

Follow-up (optional, per-operator): per-agg retention override via
`AggregationConfig.parameters["retirement_retention_secs"]`, audit
log persistence, `/api/v1/db/schemas/:agg_id/extend_retention`
manual extension endpoint.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Apr 18, 2026
@zzylol @claude
feat(sketch-db): SchemaEvictionService — background cleanup of Expire…
2a2fbb4 
…d schemas (#41)

The last piece of the Phase 5 eviction chain. Consumes the
primitives from PRs 2 + 3 (drop_agg_id, create_checked) to make
retired schemas actually reclaim space when they pass expires_at_ms,
instead of accumulating forever in the store.

## What's landed

`src/stores/sketch_db/schema_eviction.rs`:

* `SchemaEvictionConfig { poll_interval, dry_run }` — service
  tuning, independent from `SchemaRegistry::retirement_retention`
  which stays a data-model property.
* `SchemaEvictionService::new(schemas, backfill, store, config)`
  and `.spawn() -> SchemaEvictionHandle` for the tokio task.
* `SchemaEvictionHandle::shutdown().await` for clean ctrl-c.
* `SchemaEvictionService::run_once()` — pub-crate synchronous
  sweep, exposed so tests can drive deterministically.
* `warn_if_retention_inverted(data_retention, retirement_retention)`
  — startup check. If `persistence_delete_older_than <
  retirement_retention`, the two retention layers race each other
  on expired data; log a `warn!` with actionable message.

Sweep behaviour (per `run_once`):

1. List all `AggStatus::Expired` schemas.
2. For each Expired schema's agg_id:
   a. Find any `Running` backfill jobs targeting that agg_id
      → cancel them (writing to data about to drop = wasted work).
      Conform to the user's Phase 5 direction: "留着 backfill 也没
      用,直接 cancel 然后删掉".
   b. `store.drop_agg_id(agg_id)` → evict the windows.
   c. `schema_registry.remove_schema(agg_id)` → drop the registry
      entry so next tick doesn't re-evict.
3. Log audit line with `agg_id`, `metric`, `retired_at_ms`,
   `evicted_windows`, `running_backfills_cancelled`.

`--schema-eviction-dry-run` skips steps 2b/2c — every other step
runs including logging and running-backfill discovery — so the
operator can validate a new retention value before letting it
delete anything.

## Schema module changes

* `DEFAULT_RETIREMENT_RETENTION`: **1h → 24h**. Old default was
  too short for typical analyst workflows. Per the user's rule
  of thumb `persistence_delete_older_than > retirement_retention`,
  24h leaves room for day-over-day comparisons without fighting
  data retention.
* `SchemaRegistry::set_retention(duration)` — non-test method so
  `main.rs` can override at startup (previously only
  `set_retention_for_testing` existed).
* `SchemaRegistry::retirement_retention() -> Duration` — getter
  so the eviction service can log the effective value and the
  retention-inversion check can read it.
* `SchemaRegistry::remove_schema(agg_id) -> Option<AggSchema>`
  — idempotent removal, triggers persist-to-disk if configured
  so restart doesn't reintroduce the evicted entry.

## main.rs wiring

* New CLI flags:
  * `--enable-schema-eviction` (off by default)
  * `--schema-eviction-poll-secs` (default 300)
  * `--schema-eviction-dry-run` (off by default)
* On startup, if enabled AND precompute is running: spawns the
  service with the schema registry from `precompute_ingest_state`.
* Invokes `warn_if_retention_inverted` before spawn so operators
  see the ordering violation immediately on restart.
* `schema_eviction_handle.shutdown().await` slotted into the
  existing graceful-shutdown chain.

## Test plan

- [x] 5 new unit tests:
  * `run_once_drops_expired_agg_data` — fixture with Expired
    agg 1 + Active agg 2 → sweep drops agg 1's data, removes
    agg 1 from registry, leaves agg 2 alone.
  * `run_once_is_noop_with_no_expired_schemas` — Active-only
    registry → no writes.
  * `dry_run_logs_but_does_not_drop` — data + registry entry
    survive under `dry_run: true`.
  * `cancels_running_backfill_for_expired_agg` — Running job on
    agg_id=1 → sweep cancels before dropping data.
  * `retention_inverted_warning_fires_only_when_inverted`
    (doesn't panic on any of the three branches).
- [x] 718 lib tests pass (up from 713).
- [x] clippy `--workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

## What this completes

The Phase 5 eviction story end-to-end:

1. PR #39 — `Store::drop_agg_id` primitive.
2. PR #40 — `create_checked` rejects backfills outside retention.
3. **This PR** — background service consumes both to actually reclaim
   space when schemas retire + expire.

Follow-up (optional, per-operator): per-agg retention override via
`AggregationConfig.parameters["retirement_retention_secs"]`, audit
log persistence, `/api/v1/db/schemas/:agg_id/extend_retention`
manual extension endpoint.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Apr 20, 2026
@zzylol @claude
fix(sketch-db): POST /api/v1/db/backfill uses create_checked (§10.5)
daaa42a 
The HTTP handler was calling `BackfillRegistry::create()`, which
skips every invariant. PR #40 added `create_checked()` — the same
entry point the `BackfillService` uses — but the HTTP surface
kept the unchecked path, so controllers could file jobs against
unknown agg_ids or time ranges that overlap live ingest / fall
outside retention, and the worker would only discover the
mistake later (or silently waste I/O on soon-to-be-evicted
windows).

Changes:
- `handle_post_backfill_job` now requires both the backfill
  registry AND the schema registry (503 if either is absent),
  calls `create_checked(&schemas, ..., data_retention_ms)`, and
  maps `CreateError` to distinct HTTP statuses: 404 for
  `UnknownAgg`, 409 for `Overlap` / `OutOfRetention`, 201 on
  success. 400 on malformed body / inverted range is preserved.
- `HttpServer::with_data_retention_ms(u64)` threads the SimpleMapStore
  data-retention horizon into AppState so Method B rejects stale
  ranges at create time. `main.rs` wires it from
  `--persistence-delete-older-than-secs` when > 0.
- Test helper renamed `setup_test_server_with_backfill_and_schemas`
  and now takes an `&[u64]` of agg_ids to pre-register as Active.
  All four existing backfill tests updated.
- Two new tests exercise the new error paths:
  `test_backfill_post_unknown_agg_returns_404` and
  `test_backfill_post_overlap_with_live_ingest_returns_409`.

725 lib tests pass (was 723), clippy clean, fmt clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request Apr 20, 2026
Merged
4 tasks
zzylol added a commit that referenced this pull request Apr 20, 2026
@zzylol @claude
fix(sketch-db): POST /api/v1/db/backfill uses create_checked (§10.5) (#…
4590405 
…43)

The HTTP handler was calling `BackfillRegistry::create()`, which
skips every invariant. PR #40 added `create_checked()` — the same
entry point the `BackfillService` uses — but the HTTP surface
kept the unchecked path, so controllers could file jobs against
unknown agg_ids or time ranges that overlap live ingest / fall
outside retention, and the worker would only discover the
mistake later (or silently waste I/O on soon-to-be-evicted
windows).

Changes:
- `handle_post_backfill_job` now requires both the backfill
  registry AND the schema registry (503 if either is absent),
  calls `create_checked(&schemas, ..., data_retention_ms)`, and
  maps `CreateError` to distinct HTTP statuses: 404 for
  `UnknownAgg`, 409 for `Overlap` / `OutOfRetention`, 201 on
  success. 400 on malformed body / inverted range is preserved.
- `HttpServer::with_data_retention_ms(u64)` threads the SimpleMapStore
  data-retention horizon into AppState so Method B rejects stale
  ranges at create time. `main.rs` wires it from
  `--persistence-delete-older-than-secs` when > 0.
- Test helper renamed `setup_test_server_with_backfill_and_schemas`
  and now takes an `&[u64]` of agg_ids to pre-register as Active.
  All four existing backfill tests updated.
- Two new tests exercise the new error paths:
  `test_backfill_post_unknown_agg_returns_404` and
  `test_backfill_post_overlap_with_live_ingest_returns_409`.

725 lib tests pass (was 723), clippy clean, fmt clean.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request May 5, 2026
Merged
3 tasks
zzylol added a commit that referenced this pull request May 5, 2026
@zzylol @claude
chore(asap_sketchlib): bump pin past PR #39 module renames (#75)
d4bcc23 
* refactor: retire sketch-core mirror

* refactor: switch consumer imports to asap_sketchlib::sketches::*

Update PR #73 against the reorganized asap_sketchlib (PR #36): the
runtime sketches no longer live under a dedicated `asap::` module — they
were merged into the existing `src/sketches/` layout (single home per
sketch concept, ASAP-runtime types appended to the file that already
holds the high-throughput in-process variant).

Mechanical path swaps in asap-query-engine:
- `asap_sketchlib::asap::dd_sketch::*`           → `::sketches::ddsketch::*`
- `asap_sketchlib::asap::count_min::*`           → `::sketches::countmin::*`
- `asap_sketchlib::asap::count_sketch::*`        → `::sketches::count::*`
- `asap_sketchlib::asap::hll_sketch::*`          → `::sketches::hll::*`
- `asap_sketchlib::asap::kll::*`                 → `::sketches::kll::*`
- `asap_sketchlib::asap::count_min_with_heap::*` → `::sketches::cms_heap::*`
- `asap_sketchlib::asap::hydra_kll::*`           → `::sketches::hydra_kll::*`
- `asap_sketchlib::asap::set_aggregator::*`      → `::sketches::set_aggregator::*`
- `asap_sketchlib::asap::delta_set_aggregator::*`→ `::sketches::delta_set_aggregator::*`
- `asap_sketchlib::asap::config::*`              → `::asap_runtime::*`

Naming-conflict renames carried through to the consumers:
- `HllDelta` → `HllSketchDelta` (octo_delta::HllDelta still wins the short name)
- `HeapItem` → `CmsHeapItem`   (common::input::HeapItem still wins the short name)

main.rs aliases `asap_sketchlib::asap_runtime as config` so the existing
clap derive references (`config::DEFAULT_CMS_IMPL`, `config::configure(...)`)
still work without touching the rest of the bin.

Tests:
- `cargo build --workspace`                                → clean
- `cargo test -p query_engine_rust --lib precompute_operators` → 141 passed, 0 failed

Depends on ProjectASAP/asap_sketchlib#36 (force-pushed `e473ccc`).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor: align CountSketchDelta consumer with sketchlib-go wire format

Track the additive `hh_keys` field on `asap_sketchlib::CountSketchDelta`
so the proto delta path constructs the type with all fields filled in.

Sends an empty `hh_keys` for now: the vendored Rust proto bindings in
`asap_otel_proto::sketchlib::v1` haven't been regenerated against the
latest `.proto` (which carries `hh_keys` on the Go side). The TopK
rebuild on the proto-delta path will fire once those bindings sync;
the sketchlib-go-aligned semantics are already in place underneath.

Bumps the asap_sketchlib git dep to `refactor/wire-format-align-go`
(see asap_sketchlib PR #37).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(asap_sketchlib): bump pin past PR #39 module renames

Three asap_sketchlib modules were renamed in upstream PR #39:
- sketches::countmin → sketches::countminsketch
- sketches::count    → sketches::countsketch
- sketches::cms_heap → sketches::countminsketch_topk

Backend consumers updated. Cargo.toml pin moved from
refactor/wire-format-align-go branch to main (which now also has
hh_keys restoration via PR #42 and DDSketch + KLL byte parity via
#40 + #41).

Unblocks ASAPCollector Phase 3 step 3 (backend consumes
asap-precompute-rs).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 5, 2026
@zzylol @claude
feat(ingest): consume asap-precompute-rs for envelope/delta/merge logic
71c495a 
ASAPQuery-backend ingest path now uses asap-precompute-rs (Rust edge
runtime) for shared logic: envelope parsing, delta application, sketch
state reconstruction, merge. Query-side engine (PromQL aggregation,
storage, query planning) stays in this repo.

Phase 3 step 3 of the ASAP edge-framework migration. DDSketch + KLL
byte parity on upstream asap_sketchlib (#40, #41). HLL/CountSketch/CMS
still pending — functional tests gated with #[ignore = "blocked on
ASAPCollector#243"].

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request May 5, 2026
Merged
6 tasks
zzylol added a commit that referenced this pull request May 5, 2026
@zzylol @claude
feat(ingest): consume asap-precompute-rs for envelope/delta/merge log…
67c1d99 
…ic (#76)

ASAPQuery-backend ingest path now uses asap-precompute-rs (Rust edge
runtime) for shared logic: envelope parsing, delta application, sketch
state reconstruction, merge. Query-side engine (PromQL aggregation,
storage, query planning) stays in this repo.

Phase 3 step 3 of the ASAP edge-framework migration. DDSketch + KLL
byte parity on upstream asap_sketchlib (#40, #41). HLL/CountSketch/CMS
still pending — functional tests gated with #[ignore = "blocked on
ASAPCollector#243"].

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@zzylol