refactor(data_plane): rekey backfill processor + SketchStore from agg_id to sid (B7.7) - #286
Merged
Merged
Conversation
…_id to sid (B7.7) 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 May 18, 2026
zzylol
added a commit
that referenced
this pull request
May 18, 2026
…legacy retirement (#289) The DDSketch + KLL roundtrip tests carried `start_time_unix_nano: 0` on the OTLP data points they POST to the backend. The HLL/CMS/CountSketch sibling tests already used `time_unix_nano - 1s` for this field (with an explanatory comment block). The two old tests were missed during that sweep. Why this caused the failure: After PR #280 deleted the legacy `handle_query` family, the modern trait-dispatched `execute()` path is the only query route. It calls `SketchStore::query_range(sid, t0_ms, t1_ms)` which delegates to `MutableEpoch::range_query_into`, which accepts only windows where `w.0 >= start && w.1 <= end`. The stored window is `(dp.start_time_unix_nano / 1e6, dp.time_unix_nano / 1e6)`, so a `start_time_unix_nano: 0` produces a window starting in 1970 — which fails `w.0 >= now_ms - lookback_ms` for any sensible `now`, and the reducer hits `NoData` → `CapabilityMiss` → HTTP `bad_data` / "No result for query". Diagnosis (option `(b)`): the tests' construction always had this latent bug, but the legacy `handle_query` path's range semantics happened to accept the (0, time) window — that side-effect is gone post-#280. Fix: mirror the HLL helper's `start_t_ns = time_unix_nano - 1s` pattern in `build_dd_sketch_export` + `build_kll_export`, and refresh the deferred-soft-check docstrings to describe the strict-success expectation. Identical comment block now sits above the `start_time_unix_nano` field in DDSketch / KLL / HLL / CountSketch / CountMin helpers. Production code untouched. Before / after: - before: `cargo test -p data_plane --test e2e_controller_plans_and_backend_serves` → 5 passed / 2 failed - after: 10 passed / 0 failed (the 7 roundtrip cases + 2 streaming-config cases + 1 range-query case all green) - `cargo test -p data_plane --lib` → 727 passed / 0 failed / 2 ignored (no regressions; lib count grew from 715 in #286 via unrelated work landed on main since) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 18, 2026
Closed
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.
Scope
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: u64instead ofgroup_key: String, matching B7.6'ssid-bucketed worker state. The SketchStore exposes a new sid-direct
write path (
ingest_precompute_with_sid) so callers that already holdthe bucket sid skip the resolver round-trip inside the mint-driven
ingest_precompute_for_agg_configwrapper.Sites rekeyed
data_plane/src/storage_engines/sketch_db/backfill/processor.rsprocess_windownow groups raw samples into aHashMap<u64, SidBucket>(sid-keyed) instead ofHashMap<String, Vec<RawSample>>(group_key-keyed).resolve_backfill_bucket_sidmirrorsresolve_bucket_sid_for_agg_configfromdrivers/ingest/otel.rsso backfill and live ingest mint the SAME sid for the same(metric, grouping-values, agg_kind)tuple. This invariant is what lets backfill writes land in the same store row live ingest already populated for[created_at, ∞).ingest_precompute_with_sidpath; the mint-driven sibling is no longer called from this file.group_keybucket 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.rspub fn ingest_precompute_with_sid(sid, agg_cfg, output, accumulator)takes the bucket sid directly. The existingingest_precompute_for_agg_configis refactored into a thin mint-driven wrapper that delegates to the new entry point — callers that don't yet hold the sid (the liveSketchStoreSink) keep working unchanged.build_attrs_fp_and_label_mapshared 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.Shape changes
BackfillWindowProcessor::process_windowinternal bucket map:HashMap<String, Vec<RawSample>>→HashMap<u64, SidBucket { group_key, samples }>.SketchStore::ingest_precompute_with_sid(sid: u64, &AggregationConfig, &PrecomputedOutput, &dyn AggregateCore) -> Option<u64>.SketchStore::ingest_precompute_for_agg_configbody collapsed to a sid-mint then delegate; external contract unchanged.Files touched
data_plane/src/storage_engines/sketch_db/backfill/processor.rs(+283/-37)data_plane/src/storage_engines/sketch_db/index/mod.rs(+91/-39)Tests added
process_window_buckets_by_sid_via_resolver— drivesprocess_windowend-to-end with two distinct svc values × two samples each, asserts exactly two sids land in the SketchStore, bothclassify()asHit, 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'sresolve_bucket_sid_for_agg_configmirror computes for the same(metric, grouping-values, agg_kind)tuple via the SAME resolver.Not in scope (deferred follow-ups)
output_sink.rsproduction code already consumesoutput.policy_fp(PR refactor(data_plane): rekey ingest bucketing from (agg_id, group_key) to sid (B7.6) #284's report: "no changes needed there"). Its only remainingaggregation_id()site is in a test that builds aStreamingConfigmap keyed bypolicy_fp.as_u64()— the legitimate policy-registry use, not a bucket key.worker.rs/series_router.rs/drivers/ingest/otel.rsare B7.6's domain (already merged) — not touched.aggregation_id()accessor sites are all test-sideStreamingConfigmap-key uses (the map IS keyed bypolicy_fp.as_u64()) — those stay until the accessor itself is retired after docs: design for SimpleMapStore persistence (mem limit + disk flush) #4 (re-enable ignored tests).Test plan
cargo build -p data_plane— clean (only pre-existing warnings)cargo test -p data_plane --lib— 715 passed / 0 failed / 2 ignored, no regressions vs origin/maincargo test -p data_planeintegration suite — same 2 pre-existing failurescontroller_plan_to_query_full_roundtrip_ddsketch/_kllthat PR refactor(data_plane): rekey ingest bucketing from (agg_id, group_key) to sid (B7.6) #284 confirmed are pre-existing on a freshorigin/mainworktree. Unrelated to B7.7.🤖 Generated with Claude Code