fix(backend): cms_params accepts canonical w/d + legacy row_num/col_num - #268
Merged
Merged
Conversation
Two `cms_params` helpers (accumulator_factory + accuracy) read the CMS / CountSketch dimensions from `AggregationConfig.parameters`. Pre-PR they only looked at the legacy `row_num` / `col_num` keys — the keys the control plane's `sketch_params_to_json` emits (and that PR #258's `sketch_config_to_params` standardized on for OTLP policy_fp content matching) are `w` / `d`. Result was a silent mismatch: the OTLP modified-sketch ingest path content-matched on `w` / `d`, but raw-input ingest (precompute worker + sketch_db backfill) read from `row_num` / `col_num` — meaning static streaming-config YAMLs that ship in asapcollector with `row_num` / `col_num` worked for raw-input ingest but failed policy_fp lookup on the modified-OTLP side, while controller- emitted configs with `w` / `d` worked for OTLP but defaulted to `(4, 1000)` on the raw-input side. Both helpers now read canonical `w` / `d` first and fall back to `row_num` / `col_num`. Both naming conventions work everywhere; old asapcollector configs keep working, controller-emitted configs keep working, and a follow-up asapcollector PR will migrate the static YAMLs to the canonical form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zzylol
added a commit
that referenced
this pull request
May 16, 2026
…ms (#269) Per-user direction: now that the asapcollector configs migrated to canonical `w` / `d` keys (sibling PR `sync-config-canonical-w-d`), the `row_num` / `col_num` fallback PR #268 added has no remaining caller. Drop it. Three sweeps: - `accumulator_factory::cms_params` — canonical keys only; `cms_params_reads_canonical_w_d_keys` test updated to drop the legacy half. - `storage_engines::sketch_db::accuracy::cms_params` — same. - `tests::accuracy_empirical_validation_tests` — six test fixtures migrated from `row_num` / `col_num` to `d` / `w` to match the helper's new contract. Remaining `row_num` / `col_num` mentions in the codebase are all internal Rust identifiers (function arguments / struct fields / locals describing the sketch matrix dimensions) — not config- parameter keys. Those are idiomatic and stay. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
May 17, 2026
…default (#270) * refactor(backend): drop row_num/col_num legacy fallback from cms_params Per-user direction: now that the asapcollector configs migrated to canonical `w` / `d` keys (sibling PR `sync-config-canonical-w-d`), the `row_num` / `col_num` fallback PR #268 added has no remaining caller. Drop it. Three sweeps: - `accumulator_factory::cms_params` — canonical keys only; `cms_params_reads_canonical_w_d_keys` test updated to drop the legacy half. - `storage_engines::sketch_db::accuracy::cms_params` — same. - `tests::accuracy_empirical_validation_tests` — six test fixtures migrated from `row_num` / `col_num` to `d` / `w` to match the helper's new contract. Remaining `row_num` / `col_num` mentions in the codebase are all internal Rust identifiers (function arguments / struct fields / locals describing the sketch matrix dimensions) — not config- parameter keys. Those are idiomatic and stay. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(deploy): MVP smoke-test blockers — dockerignore + OpAMP endpoint default Two deploy-side bugs found while bringing up the single-node MVP demo: 1. .dockerignore: top-level `target/` rule doesn't match subdirectory `target/` paths, so per-crate caches under `data_plane/target/` and `control_plane/target/` (~700 MB after any host-side `cargo build`) get shipped into the BuildKit context. The first `docker build -f Dockerfile.backend` hit over 7 GB of `transferring backend-src:` before BuildKit canceled. Add `**/target/`, `**/.git/`, editor caches, and `**/eval-results/` so the context shrinks back to source-only. 2. control_plane: default `CONTROLLER_OPAMP_ENDPOINT` is `ws://control_plane:4320/v1/opamp` (the post-reorg crate name), but the canonical compose stack in `ASAPCollector/deploy/mvp-singlenode/docker-compose/base.yml` still names the service `controller`. The endpoint string gets baked into every emitted agent yaml under `extensions.opamp.server.ws.endpoint`, so the broken default tears down OpAMP the moment the controller pushes a fresh config. Switch the default to `ws://controller:4320/v1/opamp`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Two
cms_paramshelpers (accumulator_factory+accuracy) read CMS / CountSketch dimensions fromAggregationConfig.parameters. Pre-PR they only looked at the legacyrow_num/col_numkeys — the keys the control plane'ssketch_params_to_jsonemits (and that PR #258'ssketch_config_to_paramsstandardized on for OTLP policy_fp content matching) arew/d.The silent mismatch
w/d(viasketch_config_to_params)row_num/col_num(viacms_params)Fix
Both helpers now read canonical
w/dfirst, then fall back torow_num/col_num. Both naming conventions work everywhere. Sibling asapcollector PR will migrate the static YAMLs to canonical form.Test plan
cms_params_accepts_w_d_canonical_and_row_col_num_legacycovers both naming conventions + the empty-params defaultcargo test --workspace --lib— 1534 pass, 0 failcargo test --test e2e_controller_plans_and_backend_serves— 10 pass🤖 Generated with Claude Code