docs: design for SimpleMapStore persistence (mem limit + disk flush) - #4
Merged
Merged
Conversation
Proposes a memory-bounded, disk-backed extension to SimpleMapStore: sealed epochs flushed to per-agg segment files under an atomic manifest, driven by a configurable memory budget and time-age watermark. Phasing, config shape, concurrency plan, and four open questions included for review before implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces the three destructive CleanupPolicy variants on SimpleMapStore with a single two-knob persistence config: memory_limit_bytes as the primary bound on RAM, hot_window_ms as the secondary time watermark that guarantees a predictable hot-set under light ingest. Flusher checks memory pressure first and time watermark second, pulling from the same oldest-epoch-first ordering so the two triggers never disagree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Introduces a two-tier memory model: Tier 1 is the existing write-driven hot set bounded by memory_limit_bytes + hot_window_ms, Tier 2 is a separate read-side LRU of decoded segments bounded by a new segment_cache_bytes budget. Keeps the flusher purely write-driven (monotonic tiering, no query-frequency feedback into retention) while giving cold-but-repeatedly-queried segments a place to live in RAM. Tier 2 is segment-granular, never dirty, drop-anytime, and completely independent of the flusher's decisions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sealed epochs are frozen once the rotator seals them — the flusher can clone the Arc<Epoch> out under a brief read lock, do I/O entirely outside any store lock, then splice the Arc out under a brief write lock. No lock is ever held across disk I/O, so there is no task on the flusher's runtime waiting for it to yield, which removes the only reason to prefer async I/O. v1 ships plain std::fs on a dedicated std::thread; no Tokio runtime for the flusher. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolves all six previously-open questions in favor of the
performance-optimal answer instead of the simplest-v1 answer:
1. Segment format — 8-byte aligned, mmap-friendly, fallocate'd custom
binary (unchanged direction, concrete details added).
2. Flush I/O — sync std::fs with group-commit fdatasync batching
across a tick; no tokio::fs, no async runtime for the flusher.
3. Cold data retention — new delete_older_than_ms config knob and a
phase-3 retention sweep in the flusher loop, bounding the manifest
and keeping long-running deployments fast.
4. Flush fairness — round-robin across agg-ids, oldest-first within
each agg, replacing strict-global-oldest-first. Prevents per-agg
RwLock hot-spotting during hot-agg bursts.
5. Segment cache default — min(10% * memory_limit_bytes, 512 MiB),
scaling with the write budget instead of a fixed 64 MiB.
6. Tier-2 algorithm — W-TinyLFU via moka from day one, replacing
plain LRU. Scan-resistant, better hit rate on dashboard / recording-
rule access patterns, drop-in API.
Replaces the "Open questions" section with a "Resolved decisions" table
pointing at the section where each decision's rationale lives.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces one-file-per-sealed-epoch + dir-per-agg + JSON manifest with
the layout every mainstream TSDB converges on:
* Unit of file is a "part" — one directory per flush tick containing
meta.bin + data.bin + index.bin. All of the tick's candidate epochs
are bundled into a single data.bin regardless of which agg-id they
came from, and are locatable via a sorted, mmap-friendly index.bin
that supports O(log N) binary search on (agg_id, start_ms).
* File count scales with flush ticks, not with epochs. One-file-per-
epoch produced ~288K files/day on a 200-agg 1-minute-window setup;
the parts layout produces ~86K files (three per tick) with a
natural group-commit of fdatasync amortized over the whole tick.
* Global manifest becomes an append-only parts_manifest.log with a
periodic binary parts_manifest.snapshot, replacing the JSON file
that was rewritten in full every tick. Size is proportional to
flush ticks, not to epochs that have ever existed, and the
snapshot is mmap-cast-to-slice on startup (zero parse).
* T2 retention becomes whole-part rm -rf on tight time ranges, since
each part covers ~flush_interval_ms of data.
* Tier-2 cache is now keyed on PartId and holds mmap'd part views;
config knob renamed segment_cache_bytes -> part_cache_bytes.
Updates flusher pseudocode, flush_and_evict narrative, query path,
recovery sequence, concurrency summary, phasing, and adds entry #7 to
the Resolved decisions table. Compaction of adjacent small parts is
noted as a v2 follow-up — the layout accommodates it cleanly but v1
ships without it since T2 + a reasonable flush interval keeps part
count well within what a binary-searched Vec<PartEntry> handles.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Self-contained persistence submodule under simple_map_store/persistence/:
* config.rs — SimpleMapStorePersistenceConfig with memory budget
(high/low watermarks + hard cap), hot_window_ms, delete_older_than_ms,
flush_interval, disk_path, part_cache_bytes.
* part.rs — on-disk part format (meta.bin + data.bin + index.bin).
PartWriter bundles a slice of EpochSnapshots into a directory with
streaming crc32, 8-byte aligned payloads, dir fsync. PartReader
mmaps all three files, verifies index CRC on open, and resolves
entries lazily via load_entry().
* manifest.rs — append-only parts_manifest.log + periodic binary
parts_manifest.snapshot. append_add / append_delete are fsync'd;
compact() atomically rewrites the snapshot and truncates the log
once it grows past 4x the live-set size.
* source.rs — EpochSource trait + EpochSnapshot types. The contract
the flusher uses to enumerate, snapshot, and evict sealed epochs.
Decouples flusher from SimpleMapStorePerKey so the flusher can be
unit-tested against a fake source.
* flusher.rs — std::thread-based background loop. Three phases per
tick (memory pressure first, hot_window second, T2 retention
sweep third), round-robin across agg-ids within a phase. One part
per tick — group-commit fsync is intrinsic to the layout. Handle
is Drop'd to shut down cleanly.
* cache.rs — moka::sync::Cache<PartId, Arc<PartReader>> with weight
based on mmap bytes. 0-budget disables the cache; default sizing
scales with memory_limit_bytes.
* recovery.rs — load manifest at startup, verify every live part's
meta.bin, append delete records for corrupt entries, sweep orphan
part dirs not referenced by the manifest.
12 unit tests cover: part round-trip, CRC detection of meta
corruption, empty-snapshot rejection, manifest append + reload,
compact + log truncation, overlap filtering, memory-pressure flush,
hot-window flush, T2 sweep, empty-dir recovery, orphan sweep, corrupt
part eviction.
Adds three deps: moka 0.12 (sync, weight-based W-TinyLFU), memmap2
0.9 (zero-copy part reads), crc32fast 1.4 (streaming CRCs).
Not yet wired into SimpleMapStorePerKey — follow-up commit.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y read-through
Wraps SimpleMapStorePerKey's fields in an Arc<PerKeyInner> so the
background flusher can share state with the query path, and adds a
new persistence-aware constructor:
pub fn with_persistence(
streaming_config: Arc<StreamingConfig>,
cleanup_policy: CleanupPolicy,
persistence_cfg: SimpleMapStorePersistenceConfig,
) -> PersistResult<Self>
The existing `new()` / `new_with_strategy()` are unchanged — all 7
call sites in main.rs, bins, test helpers, and benches continue to
construct in-memory-only stores with no code change.
Behavior when persistence is enabled:
* Startup runs recovery::recover() against disk_path (orphan sweep +
CRC verification of any live parts left by a prior run), then
starts a FlusherHandle against Arc<PerKeyInner>.
* Insert path always runs maybe_rotate_epoch() regardless of
CleanupPolicy, so sealed epochs accumulate for the flusher to
pick up. Falls back to PERSISTENCE_DEFAULT_EPOCH_CAPACITY (1024)
when the streaming config does not set num_aggregates_to_retain.
* maybe_rotate_epoch() with persistence_enabled=true skips its
destructive eviction step — the flusher handles eviction via its
memory-pressure / hot-window / T2 triggers instead.
* cleanup_old_aggregates() is a no-op when persistence is on.
* query_precomputed_output() extends the existing in-memory merge
with a disk read-through: walks the parts manifest for
overlapping parts, resolves each via the moka Tier-2 cache,
decodes matching entries with accumulator_serde::
deserialize_accumulator(), and merges into the result map.
* Drop shuts the flusher down before the Arc<PerKeyInner> can be
released, guaranteeing no torn-destruction races.
EpochSource is implemented on PerKeyInner:
* list_sealed_epochs() walks the DashMap and returns one
SealedEpochRef per sealed epoch, with a coarse APPROX_BYTES_PER_
SKETCH estimate.
* snapshot_sealed_epoch() takes a per-agg read lock, clones entries
into EpochSnapshot form with labels resolved from the intern
table and sketches serialized via
serialize_accumulator_arroyo(), then drops the lock before
returning — no per-agg lock held across downstream disk I/O.
* evict_sealed_epoch() removes the sealed BTreeMap entry under a
write lock and prunes the corresponding read_counts.
v1 caveats documented in the source:
* query_precomputed_output_exact() only reads in-memory state. A
subsequent PR will extend the exact path with disk read-through.
* Memory accounting is coarse: entries * 4096 bytes. Per-accumulator
sizing (via a new AggregateCore::approx_memory_bytes trait method)
is a follow-up.
* Only AggregateCore types with working Arroyo round-trips in
accumulator_serde (Sum, KLL, HydraKLL, CountMinSketch, Set,
DeltaSet, MultipleSum, MultipleIncrease) are persistable. Others
are logged and skipped in query_disk_parts().
Three integration tests under tests/persistence_integration_tests.rs:
* with_persistence_flushes_sealed_epochs_to_disk — construct,
insert, wait for flush, verify in-memory entries have decreased
while the query still returns all inserted buckets.
* query_read_through_merges_memory_and_disk_ranges — 6 windows,
partial range query, verify filter is honored regardless of
whether the hit came from memory or disk.
* construct_and_drop_shuts_flusher_cleanly — smoke test that Drop
joins the flusher thread without deadlock.
Full test suite: 450 passed, 0 failed, 5 ignored.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces the flat APPROX_BYTES_PER_SKETCH = 4096 constant in per_key.rs
with real per-accumulator estimates via a new trait method on
AggregateCore:
fn approx_memory_bytes(&self) -> usize
The default impl returns 4096 (unchanged behavior for any concrete
type that doesn't override it), and 11 concrete accumulator types now
override it with type-aware estimates:
* SumAccumulator, MinMaxAccumulator, IncreaseAccumulator —
size_of::<Self>() for these scalars.
* MultipleSumAccumulator, MultipleMinMaxAccumulator —
~96 bytes per HashMap entry.
* MultipleIncreaseAccumulator — ~160 bytes per entry
(HashMap<Key, IncreaseAccumulator>).
* SetAggregatorAccumulator, DeltaSetAggregatorAccumulator —
~96 bytes per HashSet entry.
* CountMinSketchAccumulator — 16 KiB conservative constant.
* DatasketchesKLLAccumulator — 4 KiB (k=200 KLL).
* HydraKllSketchAccumulator — 32 KiB (row*col grid of KLLs).
per_key.rs changes:
* New epoch_approx_bytes(&SealedEpoch) -> usize helper that sums
each entry's approx_memory_bytes.
* Insert path pre-computes a batch sum once and bumps
mem_bytes_sealed by the real byte total instead of count *
constant. Each approx_memory_bytes call is O(1) or a single
field read, so the pre-sum adds only a handful of arithmetic
ops per item — no measurable hot-path cost.
* list_sealed_epochs, snapshot_sealed_epoch, evict_sealed_epoch
all now route through epoch_approx_bytes.
The numbers are still conservative estimates — accurate to ~1-2x, not
exact — but they give the flusher real proportional signal between
scalar accumulators, multi-valued aggregates, and large sketches,
which is what the memory-pressure trigger actually needs.
All 450 existing tests pass unchanged.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds the enum-level SimpleMapStore::with_persistence_per_key
wrapper plus --persistence-* CLI flags on both production binaries.
New public helper:
SimpleMapStore::with_persistence_per_key(
streaming_config, cleanup_policy, persistence_cfg,
) -> PersistResult<SimpleMapStore>
Always returns the PerKey variant — persistence only targets per-key
locking; the Global variant stays in-memory-only.
New CLI args (both main.rs and src/bin/precompute_engine.rs):
--persistence-enabled
--persistence-dir <PATH>
--persistence-memory-limit-mb <USIZE> (default: 2048)
--persistence-hot-window-secs <U64> (default: 3600, 0 disables)
--persistence-delete-older-than-secs <U64> (default: 604800, 0 disables)
--persistence-flush-interval-ms <U64> (default: 1000)
--persistence-part-cache-mb <U64> (default: min(10%*mem, 512))
When --persistence-enabled is passed, both binaries:
* Build a SimpleMapStorePersistenceConfig from the CLI args
(low_water = 80% of high, hard_cap = 125% of high).
* Call SimpleMapStore::with_persistence_per_key instead of
new_with_strategy.
* Force LockStrategy::PerKey (log an info message if --lock-strategy
was Global).
Unchanged when --persistence-enabled is off: the existing
new_with_strategy path runs exactly as before. No other binary
(test_e2e_precompute, bench_precompute_sketch,
e2e_quickstart_resource_test) is touched — those are test/bench paths
that don't need persistence.
All 450 existing tests still pass. Full bin build passes.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Four scenarios covering the hot paths exposed by the persistence
layer, runnable via:
cargo test --release -p query_engine_rust --lib \
tests::persistence_perf_tests -- --ignored --nocapture
1. insert_throughput_in_memory_vs_persistent — raw insert rate with
the in-memory store, with persistence enabled but flusher idle
(isolates insert-path overhead), and with persistence + aggressive
flushing (measures back-pressure cost).
2. query_latency_memory_only_vs_disk_through — p50/p90/p99 range
query latency with all data in memory vs. everything flushed and
read back via the moka part cache + disk mmap.
3. flush_throughput_sustained — insert a large batch, then poll
diagnostic_info until total_time_map_entries drops to the current
epoch's capacity (i.e., everything that can be flushed has been),
and report drain rate + MiB/s. The drain-done criterion uses
time-map entries because current-epoch bytes are counted in
total_sketch_bytes but never get flushed while hot.
4. memory_bound_adherence_under_overload — push 10x the memory
limit through the store and record the peak tracked bytes. Verifies
the flusher keeps memory roughly bounded without fully draining
to zero.
SumAccumulator throughout for simplicity; production sketch types
(KLL/CountMin/HydraKLL) would have different numbers because their
Arroyo serialize path is more expensive.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The v1 design reserved hard_cap_bytes as the insert-path blocking
ceiling but never wired it up — insert_for_store_key happily grew
mem_bytes_sealed past the limit. Perf test memory_bound_adherence
caught this at 1.53x the nominal high-water mark on a 10x overload.
This commit wires it up:
* FlusherHandle::wait_for_memory_under(mem_counter, cap, max_wait)
parks the calling thread on the existing pressure_cv/pressure_mutex
until `mem_counter.load() < cap` or the timeout expires. Kicks the
flusher once on entry so it runs immediately instead of waiting
for its own interval tick. Returns false on timeout so a stuck
flusher degrades (logged warning + insert proceeds) rather than
hanging the ingest thread.
* PerKeyInner gains `hard_cap_bytes: usize`, populated from the
persistence config in with_persistence and set to usize::MAX in
the in-memory-only constructor path (no blocking).
* insert_for_store_key checks mem_bytes_sealed against hard_cap_bytes
BEFORE taking the per-agg RwLock::write. Running the check ahead of
the lock means back-pressure on one agg-id doesn't stall unrelated
queries on the same agg while we wait.
* INSERT_BACK_PRESSURE_TIMEOUT = 30s — long enough for a healthy
flusher to always complete one tick, short enough that a genuinely
stuck flusher surfaces as a warning rather than a deadlock.
* PersistenceState._flusher renamed to .flusher since the insert
path now reaches it as well as Drop.
New integration test
hard_cap_back_pressure_blocks_inserts_until_flusher_drains:
- Configures memory_limit=512 B, hard_cap=640 B, 200ms flush tick.
- Inserts 200 items (~3200 B total) in tight batches.
- Asserts that at least one insert took ≥3 ms (unambiguous signal
of a condvar wait on a hot path where normal inserts are <1 ms).
- Reports median / max / slow-count in the failure message for
debugging if the back-pressure regresses.
Perf re-run of memory_bound_adherence_under_overload confirms the fix:
before: peak 390 KiB (1.53x high-water)
after: peak 320 KiB (1.25x high-water = hard_cap_bytes exactly)
The 1.25x ratio is the intended behavior — inserts block the moment
the counter reaches hard_cap, so it tracks the cap with bounded
overshoot from the in-flight batch. Insert throughput under overload
drops from 2.80 M/s to 1.06 M/s, which is the cost of back-pressure
under sustained overload and exactly what we want: a bounded memory
envelope with a deterministic latency impact, not unbounded growth.
Full suite: 451 passed, 0 failed, 9 ignored.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
force-pushed
the
design/simple-map-store-persistence
branch
from
April 14, 2026 17:21
761b525 to
96b13dc
Compare
zzylol
added a commit
that referenced
this pull request
Apr 14, 2026
…PR I)
Adds MessagePack as a parallel encoding option alongside the
protobuf-encoded sketchlib `*State` path that PR B and PR C wired up
for all five modified-OTLP `Metric.data` sketch variants. The new
encoding is a cross-language contract between DataCollector (Go,
sketchlib-go) and ASAPQuery-backend (Rust, sketch-core) — both sides
serialize the same cross-language sketch-core wire struct via
MessagePack. This gives sketchlib-go a practical alternative to
prost-encoded sketchlib proto without touching existing paths.
## Proto
Adds two new enum variants to each of the five per-sketch encoding
enums in the vendored `opentelemetry.proto.metrics.v1`:
* `*_ENCODING_MSGPACK = 3`
* `*_ENCODING_MSGPACK_DELTA = 4`
Backward-compatible: existing producers emitting tags 0-2 see no
change. The vendored proto is the consumer side — DataCollector's
upstream proto can adopt matching enum names in a parallel PR without
breaking either side, since protobuf enums are just integer tags.
## Dispatcher (`drivers/ingest/otel.rs`)
`decode_modified_otlp_sketch_bytes` now matches on the full `encoding`
i32 value rather than only accepting `PROTO = 1`:
* `PROTO = 1` → per-sketch `from_sketchlib_proto_bytes` (existing)
* `MSGPACK = 3` → per-sketch `from_msgpack_bytes` (new)
* `PROTO_DELTA = 2`, `MSGPACK_DELTA = 4` → deferred, caller falls
through to §5.2 fallback
## Per-sketch decoders
Each of the five accumulators gains a `from_msgpack_bytes(buffer)`
constructor that wraps the sketch-core `deserialize_msgpack` already
present on the underlying type:
* `CountMinSketchAccumulator::from_msgpack_bytes`
* `CountSketchAccumulator::from_msgpack_bytes`
* `DatasketchesKLLAccumulator::from_msgpack_bytes` — and this is
notable: unlike the `_ENCODING_PROTO` path (which does lossy
statistical reconstruction via `update()` replay because
sketchlib's `KllState` proto elides level structure the Rust
backend types keep private), the msgpack path is a **bit-identical
round-trip** because sketch-core's `KllSketch` serializes its
full internal state to msgpack. This gives Go producers a way to
send KLL sketches losslessly to the Rust backend.
* `DDSketchAccumulator::from_msgpack_bytes`
* `HllSketchAccumulator::from_msgpack_bytes`
## Tests
Unit tests — 6 new, 2 per decoder (round-trip + garbage rejection)
on `CountSketchAccumulator`, `DDSketchAccumulator`, and
`HllSketchAccumulator`. CountMin + KLL msgpack paths already had
coverage through `deserialize_from_bytes_arroyo` tests.
E2e test — new `e2e_count_min_sketch_msgpack_modified_otlp_path` in
`tests/e2e_modified_otlp_sketch_path.rs`. Builds a real
`CountMinSketch` in sketch-core, serializes via msgpack, POSTs through
the OTLP HTTP receiver with `encoding = COUNT_MIN_SKETCH_ENCODING_MSGPACK`,
closes a window, and verifies the stored accumulator preserves per-key
counts. Covers the dispatcher path end-to-end for the msgpack branch;
the other four variants share the same branch so per-variant smoke
tests aren't needed for dispatcher coverage (per-variant msgpack
round-trips are covered by the unit tests).
## Drive-by: fix two clippy warnings inherited from main
`cargo clippy --all-targets -- -D warnings` on rust 1.91 flags two
pre-existing issues in the persistence PR #4 code path:
* `cache.rs` unnecessary `u64 as u64` casts
* `part.rs` test `&[snap.clone()]` → `std::slice::from_ref(&snap)`
Fixed in this PR since the CI clippy gate would otherwise block PR I.
## Validation
- cargo check --all-targets: clean
- cargo clippy --all-targets -- -D warnings: clean
- cargo fmt --check: clean
- cargo test -p sketch-core --lib: 54 passed
- cargo test -p query_engine_rust --lib: 480 passed (includes 6 new msgpack unit tests)
- cargo test -p query_engine_rust --test e2e_modified_otlp_sketch_path: 6 passed (5 existing PROTO variants + 1 new MSGPACK variant)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 14, 2026
The existing `hard_cap_back_pressure_blocks_inserts_until_flusher_drains` test asserts that at least one insert takes ≥3 ms wall-clock time when sealed memory hits the hard cap — the intuition being that a blocked insert must wait ≥1 flush tick for the condvar to fire. On CI (fast runners, small test data) the flush tick completes well under 1 ms, so the slow path runs and fully drains before the 3 ms threshold ever trips. Result: the assertion is flaky even though back-pressure is wired up correctly. Replace wall-clock timing with a dedicated monotonic counter. ## Changes * `FlusherShared` gains `back_pressure_wait_count: AtomicU64` * `FlusherHandle::wait_for_memory_under` increments the counter exactly once per call that observes `mem_counter >= cap` (i.e. every call that enters the blocking wait loop — the fast-path early return does not bump the counter) * `FlusherHandle::back_pressure_wait_count()` getter exposes it * `SimpleMapStorePerKey::back_pressure_wait_count()` proxies through to the flusher, returning 0 when persistence is disabled * The test now asserts `store.back_pressure_wait_count() > 0` after the 200-insert loop — a timing-independent signal ## Why a counter, not a timing threshold Tests that assert "this path is slower than X ms" assume the path is slow for a reason other than what it's actually measuring. Here, `wait_for_memory_under` blocks until the flusher drains, and the drain is fast, so the total wait can be sub-millisecond even though the mechanism is firing exactly as designed. Timing thresholds trade signal for noise on fast hardware. A counter sidesteps that tradeoff: it is 0 if the mechanism never fires and positive if it fires even once — exactly the semantic the test cares about. ## Validation Ran 5× back-to-back: all 5 passed (previously observed flake on CI). Full query_engine_rust --lib suite: 480 passed. Clippy + fmt clean. Drive-by fix: inherited from main (the persistence PR #4 merge), not introduced by this PR. Landing it here because the PR is already touching enough persistence-adjacent surface (clippy fixes) that CI needs to be unblocked on the full test gate, and a real fix is cleaner than a retry loop or `#[ignore]`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 14, 2026
Adds `HotReloadStreamingConfig`, a thin `arc_swap::ArcSwap` wrapper,
plus `GET/POST /api/v1/streaming-config` HTTP endpoints so an external
controller (or a test harness) can push a new `StreamingConfig` at
runtime without restarting the query engine binary.
This is **phase 1** — the narrowest useful slice. The machinery exists
end-to-end (wire format, endpoint, swap), tests pin the contract, and
the controller has a working push target. Query execution and ingest
routing do NOT yet re-snapshot per request; those are phase 2 and are
documented on `HotReloadStreamingConfig` so the boundaries are clear.
## What's new
### `asap-query-engine/src/data_model/hot_reload_config.rs`
New module with `HotReloadStreamingConfig`, cloneable, built on
`Arc<ArcSwap<StreamingConfig>>`:
* `new(StreamingConfig)` / `from_arc(Arc<StreamingConfig>)` —
construct from initial config (two entry points so callers don't
have to double-allocate)
* `snapshot() -> Arc<StreamingConfig>` — cheap, lock-free read
* `swap(StreamingConfig) -> Arc<StreamingConfig>` — atomic replace,
returns the old `Arc` so callers can diff added/removed agg_ids
Four unit tests pin:
* initial snapshot reflects constructor
* swap replaces atomically (old handle unchanged, new reads fresh)
* clones share the underlying `ArcSwap` (so server-held handle and
test-held handle see the same swaps)
* concurrent reader never observes a torn state while a writer
swaps in a loop
### `GET / POST /api/v1/streaming-config` in `http.rs`
Two new routes on the existing control-plane surface, next to
`/api/v1/precompute`, `/api/v1/health`, `/api/v1/store/metrics`:
* **GET** — return the currently active config as JSON
(`{status, aggregation_count, aggregation_ids,
streaming_config}`). Used by tests and operators to verify a
push landed.
* **POST** — accept a YAML body matching the existing
`StreamingConfig::from_yaml_data` shape (the same format the
binary loads at startup), parse, validate, and atomically swap
via `HotReloadStreamingConfig::swap`. Returns `{status,
agg_ids_added, agg_ids_removed, new_aggregation_count}` so the
caller can confirm the transition without a round-trip to GET.
Logs a warning if any agg_ids were removed, since in-flight
precompute worker groups for those ids continue with their
construction-time config until they close naturally (phase 1
limitation).
Both endpoints return `503 Service Unavailable` when the server was
constructed without `HttpServer::with_hot_reload_config(...)` — a
clear failure mode for misconfigured deployments.
### `HttpServer::with_hot_reload_config(handle)` builder
New opt-in builder method on `HttpServer`. Without it, the endpoints
return 503 — so existing code paths (unit tests, legacy binaries) are
unaffected by this PR. `main.rs` calls it to attach the production
handle.
### `main.rs` wiring
Constructs a single `HotReloadStreamingConfig` from the startup
`Arc<StreamingConfig>` and passes it to `HttpServer::with_hot_reload_config`.
Other consumers (SimpleEngine, PrecomputeEngine, SimpleMapStore,
KafkaConsumer) still receive the startup `Arc<StreamingConfig>` and
will ignore swaps until phase 2.
## What's NOT hot-reloaded yet (documented on the module)
* **SimpleEngine query execution** — holds a startup snapshot,
doesn't re-snapshot per query. A query landing after a swap
sees the old config. Phase 2 will change this to re-snapshot on
query entry (one `Arc::clone` per query, negligible cost).
* **Ingest router / OTLP receiver** — routes by startup
`AggregationConfig` clones. New metric/labels mapped to new
agg_ids after a swap would not be routed until phase 2 wires
the router to re-snapshot per incoming message.
* **In-flight precompute worker `GroupState`** — each
`GroupState` caches `Arc<AggregationConfig>` at group creation.
Existing groups complete with their original config (correct
semantics, not a bug). New groups created after a swap use the
new config. This is the intended behavior for graceful add and
for param-tuning that only matters for future windows. The
only case where phase 1 is visibly incomplete is **removing**
an agg_id mid-window — the existing groups for that id keep
ingesting until their tumbling window closes. The POST handler
logs a warning when this happens.
## Drive-by fixes
Same two clippy-on-rust-1.91 issues in persistence code that
PR #9 also fixes:
* `persistence/cache.rs` — unnecessary `u64 as u64` cast
* `persistence/part.rs` — test `&[snap.clone()]` → `std::slice::from_ref(&snap)`
Inherited from main (persistence PR #4); fixed here so CI's clippy
gate passes on this branch too. These will be harmless duplicates if
PR #9 lands first.
## Tests
* **Unit tests** (4, in `hot_reload_config.rs`) — snapshot /
swap / clone-sharing / concurrent race.
* **HTTP integration tests** (3, in `http.rs::tests`):
- `test_streaming_config_hot_reload_round_trip`: starts a test
server with a hot-reload handle, POSTs a 2-agg YAML config,
asserts the POST response (agg_ids_added, new count), then
GETs and verifies the count + ids. Also asserts that the
externally-held `HotReloadStreamingConfig::snapshot()` sees
the swap, confirming the ArcSwap is shared.
- `test_streaming_config_hot_reload_missing_handle_503`: no
handle attached, both GET and POST return 503.
- `test_streaming_config_hot_reload_rejects_bad_yaml`: POST
with garbage YAML returns 400 + error message.
## Validation
* cargo check --all-targets: clean
* cargo clippy --all-targets -- -D warnings: clean
* cargo fmt --check: clean
* cargo test -p query_engine_rust --lib: 481 passed
## Follow-ups
* **Phase 2**: re-snapshot per query in `SimpleEngine`, per message
in the ingest router. Will likely change `SimpleEngine`'s field
from `Arc<StreamingConfig>` to `Arc<HotReloadStreamingConfig>`
and add a helper that snapshots at query entry.
* **Controller → backend channel**: once PR E's endpoint is merged,
the DataCollector controller can call it from its replanner
(`controller/src/replan.rs`) as a new transport alongside the
existing OpAMP push to agents. Tracked independently.
* **Graceful drain on removal**: PR E currently warns but does not
block config reloads that remove in-flight agg_ids. A stronger
guarantee would explicitly drain affected groups before
applying the swap.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
The existing `hard_cap_back_pressure_blocks_inserts_until_flusher_drains` test asserts that at least one insert takes ≥3 ms wall-clock time when sealed memory hits the hard cap — the intuition being that a blocked insert must wait ≥1 flush tick for the condvar to fire. On CI (fast runners, small test data) the flush tick completes well under 1 ms, so the slow path runs and fully drains before the 3 ms threshold ever trips. Result: the assertion is flaky even though back-pressure is wired up correctly. Replace wall-clock timing with a dedicated monotonic counter. ## Changes * `FlusherShared` gains `back_pressure_wait_count: AtomicU64` * `FlusherHandle::wait_for_memory_under` increments the counter exactly once per call that observes `mem_counter >= cap` (i.e. every call that enters the blocking wait loop — the fast-path early return does not bump the counter) * `FlusherHandle::back_pressure_wait_count()` getter exposes it * `SimpleMapStorePerKey::back_pressure_wait_count()` proxies through to the flusher, returning 0 when persistence is disabled * The test now asserts `store.back_pressure_wait_count() > 0` after the 200-insert loop — a timing-independent signal ## Why a counter, not a timing threshold Tests that assert "this path is slower than X ms" assume the path is slow for a reason other than what it's actually measuring. Here, `wait_for_memory_under` blocks until the flusher drains, and the drain is fast, so the total wait can be sub-millisecond even though the mechanism is firing exactly as designed. Timing thresholds trade signal for noise on fast hardware. A counter sidesteps that tradeoff: it is 0 if the mechanism never fires and positive if it fires even once — exactly the semantic the test cares about. ## Validation Ran 5× back-to-back: all 5 passed (previously observed flake on CI). Full query_engine_rust --lib suite: 480 passed. Clippy + fmt clean. Drive-by fix: inherited from main (the persistence PR #4 merge), not introduced by this PR. Landing it here because the PR is already touching enough persistence-adjacent surface (clippy fixes) that CI needs to be unblocked on the full test gate, and a real fix is cleaner than a retry loop or `#[ignore]`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 15, 2026
Adds `HotReloadStreamingConfig`, a thin `arc_swap::ArcSwap` wrapper,
plus `GET/POST /api/v1/streaming-config` HTTP endpoints so an external
controller (or a test harness) can push a new `StreamingConfig` at
runtime without restarting the query engine binary.
This is **phase 1** — the narrowest useful slice. The machinery exists
end-to-end (wire format, endpoint, swap), tests pin the contract, and
the controller has a working push target. Query execution and ingest
routing do NOT yet re-snapshot per request; those are phase 2 and are
documented on `HotReloadStreamingConfig` so the boundaries are clear.
## What's new
### `asap-query-engine/src/data_model/hot_reload_config.rs`
New module with `HotReloadStreamingConfig`, cloneable, built on
`Arc<ArcSwap<StreamingConfig>>`:
* `new(StreamingConfig)` / `from_arc(Arc<StreamingConfig>)` —
construct from initial config (two entry points so callers don't
have to double-allocate)
* `snapshot() -> Arc<StreamingConfig>` — cheap, lock-free read
* `swap(StreamingConfig) -> Arc<StreamingConfig>` — atomic replace,
returns the old `Arc` so callers can diff added/removed agg_ids
Four unit tests pin:
* initial snapshot reflects constructor
* swap replaces atomically (old handle unchanged, new reads fresh)
* clones share the underlying `ArcSwap` (so server-held handle and
test-held handle see the same swaps)
* concurrent reader never observes a torn state while a writer
swaps in a loop
### `GET / POST /api/v1/streaming-config` in `http.rs`
Two new routes on the existing control-plane surface, next to
`/api/v1/precompute`, `/api/v1/health`, `/api/v1/store/metrics`:
* **GET** — return the currently active config as JSON
(`{status, aggregation_count, aggregation_ids,
streaming_config}`). Used by tests and operators to verify a
push landed.
* **POST** — accept a YAML body matching the existing
`StreamingConfig::from_yaml_data` shape (the same format the
binary loads at startup), parse, validate, and atomically swap
via `HotReloadStreamingConfig::swap`. Returns `{status,
agg_ids_added, agg_ids_removed, new_aggregation_count}` so the
caller can confirm the transition without a round-trip to GET.
Logs a warning if any agg_ids were removed, since in-flight
precompute worker groups for those ids continue with their
construction-time config until they close naturally (phase 1
limitation).
Both endpoints return `503 Service Unavailable` when the server was
constructed without `HttpServer::with_hot_reload_config(...)` — a
clear failure mode for misconfigured deployments.
### `HttpServer::with_hot_reload_config(handle)` builder
New opt-in builder method on `HttpServer`. Without it, the endpoints
return 503 — so existing code paths (unit tests, legacy binaries) are
unaffected by this PR. `main.rs` calls it to attach the production
handle.
### `main.rs` wiring
Constructs a single `HotReloadStreamingConfig` from the startup
`Arc<StreamingConfig>` and passes it to `HttpServer::with_hot_reload_config`.
Other consumers (SimpleEngine, PrecomputeEngine, SimpleMapStore,
KafkaConsumer) still receive the startup `Arc<StreamingConfig>` and
will ignore swaps until phase 2.
## What's NOT hot-reloaded yet (documented on the module)
* **SimpleEngine query execution** — holds a startup snapshot,
doesn't re-snapshot per query. A query landing after a swap
sees the old config. Phase 2 will change this to re-snapshot on
query entry (one `Arc::clone` per query, negligible cost).
* **Ingest router / OTLP receiver** — routes by startup
`AggregationConfig` clones. New metric/labels mapped to new
agg_ids after a swap would not be routed until phase 2 wires
the router to re-snapshot per incoming message.
* **In-flight precompute worker `GroupState`** — each
`GroupState` caches `Arc<AggregationConfig>` at group creation.
Existing groups complete with their original config (correct
semantics, not a bug). New groups created after a swap use the
new config. This is the intended behavior for graceful add and
for param-tuning that only matters for future windows. The
only case where phase 1 is visibly incomplete is **removing**
an agg_id mid-window — the existing groups for that id keep
ingesting until their tumbling window closes. The POST handler
logs a warning when this happens.
## Drive-by fixes
Same two clippy-on-rust-1.91 issues in persistence code that
PR #9 also fixes:
* `persistence/cache.rs` — unnecessary `u64 as u64` cast
* `persistence/part.rs` — test `&[snap.clone()]` → `std::slice::from_ref(&snap)`
Inherited from main (persistence PR #4); fixed here so CI's clippy
gate passes on this branch too. These will be harmless duplicates if
PR #9 lands first.
## Tests
* **Unit tests** (4, in `hot_reload_config.rs`) — snapshot /
swap / clone-sharing / concurrent race.
* **HTTP integration tests** (3, in `http.rs::tests`):
- `test_streaming_config_hot_reload_round_trip`: starts a test
server with a hot-reload handle, POSTs a 2-agg YAML config,
asserts the POST response (agg_ids_added, new count), then
GETs and verifies the count + ids. Also asserts that the
externally-held `HotReloadStreamingConfig::snapshot()` sees
the swap, confirming the ArcSwap is shared.
- `test_streaming_config_hot_reload_missing_handle_503`: no
handle attached, both GET and POST return 503.
- `test_streaming_config_hot_reload_rejects_bad_yaml`: POST
with garbage YAML returns 400 + error message.
## Validation
* cargo check --all-targets: clean
* cargo clippy --all-targets -- -D warnings: clean
* cargo fmt --check: clean
* cargo test -p query_engine_rust --lib: 481 passed
## Follow-ups
* **Phase 2**: re-snapshot per query in `SimpleEngine`, per message
in the ingest router. Will likely change `SimpleEngine`'s field
from `Arc<StreamingConfig>` to `Arc<HotReloadStreamingConfig>`
and add a helper that snapshots at query entry.
* **Controller → backend channel**: once PR E's endpoint is merged,
the DataCollector controller can call it from its replanner
(`controller/src/replan.rs`) as a new transport alongside the
existing OpAMP push to agents. Tracked independently.
* **Graceful drain on removal**: PR E currently warns but does not
block config reloads that remove in-flight agg_ids. A stronger
guarantee would explicitly drain affected groups before
applying the swap.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 18, 2026
…iction prereq)
Adds the primitive the upcoming `SchemaEvictionService` will call to
reclaim space when a retired schema hits `expires_at_ms`. Analogous
to ClickHouse's `ALTER TABLE ... DROP PARTITION` — O(1)-ish key
removal, atomic w.r.t. concurrent reads of the same agg_id.
## What's landed
`Store` trait gains:
```rust
fn drop_agg_id(&self, _agg_id: u64)
-> Result<usize, Box<dyn std::error::Error + Send + Sync>> {
Ok(0)
}
```
Default impl returns `Ok(0)` so non-`SimpleMapStore` implementors
keep compiling — they can opt in later.
`SimpleMapStoreGlobal::drop_agg_id` and
`SimpleMapStorePerKey::drop_agg_id` override with real eviction:
* Count the windows being dropped (for the return value + audit log)
* Remove the per-agg_id entry from the main store map
* Clear `earliest_timestamp_per_aggregation_id[agg_id]`
* Clear `read_counts[agg_id]` (Global only — PerKey doesn't have it)
* Leaves `metrics` / `items_inserted` alone (those are metric-keyed,
not agg-keyed; other agg_ids under the same metric survive)
## Contract documented on the trait
* **Idempotent**: unknown `agg_id` is a no-op, returns `Ok(0)`.
* **Atomic w.r.t. reads for same agg_id**: Global grabs the store's
Mutex, PerKey uses DashMap's per-shard atomic remove.
* **Does NOT touch registries**: caller is responsible for removing
the schema / backfill-job entries.
## Test plan
7 new tests in `drop_agg_id_tests`:
- Global + PerKey variants × {removes target, unknown agg no-op,
clears earliest_ts index}.
- Drop-then-reinsert works as a fresh agg (no residual state).
710 lib tests total (up from 703); clippy + fmt clean.
## Next
PR #3: `BackfillRegistry::create_checked` gains `persistence_delete_older_than_ms`
check so jobs requesting `start_ms` outside the retention window get
rejected up-front with `CreateError::OutOfRetention`.
PR #4: `SchemaEvictionService` tokio task that consumes `drop_agg_id`
to actually clean up expired schemas.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Apr 18, 2026
…iction prereq) (#39) Adds the primitive the upcoming `SchemaEvictionService` will call to reclaim space when a retired schema hits `expires_at_ms`. Analogous to ClickHouse's `ALTER TABLE ... DROP PARTITION` — O(1)-ish key removal, atomic w.r.t. concurrent reads of the same agg_id. ## What's landed `Store` trait gains: ```rust fn drop_agg_id(&self, _agg_id: u64) -> Result<usize, Box<dyn std::error::Error + Send + Sync>> { Ok(0) } ``` Default impl returns `Ok(0)` so non-`SimpleMapStore` implementors keep compiling — they can opt in later. `SimpleMapStoreGlobal::drop_agg_id` and `SimpleMapStorePerKey::drop_agg_id` override with real eviction: * Count the windows being dropped (for the return value + audit log) * Remove the per-agg_id entry from the main store map * Clear `earliest_timestamp_per_aggregation_id[agg_id]` * Clear `read_counts[agg_id]` (Global only — PerKey doesn't have it) * Leaves `metrics` / `items_inserted` alone (those are metric-keyed, not agg-keyed; other agg_ids under the same metric survive) ## Contract documented on the trait * **Idempotent**: unknown `agg_id` is a no-op, returns `Ok(0)`. * **Atomic w.r.t. reads for same agg_id**: Global grabs the store's Mutex, PerKey uses DashMap's per-shard atomic remove. * **Does NOT touch registries**: caller is responsible for removing the schema / backfill-job entries. ## Test plan 7 new tests in `drop_agg_id_tests`: - Global + PerKey variants × {removes target, unknown agg no-op, clears earliest_ts index}. - Drop-then-reinsert works as a fresh agg (no residual state). 710 lib tests total (up from 703); clippy + fmt clean. ## Next PR #3: `BackfillRegistry::create_checked` gains `persistence_delete_older_than_ms` check so jobs requesting `start_ms` outside the retention window get rejected up-front with `CreateError::OutOfRetention`. PR #4: `SchemaEvictionService` tokio task that consumes `drop_agg_id` to actually clean up expired schemas. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
zzylol
added a commit
that referenced
this pull request
Apr 20, 2026
Addresses the ASAPQuery-backend TODO.md item #4 (serialization format versioning tests) — the paper blocker for "sketchDB restarts without data loss across format bumps." Adds `src/tests/persist_format_versioning_tests.rs` with 14 tests covering the three format-versioned stores: ## SchemaRegistry (5 tests) - realistic_multi_schema_round_trips_through_disk — 4-schema state with Active + Retired, drop + reload preserves timestamps and wire version - tampered_version_field_triggers_safe_fallback — field-level attack (valid v1 snapshot, flip version=999) must not leak tampered schemas; fallback re-writes clean v1 - truncated_snapshot_falls_back_without_panic — half-file scenario - empty_snapshot_file_falls_back_without_panic — zero-byte file - wrong_shape_snapshot_falls_back_without_panic — valid JSON with missing `schemas` array ## BackfillRegistry (5 tests) - realistic_multi_job_round_trips_through_disk — all five BackfillStatus variants (Queued/Running/Complete/Cancelled/ Failed) survive disk round-trip - v1_wire_format_carries_next_job_id_field — golden-file variant asserting next_job_id=43 is honoured so post-restart create() returns 43 not 1 - tampered_version_field_triggers_safe_fallback — same as schema - truncated_snapshot_falls_back_without_panic - wrong_shape_snapshot_falls_back_without_panic ## SimpleMapStore part meta.bin (4 tests) Previously-untested binary-header versioning: - bad_magic_returns_format_error — 0xDEADBEEF where MAGIC_META should be - unsupported_version_returns_format_error — PART_FORMAT_VERSION bumped in file to 9999 - truncated_header_returns_error — 32-byte header (spec says 64) - missing_meta_file_returns_error — part dir exists but meta.bin does not All three format stores are now locked under "bump-forward won't silently re-interpret old data" tests. Future format version bumps in any of the three modules will need to update these tests' expected-error assertions, forcing a conscious migration plan. 752 lib tests pass (+14), clippy clean, fmt clean. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 task
zzylol
added a commit
that referenced
this pull request
May 13, 2026
Post-M2.3 reorg #4 of 8. Moves `query_engines/asap_query_engine/warm_tier/` → `sketch_db/query/`. Pairs the warm-tier read code with the data it reads, mirroring how `backfill/` is paired with the data it writes. The warm-tier reducer is sketch-DB-specific (decodes sketches, runs them against a SketchStore) — it doesn't belong under the backend-agnostic `query_engines/` umbrella. `asap_query_engine/mod.rs` re-exports `pub use crate::storage_engines::sketch_db::query as warm_tier` so the engine's internal references (and any `warm_tier::*` callers) keep compiling at the legacy path. 5 files relocated. 783/783 lib tests pass. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
2 tasks
zzylol
added a commit
that referenced
this pull request
May 13, 2026
#185) Schema retirement #3. Repoint `ASAPQueryEngine::timeline_for_query` from `SchemaRegistry::timeline_for_metric` to the sid-level `storage_engines::sketch_db::query::timeline::timeline_for_metric` landed in #183. The cross-reconfigure dispatcher (`try_handle_query_promql_via_timeline`) now reads its segments from the sid catalog rather than from `SchemaRegistry`. The sid-level timeline populates `TimelineSegment.agg_id` with a content-hash of `(metric, agg_kind, group_by_keys)` rather than a `StreamingConfig.aggregation_id`. Until schema retirement #5 ports the per-segment dispatch to sid-level evaluation, the segment-→-aggregation_config lookup inside the dispatcher is best-effort: when no segment resolves to an in-config aggregation the dispatcher returns `None` so the caller falls back to the default single-agg path instead of regressing cross-reconfigure queries to empty-result-plus-warnings. The schema retirement plan keeps the `schema_registry` field on `ASAPQueryEngine` alive for now — it's still referenced by the ingest barrier and the swap-handler driver. Both go away in retirements #4 + #5. Two tests in `tests/schema_timeline_dispatch_tests.rs` are marked `#[ignore]`: they build two distinct `AggregationConfig`s with identical content (same metric / Sum / `host` grouping). In the sid catalog those collapse to one signature group → one segment, so the dispatcher can no longer reproduce the schema-boundary-stitch scenario from a SchemaRegistry-shaped fixture. The third single-schema regression test still passes unchanged. Re-enabling these is part of retirement #5 (sid-level dispatch) or a fixture rewrite that uses two genuinely distinct signatures. 787/787 lib tests pass; 5 ignored. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zzylol
added a commit
that referenced
this pull request
May 13, 2026
Schema retirement #4. Add the sid-level mirror of `SchemaRegistry::reconcile` under `sketch_db/lifecycle/reconcile.rs`: pub fn reconcile_from_streaming_config( store: &SketchStore, config: &StreamingConfig, retention: Duration, ) -> SidReconcileSummary Iterates every sid in `store`, computes its content signature `(metric_name, agg_kind, group_by_keys)`, and force-retires any sid whose signature is not represented in the new config. Mirrors the "retire orphans" half of the schema-version reconcile; the "add new ids" half is implicit in the sid model (sids are minted lazily by the ingest path on first write). Wired alongside the existing `SchemaRegistry::reconcile` at every reconcile call site: - `route_otlp_to_precompute` in `drivers/ingest/otel.rs` (raw-OTLP ingest path) - `route_modified_otlp_sketches_to_precompute` in `drivers/ingest/otel.rs` (modified-OTLP sketch path) - `streaming-config` swap handler in `drivers/query/servers/http.rs` (event-driven entry) Both registries run in parallel for now: the §6.3 ingest barrier `ingest_state.schemas.is_writable(agg_id)` still depends on `SchemaRegistry` lifecycle state, so we keep the schema reconcile alive until retirement #5 deletes the registry and replaces the barrier with a sid-level check. Six new unit tests cover empty-config, matching-signature, different-signature, idempotency (already-retired sid not re-retired), distinct-metrics isolation, and different-agg-type discrimination. Sketch-typed agg-configs are not yet covered (the `AggregationConfig` shape doesn't carry sketch params today — parallel capability-routing channel handles that lifecycle). 793/793 lib tests pass; 5 ignored. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
May 18, 2026
…281) The controller's collector-yaml emit used `HashMap<String, Value>` for `processors`, `receivers`, `extensions`, `connectors`, `exporters`, and `pipelines`. serde_yaml iterates a HashMap in Rust's randomized order, so calling `emit_edge_yaml` / `emit_edge_yaml_5sketch_routing` / `emit_gateway_yaml` twice on the same input produced byte-different YAML strings — same semantic content, different key ordering. This broke the agent's opampextension byte-level no-op check (ASAPCollector#381 + the Issue #4 follow-up): the agent saw "incoming bytes != on-disk bytes" on every reconnect, applied, restarted, the controller pushed the SAME semantic config again, and the agent looped. 10+ restarts per smoke test before any sketch could flush. Fix: switch the six HashMap fields on `CollectorYaml` / `ServiceSection` to `BTreeMap`. BTreeMap iterates in key order; serde_yaml's emit is now deterministic. Same semantic content always serializes to the same bytes. Bonus: this matches the controller's broader content-addressed identity story — `PolicyFingerprint::from_config` already sorts its hash inputs into canonical order, and `SeriesIdResolver` keys are sorted attribute fingerprints. The emit was the last non-deterministic hop. The agent's no-op check (ASAPCollector PR for Issue #4) is still useful as defense-in-depth — handles non-controller OpAMP servers that might still push slightly different bytes for the same intent. Test plan: * `cargo test -p control_plane --lib`: 706/706 pass. * Empirically verified pre-fix: `curl /api/v1/collector-config/agent` twice → key ordering differs (processors block: countmin / countsketch / transform/keep_for_* appear in different positions). Post-fix: identical bytes across calls. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 18, 2026
…_id to sid (B7.7) (#286) Third sub-step of schema-retirement #5 (issue #272), following B7.6's ingest-side rekey. The backfill processor's per-window grouping is now keyed by `sid: u64` instead of `group_key: String`, matching B7.6's sid-bucketed worker state. The SketchStore exposes a new sid-direct write path (`ingest_precompute_with_sid`) so callers that already hold the bucket sid skip the resolver round-trip inside the mint-driven `ingest_precompute_for_agg_config` wrapper. ## Sites rekeyed - `data_plane/src/storage_engines/sketch_db/backfill/processor.rs` - `process_window` now groups raw samples into a `HashMap<u64, SidBucket>` (sid-keyed) instead of `HashMap<String, Vec<RawSample>>` (group_key-keyed). - New helper `resolve_backfill_bucket_sid` mirrors `resolve_bucket_sid_for_agg_config` from `drivers/ingest/otel.rs` so backfill and live ingest mint the SAME sid for the same `(metric, grouping-values, agg_kind)` tuple. This is the invariant that lets backfill writes land in the same store row live ingest already populated for `[created_at, ∞)`. - Per-bucket writes go through the new `ingest_precompute_with_sid` path; the mint-driven sibling is no longer called from this file. - Resolver-less fallback (legacy / registry-only test setups) keeps a stable per-`group_key` bucket id so accumulator builds still preserve sample ordering — but the write itself is skipped in that branch anyway (no resolver ⇒ no precompute write, matching pre-B7.7 behaviour). - `data_plane/src/storage_engines/sketch_db/index/mod.rs` - New `pub fn ingest_precompute_with_sid(sid, agg_cfg, output, accumulator)` takes the bucket sid directly. The existing `ingest_precompute_for_agg_config` is refactored into a thin mint-driven wrapper that delegates to the new entry point — callers that don't yet hold the sid (the live `SketchStoreSink`) keep working unchanged. - Extracted `build_attrs_fp_and_label_map` shared by both methods so the mint-driven path (B7.6) and the sid-direct path (B7.7) stay byte-identical on the values they hand to the index. ## Tests added - `process_window_buckets_by_sid_via_resolver` — drives `process_window` end-to-end with two distinct svc values × two samples each, asserts exactly two sids land in the SketchStore, both `classify()` as `Hit`, and registry provenance is one entry per window. - `backfill_sid_matches_live_ingest_sid_for_same_grouping_values` — locks the live-vs-backfill sid namespace invariant: the sid the backfill helper computes for `(cfg, "latency{svc=a,zone=z0}")` must equal what the live ingest path's `resolve_bucket_sid_for_agg_config` mirror computes for the same `(metric, grouping-values, agg_kind)` tuple via the SAME resolver. ## Not in scope (deferred follow-ups) - `output_sink.rs` production code already consumes `output.policy_fp` (PR #284's report: "no changes needed there"). Its only `aggregation_id()` site is in a test that builds a `StreamingConfig` map keyed by policy_fp.as_u64() — the legitimate policy-registry use, not a bucket key. - `worker.rs` / `series_router.rs` / `drivers/ingest/otel.rs` are B7.6's domain (already merged) — not touched. - Remaining `aggregation_id()` accessor sites are all test-side `StreamingConfig` map-key uses (the map IS keyed by policy_fp.as_u64()) — those stay until the accessor itself is retired after #4 (re-enable ignored tests). ## Test plan - `cargo build -p data_plane` — clean - `cargo test -p data_plane --lib` — 715 passed / 2 ignored, no regressions vs origin/main - `cargo test -p data_plane` integration suite — same 2 pre-existing failures `controller_plan_to_query_full_roundtrip_ddsketch` / `_kll` PR #284 confirmed are pre-existing - Both new regression tests pass Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Design doc only — no implementation yet. Proposes a persistence layer for
SimpleMapStoreso it can honor a memory budget and flush cold sealedepochs to disk instead of either overprovisioning RAM or dropping data via
CleanupPolicy::CircularBuffer.Key decisions in the doc:
<disk_path>/agg_<id>/seg_<n>.bin+ atomicmanifest.json(write → fsync → rename). Orphan sweep on recovery.AggregateCore::approx_memory_bytes()(cheap per-type estimate) + oneAtomicUsizeon the store — not exact, good enough to drive policy.SimpleMapStorePersistenceConfigwith high-water / low-water / hard-cap memory budget,flush_older_than_ms, flush cadence, disk path.enabled = falseby default so existing deployments are unaffected.RwLock::writebriefly to splice the epoch out ofsealed_epochs. No lock held across fsync.merge_with.enabled = false.The doc ends with four open questions I'd like called before implementation:
tokio::fsfor flush I/O, sync locks elsewhere — OK?flush_older_than_msfold intoCleanupPolicy, or stay separate (my rec: separate — cleanup is destructive, persistence is not)?Test plan
asap-query-engine/src/stores/simple_map_store/and flag anything stale.No code changes in this PR — nothing to build or run.
🤖 Generated with Claude Code