Skip to content

feat: capability-miss → ControllerClient call-out (PR G) - #11

Merged
zzylol merged 1 commit into
mainfrom
feat/controller-client-capability-miss
Apr 15, 2026
Merged

zzylol merged 1 commit into
mainfrom
feat/controller-client-capability-miss

Conversation

@zzylol

@zzylol zzylol commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

PR G — closes the query plane's side of the feedback loop with the DataCollector controller.

The problem this solves

Today when SimpleEngine gets a query it can't answer from precomputed state — streaming_config.find_compatible_aggregation(&requirements) returns None — it silently falls through to the §5.2 fallback (direct Prometheus read, SQL forwarding) and returns whatever that gets. The controller never learns that the workload wants a sketch it doesn't currently have, so the gap persists across every future query of the same shape.

What this PR adds

Three call sites in simple_engine.rs (two SQL, one PromQL) now go through a helper:

fn find_compatible_aggregation_with_miss_notify(&self, requirements: &QueryRequirements)
    -> Option<AggregationIdInfo>
{
    let result = self.streaming_config.find_compatible_aggregation(requirements);
    if result.is_none() {
        spawn_capability_miss_notify(&self.controller_client, requirements);
    }
    result
}

On miss, the helper fires a fire-and-forget tokio::spawn task that POSTs a JSON payload (metric, statistics, data range, grouping labels, spatial filter) to the DataCollector controller's plan endpoint. The current query continues straight into the §5.2 fallback — it is not retried, not delayed, not blocked on the controller response.

The new pieces:

  • drivers/query/controller_client.rs (new module)

    • trait ControllerClient { async fn notify_capability_miss(&QueryRequirements) -> Result<(), String>; }
    • HttpControllerClient — HTTP impl with a 5s timeout so a slow or unreachable controller can't stall the spawned task. POSTs a flat JSON DTO projection of QueryRequirements — using a flat DTO avoids adding Serialize derives to shared asap_types / promql_utilities crates.
    • spawn_capability_miss_notify(&Option<Arc<dyn ControllerClient>>, &QueryRequirements) — fire-and-forget helper. Spawns via tokio::spawn. No-op when the option is None. Errors logged at WARN — capability misses must never fail the query.
    • MockControllerClient exported from tests so follow-up integration tests can compose a SimpleEngine with a fake client without needing an HTTP mock.
  • SimpleEngine::with_controller_client(...) builder — new field controller_client: Option<Arc<dyn ControllerClient>>, unset by default, so existing code paths are behaviorally unchanged.

  • --controller-endpoint <URL> CLI flag on main.rs. Default: unset. When set, main.rs constructs an HttpControllerClient and attaches it via SimpleEngine::with_controller_client(...).

How this closes the e2e loop with PR #10

PR #10 gave the backend a POST /api/v1/streaming-config endpoint so an external process can push a new StreamingConfig at runtime. This PR gives the controller a reason to call it:

query arrives → SimpleEngine miss
    ↓
PR #11 fires fire-and-forget POST to controller
    ↓
current query returns §5.2 fallback answer
    ↓
controller's replanner generates a new sketch plan
    ↓
controller POSTs the new plan to backend via PR #10's endpoint
    ↓
next query for the same shape finds a precomputed match

Why fire-and-forget instead of retry

Retrying the query after the plan lands would require waiting on:

  1. Controller plan-generation latency
  2. The push back to the backend
  3. At least one flusher tick before the new aggregation shows up in the worker pool

That coordination is out of scope for PR G. The fire-and-forget approach treats misses as telemetry that closes the loop over multiple query events rather than within a single query — the §5.2 fallback remains the correctness anchor, and the loop pays off on the second identical query, not the first.

Tests

Five new unit tests in controller_client.rs:

  1. payload_projects_all_requirement_fields — DTO conversion from QueryRequirements round-trips through JSON correctly
  2. spawn_helper_is_noop_when_client_is_none — safe to call with None
  3. spawn_helper_invokes_client_via_tokio_spawn — mock client confirms the fire-and-forget path fires end-to-end
  4. http_client_reports_non_success_status — real axum mock returning 500 → client maps to formatted Err
  5. http_client_success_path_round_trips_payload — real axum mock parses the posted JSON and asserts field-by-field

Drive-by

Same two clippy-on-rust-1.91 issues in persistence code that PR #9 and PR #10 also fix:

  • persistence/cache.rs — unnecessary u64 as u64 cast
  • persistence/part.rs — test &[snap.clone()]std::slice::from_ref(&snap)

Harmless duplicate when PR #9 lands first.

Validation

  • cargo check --all-targets: clean
  • cargo clippy --all-targets -- -D warnings: clean
  • cargo fmt --check: clean
  • cargo test -p query_engine_rust --lib: 479 passed (up 5 from main — the new unit tests)

Stack

  • PR #9 — PR I: MessagePack encoding parity
  • PR #10 — PR E: StreamingConfig hot-reload phase 1 (the controller's push target for the plan generated in response to this PR's notification)
  • PR feat: capability-miss → ControllerClient call-out (PR G) #11 (this PR) — PR G: capability-miss → ControllerClient notification
  • DataCollector #154 — proto MSGPACK enum names
  • DataCollector #155 — PR F: OpAMP config push architecture docs

Follow-ups

  • Query retry — wire an optional retry after the plan lands and the backend's StreamingConfig reflects the new plan. Needs PR E phase 2 (per-query re-snapshot in SimpleEngine). Retry-once, bounded wait, fall back to §5.2 on timeout.
  • Deduplication / rate-limiting — LRU + "seen in last N seconds" filter to prevent the same requirements from spawning notifications repeatedly under load.
  • Retry telemetry — (miss → notify → plan landed) latency tracking to tell whether the loop closes fast enough to matter.

🤖 Generated with Claude Code

When SimpleEngine fails to match a query against any stored
aggregation — `StreamingConfig::find_compatible_aggregation` returns
`None` — the query plane now fires a fire-and-forget notification to
the DataCollector controller so the controller can generate a new
sketch plan and push it back via PR E's `/api/v1/streaming-config`
endpoint.

The query itself is **not** retried. It continues to fall through to
the existing §5.2 fallback (direct Prometheus read, SQL forwarding,
etc.) and returns whatever the fallback provides. Future queries
benefit once the new plan lands. Wiring the query retry into the plan
generation latency is explicitly out of scope — the fire-and-forget
model sidesteps the coordination problem and keeps the §5.2 fallback
as the correctness anchor.

## New module: `drivers/query/controller_client`

A thin transport-agnostic abstraction:

  ```rust
  #[async_trait]
  pub trait ControllerClient: Send + Sync {
      async fn notify_capability_miss(
          &self,
          requirements: &QueryRequirements,
      ) -> Result<(), String>;
  }
  ```

Plus:

  * `HttpControllerClient` — HTTP impl with a 5-second timeout so a
    slow or unreachable controller can't stall the spawned task.
    POSTs a flat JSON payload projecting `QueryRequirements` into
    serde-friendly fields (metric, statistics as `Debug` strings,
    data_range_ms, grouping_labels as `Vec<String>`,
    spatial_filter_normalized). Using a flat DTO avoids adding
    `Serialize` derives to shared `asap_types` / `promql_utilities`
    crates.

  * `spawn_capability_miss_notify(&Option<Arc<dyn ControllerClient>>,
    &QueryRequirements)` — fire-and-forget helper used by the query
    hot path. Spawns via `tokio::spawn` so the query return path is
    never blocked on network I/O. No-op when the option is `None`.
    Errors are logged at WARN level — capability misses must never
    fail the query.

## SimpleEngine wiring

  * New field `controller_client: Option<Arc<dyn ControllerClient>>`
  * New builder method `with_controller_client(client)` —
    unset by default, so existing code paths (unit tests, callers
    that don't configure a controller) are behaviorally unchanged.
  * New helper `find_compatible_aggregation_with_miss_notify` —
    wraps `streaming_config.find_compatible_aggregation` with the
    fire-and-forget notification on `None`.
  * Three capability-miss call sites updated to use the helper:
      - SQL temporal/spatial  (line ~1980)
      - SQL spatio-temporal   (line ~2130)
      - PromQL                (line ~2755)

## main.rs

New CLI flag:

  ```
  --controller-endpoint <URL>
      DataCollector controller endpoint for capability-miss
      notifications. When set, SimpleEngine fires a fire-and-forget
      POST on every miss. When unset (default), misses fall through
      to the §5.2 fallback silently.
  ```

When set, main.rs constructs an `HttpControllerClient` and attaches
it via `SimpleEngine::with_controller_client(...)`.

## Tests

Five new unit tests in `controller_client.rs`, covering:

  1. `payload_projects_all_requirement_fields` — DTO conversion from
     `QueryRequirements` round-trips through JSON correctly
  2. `spawn_helper_is_noop_when_client_is_none` — fire-and-forget
     helper is safe to call with `None`
  3. `spawn_helper_invokes_client_via_tokio_spawn` — with a mock
     client, the spawned task actually fires and is observable via
     a shared counter (exercises the fire-and-forget path end-to-end)
  4. `http_client_reports_non_success_status` — real axum mock
     server returning 500, asserts the client maps the status to
     a formatted `Err`
  5. `http_client_success_path_round_trips_payload` — real axum
     mock server, client POSTs a capability-miss notification, the
     server parses the JSON body and asserts the fields match the
     source `QueryRequirements`

A public `MockControllerClient` is also exported from the test
module so follow-up tests in other crates (or other modules) can
compose a SimpleEngine with a fake client without needing an HTTP
mock.

## Drive-by

Same two clippy-on-rust-1.91 issues in persistence code that PR #9
and PR E also fix:

  * `persistence/cache.rs` — unnecessary `u64 as u64` cast
  * `persistence/part.rs` — test `&[snap.clone()]` →
    `std::slice::from_ref(&snap)`

Inherited from main; fixed here so CI's clippy gate passes on this
branch too. Harmless duplicate when PR #9 lands first.

## Validation

  * cargo check --all-targets: clean
  * cargo clippy --all-targets -- -D warnings: clean
  * cargo fmt --check: clean
  * cargo test -p query_engine_rust --lib: 479 passed (up 5 from
    main — the new unit tests)

## Follow-up

  * **Query retry** — wire an optional retry after the controller
    plan lands and the backend's StreamingConfig reflects the new
    plan. Needs PR E phase 2 (hot-reload picked up by SimpleEngine
    at query time) to be useful. Retry-once semantics, bounded wait,
    with a clear fallback to the §5.2 path if the plan never lands.
  * **Deduplication / rate-limiting** — the current fire-and-forget
    spawns one notification per miss. Under heavy miss load on the
    same `QueryRequirements`, the controller may be asked to plan
    repeatedly. A simple LRU + "seen in last N seconds" filter in
    `HttpControllerClient` would prevent this.
  * **Retry telemetry** — track (miss → notify → plan landed) latency
    across queries so we can actually tell if the loop closes fast
    enough to matter.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@zzylol
zzylol force-pushed the feat/controller-client-capability-miss branch from f9d86f0 to c049f25 Compare April 15, 2026 16:58
@zzylol
zzylol merged commit 56c8bba into main Apr 15, 2026
5 of 6 checks passed
@zzylol
zzylol deleted the feat/controller-client-capability-miss branch April 15, 2026 18:46
zzylol added a commit that referenced this pull request Apr 15, 2026
Makes `POST /api/v1/streaming-config` actually useful for query-time
behavior. Before this PR, SimpleEngine snapshotted the startup config
into `Arc<StreamingConfig>` once at construction and ignored every
subsequent swap — meaning a controller push via PR #10's endpoint
was observable only by the GET debug endpoint, not by queries.

Phase 2 threads the shared `HotReloadStreamingConfig` handle all the
way into SimpleEngine and re-snapshots per query:

  main.rs builds one HotReloadStreamingConfig
    ├─> HttpServer::with_hot_reload_config(handle.clone())
    │       ↑ POST /api/v1/streaming-config swaps via this handle
    │
    └─> SimpleEngine::new_with_hot_reload(handle.clone())
            ↑ each query calls streaming_config_snapshot() which
              load_full()s from the same underlying ArcSwap

Because `HotReloadStreamingConfig` clones share the same inner
`Arc<ArcSwap<StreamingConfig>>`, a swap on one clone is immediately
visible through the other. That's the entire mechanism.

## Changes

### `SimpleEngine`

  * Field `streaming_config: Arc<StreamingConfig>` → `streaming_config_source: HotReloadStreamingConfig`
  * New constructor `SimpleEngine::new_with_hot_reload(...)` takes the
    shared handle. `main.rs` uses this path.
  * Existing `SimpleEngine::new(...)` signature preserved for tests,
    binaries, and legacy callers. Internally wraps the provided
    `Arc<StreamingConfig>` in a FRESH `HotReloadStreamingConfig` so
    external unrelated handles don't leak in — the `legacy_new_constructor_is_independent_of_external_handle`
    test pins this behavior.
  * New accessor `streaming_config_snapshot(&self) -> Arc<StreamingConfig>`.
    Each call observes whatever was most recently pushed via PR #10.
    Doc comment explains the per-query-binding discipline internal
    callers must follow.

### 13 internal call sites in `simple_engine.rs`

Each site that reads from `streaming_config` is converted to use
either:

  * Inlined `self.streaming_config_snapshot().get_aggregation_config(...).map(|c| c.X.clone())...`
    when the access is a single-expression chain whose result copies
    out of the borrowed `AggregationConfig` (e.g. `window_size * 1000`
    or a `.clone()` on an owned field).
  * A per-helper `let streaming_config = self.streaming_config_snapshot();`
    binding when a borrowed `&AggregationConfig` outlives the initial
    expression (e.g. `create_store_query_plan`, which binds the
    config once and reads multiple fields from it). The binding
    ensures a consistent view for that helper's entire execution
    and keeps the borrowed reference valid through the local Arc's
    lifetime.

Per-query entry points do NOT bind a single top-level snapshot today.
Instead each helper re-snapshots independently. Under a concurrent
swap that races with a running query, two helpers in the same query
could see different configs — but both views are internally
consistent, and the resulting query answer remains correct under
EITHER view (old or new). Swaps are rare; the torn-state window
is microseconds. Binding a single per-query snapshot and plumbing
it through helper signatures would be a much larger refactor and
is tracked as phase 3.

### `main.rs`

Switches from `SimpleEngine::new(streaming_config.clone(), ...)` to
`SimpleEngine::new_with_hot_reload(hot_reload_config.clone(), ...)`.
The same `HotReloadStreamingConfig` handle now flows to both the HTTP
server (which handles POST) and the engine (which reads), so a POST
is observable by the next query.

## Tests

Three new unit tests in a `hot_reload_phase2_tests` module at the
end of `simple_engine.rs`:

1. `streaming_config_snapshot_starts_at_initial_config` — baseline
   sanity check that the initial config is observable through the
   new accessor.

2. `streaming_config_snapshot_observes_post_construction_swap` —
   **the core PR E phase 2 guarantee**: build the engine with a
   shared handle, swap the handle from outside, call the accessor
   again, and verify the new config is visible. Also asserts the
   previous snapshot remains internally consistent (per-query
   stability).

3. `legacy_new_constructor_is_independent_of_external_handle` —
   verifies that the backwards-compat `SimpleEngine::new` path
   snapshots into a FRESH internal `HotReloadStreamingConfig`, so
   a swap on an unrelated external handle does NOT leak into
   test/bin-only constructions.

## Validation

  * cargo check --all-targets: clean
  * cargo clippy --all-targets -- -D warnings: clean
  * cargo fmt --check: clean
  * cargo test -p query_engine_rust --lib: 495 passed (up 3 from
    main — the phase 2 unit tests)

## What closes now

Paired with PR #10 (endpoint) and PR #11 (capability-miss notify),
phase 2 closes the query-side of the control loop end-to-end:

  query hits SimpleEngine miss
    → PR #11 fires fire-and-forget POST to controller /api/v1/plan
    → §5.2 fallback serves the current query
    → controller generates new plan
    → controller POSTs to backend /api/v1/streaming-config (PR #10)
    → swap lands in the shared HotReloadStreamingConfig
    → next query re-snapshots via PR E phase 2 and finds a match

## What's still NOT hot-reloaded

  * **Ingest router / OTLP receiver** — still routes by startup
    `AggregationConfig` clones. Adding a new agg_id via POST does
    not yet cause incoming metrics to be routed into that agg_id
    on the ingest side. Tracked as phase 3.
  * **In-flight precompute worker GroupState** — unchanged from
    phase 1. Existing groups complete with construction-time config;
    new groups pick up the new config. Intended semantics.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 11, 2026
… + typed Predicate (#136)

Additive lift of all 10 A-classified legacy variants into the canonical
`intent_algebra::query_expr::QueryExpr`, plus the minimal typed
`Predicate` IR for `Filter.pred` covering the 4 used-and-cleanly-shaped
ScalarExpr variants.

## What landed

### Canonical `QueryExpr` grew from 5 → 15 variants

Pre-existing (PR #130 / canonical): `Scan`, `Window`, `Aggregate`,
`LetBinding`, `Ref`.

New (this PR): `Filter`, `Project`, `Partition`, `Distinct`, `Merge`,
`Join`, `SetOp`, `Sort`, `Limit`, `BinaryOp`.

Naming convention: single-input variants use `child:` (matches existing
canonical `Window`/`Aggregate`/`LetBinding`). Multi-input variants
follow design.md §6 shapes exactly: `Merge { children }`,
`Join { kind, pred, left, right }`, `SetOp { kind, all, left, right }`,
`BinaryOp { op, lhs, rhs, vector_match }`.

### Supporting types lifted alongside

`ColumnRef`, `PartitionKeys`, `BinaryOpKind`, `JoinKind`, `SetOpKind`,
`SortKey`, `VectorMatch`/`VectorMatchKind`/`VectorGrouping`/`GroupSide`,
`LiteralValue`, `ProjectItem`. All re-exported from
`intent_algebra::mod.rs`.

### New typed `Predicate`

```rust
pub enum Predicate {
    Column(ColumnRef),
    Literal(LiteralValue),
    BinaryOp { op: BinaryOpKind, lhs: Box<Predicate>, rhs: Box<Predicate> },
    IsNull { expr: Box<Predicate>, negated: bool },
}
```

Plus `Predicate::from_legacy_scalar(&legacy::ScalarExpr) ->
Result<Predicate, QueryExprError>`. Translates the 4 supported
variants (Column / Literal / BinaryOp / IsNull); returns
`QueryExprError::UnsupportedLegacyScalar(name)` for the 4 E-deferred
(FunctionCall / ScalarSubquery / InList / Between). Per user decision,
those stay in legacy until a real consumer demands the typed shape.

Translation handles `LiteralValue::Duration → Int(nanos)` fold (canonical
LiteralValue is deliberately narrower).

### Consumer migration deferred to subsequent batches

**Zero consumer-site redirects in this PR** — and that's the right
call. Every legacy-A-variant consumer (`query_parser/`, `physical/
{stage_split,planner,allocator}.rs`, `optimizer/engine.rs`,
`legacy_lower.rs`) simultaneously matches A-variants AND C-variants
(`Source`, `Aggregate`, `Window`, `SketchAgg`, `WindowedAgg`, `TopK`,
`HistogramQuantile`, `PromQLSubquery`) AND constructs `ScalarExpr`
predicates with E-deferred variants. Migrating them mid-pipeline would
require lifting C-variants and E-variants in the same PR — explicit
out-of-scope per the batch plan.

The consumers migrate one C-batch at a time:
- Batch 3a (PR #11): `AggFunc → AggIntent`
- Batch 3b (PR #10): `TopK` split
- Batch 3c (PR #8): `WindowedAgg` un-fusion
- Batch 3d (PR #9): `SketchAgg → Aggregate@L3`
- Batch 3e (PR #12): PromQL parser `histogram_quantile → Quantile`
- Batch 13: retire `legacy_expr.rs` + `legacy_lower.rs`

### Catch-all arms added in 6 canonical-side consumers

Files: `optimizer/cost/mod.rs`, `physical/colored_dag/{allocator,emitter}.rs`,
`sketch_algebra/lower.rs`, `warm_tier_analysis.rs`. These previously
matched canonical `QueryExpr` exhaustively over the 5 pre-existing
variants; now they need conservative handling for the 10 new ones
(Edge stage / Logical wrap / child-walk / zero-cost) with TODO
markers for the follow-up reshape batches.

### Serde tag note

Initially gave `ColumnRef`/`LiteralValue`/`PartitionKeys`/`Predicate`
internally-tagged `#[serde(tag = "kind", rename_all = "snake_case")]`
attrs, but serde rejects internally-tagged newtype variants containing
primitives (e.g. `Named(String)`). Switched to externally-tagged
`#[serde(rename_all = "snake_case")]`. Caught by the round-trip test.

## Build + test

- `cargo build --release -p controller` — clean
- `cargo build --release -p query_engine_rust` — clean
- `cargo test -p controller --lib` — **666 passed** (was 655 after
  Batch 1; +11 from new typed-Predicate + schema tests)
- `cargo test -p query_engine_rust --lib -- engines::warm_tier` — 13/13

## Diff: 8 files, +794 / -8

Co-authored-by: zz_y <zz_y@node0.zz-y-304941.softmeasure-pg0.clemson.cloudlab.us>
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant