feat: cluster variants, ADBC health/reconcile, and distributed runnin…#93
feat: cluster variants, ADBC health/reconcile, and distributed runnin…#93amitgilad3 wants to merge 1 commit into
Conversation
…g counts Expand one persisted cluster config into runtime warehouses (base::variant), with built-in SaaS introspection and optional healthCheckQuery/reconcileQuery. In distributed mode, reconcile publishes engine ground truth to cluster_capacity_counters.running; fleet admission stays on capacity leases. Includes Studio editors, Postgres migration, and architecture docs.
📝 WalkthroughWalkthroughThis PR adds cluster "variant" expansion allowing one persisted cluster configuration to expand into multiple runtime clusters via override merging, adds ADBC-specific introspection adapters (Databricks, Snowflake, BigQuery, Redshift) for health/reconcile probing without waking auto-suspending warehouses, refactors Postgres capacity leasing to use the lease table as source of truth, wires variant expansion and custom health/reconcile SQL through main.rs runtime loops, adds Studio UI for editing variants and custom queries, and documents the new architecture. ChangesCluster variants and health/reconcile system
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Studio
participant AdminAPI
participant MainReload
participant AdbcAdapter
participant CapacityStore
Studio->>AdminAPI: upsert cluster config with variants, healthCheckQuery, reconcileQuery
AdminAPI->>MainReload: trigger config reload
MainReload->>MainReload: expand_cluster_variants(base config)
MainReload->>AdbcAdapter: build adapter per expanded cluster
loop every 30s
MainReload->>AdbcAdapter: execute_custom_health_check or health_check()
end
loop every 30s (reconcile)
MainReload->>AdbcAdapter: execute_custom_reconcile_query or fetch_running_query_count()
AdbcAdapter-->>MainReload: running count
MainReload->>CapacityStore: publish_running_count(cluster, count)
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds “cluster variants” (expanding one persisted cluster config into multiple runtime clusters like base::variant), introduces ADBC health-check + running-count reconciliation (including SaaS-friendly introspection), and clarifies/implements distributed-mode semantics where admission uses fleet-wide leases while “running” reflects backend ground truth published via Postgres.
Changes:
- Implement cluster variant expansion across config load/reload paths, persistence schema, and Studio cluster editors.
- Add health/reconcile plumbing (custom SQL overrides + built-in introspection for Snowflake/Databricks/BigQuery/Redshift) and publish backend running counts in distributed mode.
- Expand documentation and diagrams (Mermaid support) describing variants, distributed coordination, and observability semantics.
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/sidebars.ts | Adds the new architecture doc to the sidebar navigation. |
| website/package.json | Adds Mermaid theme dependency for Docusaurus docs. |
| website/docusaurus.config.ts | Enables Mermaid rendering in Docusaurus. |
| website/docs/studio.md | Documents Studio cluster CRUD, variants, and health/reconcile fields. |
| website/docs/intro.md | Links intro routing section to new cluster variants/health doc. |
| website/docs/configuration.md | Documents distributed mode + variants configuration and semantics. |
| website/docs/architecture/system-map.md | Updates architecture flow and adds Mermaid diagrams + new sections. |
| website/docs/architecture/routing-and-clusters.md | Documents health + reconcile behavior and cluster variants interaction. |
| website/docs/architecture/overview.md | Adds new architecture doc entry for variants/health/reconcile. |
| website/docs/architecture/observability.md | Clarifies meaning of running gauges in distributed mode. |
| website/docs/architecture/cluster-variants-and-health.md | New in-depth architecture page for variants + health/reconcile + distributed coordination. |
| website/docs/architecture/adding-support/backend.md | Adds guidance for implementing SaaS ADBC introspection. |
| queryflux-studio/README.md | Documents new Studio modules for variants and probe fields. |
| queryflux-studio/lib/cluster-persist-form.ts | Extends upsert builders to include variants and health/reconcile queries. |
| queryflux-studio/lib/api-types.ts | Adds ClusterVariant + wires variants into cluster API types. |
| queryflux-studio/lib/adbc-saas-variants.ts | New helper module for SaaS variant rows ↔ persisted variants + validation. |
| queryflux-studio/lib/adbc-probe-defaults.ts | New defaults/placeholders for health/reconcile query fields in Studio. |
| queryflux-studio/components/cluster-config/index.ts | Exports new ADBC variants editor + probe fields components. |
| queryflux-studio/components/cluster-config/adbc-saas-variants-editor.tsx | New UI component for SaaS warehouse/project variants editing. |
| queryflux-studio/components/cluster-config/adbc-health-reconcile-fields.tsx | New UI component for health/reconcile query overrides. |
| queryflux-studio/components/add-cluster-dialog.tsx | Adds variants + health/reconcile inputs to the add-cluster flow. |
| queryflux-studio/app/clusters/clusters-grid.tsx | Adds variants + health/reconcile editing to the clusters grid editor. |
| crates/queryflux/src/main.rs | Implements variant expansion, custom probe maps, and distributed reconcile publication. |
| crates/queryflux-persistence/src/postgres/mod.rs | Adds variants to cluster upserts and adjusts capacity/reconcile persistence APIs. |
| crates/queryflux-persistence/src/postgres/migrations/20260704000001_cluster_variants.sql | Adds variants JSONB column to persisted cluster configs. |
| crates/queryflux-persistence/src/lib.rs | Extends CapacityStore trait for published running counts vs lease counts. |
| crates/queryflux-persistence/src/in_memory.rs | Implements new CapacityStore methods for in-memory persistence. |
| crates/queryflux-persistence/src/cluster_config.rs | Adds variants support to persisted cluster structs and upsert conversion. |
| crates/queryflux-frontend/src/state.rs | Extends LiveConfig with custom health/reconcile query maps. |
| crates/queryflux-engine-adapters/src/lib.rs | Adds adapter entrypoints for executing custom health/reconcile SQL. |
| crates/queryflux-engine-adapters/src/adbc/test_fixtures.rs | Adds Arrow fixtures for introspection tests. |
| crates/queryflux-engine-adapters/src/adbc/sql_helpers.rs | Adds shared ADBC SQL helpers (escaping, batch parsing). |
| crates/queryflux-engine-adapters/src/adbc/snowflake.rs | Adds Snowflake introspection via SHOW WAREHOUSES parsing. |
| crates/queryflux-engine-adapters/src/adbc/redshift.rs | Adds Redshift introspection via stv_recents. |
| crates/queryflux-engine-adapters/src/adbc/mod.rs | Wires driver-specific introspection + exposes helpers for internal modules. |
| crates/queryflux-engine-adapters/src/adbc/introspection.rs | Adds AdbcIntrospection trait for SaaS-friendly probing. |
| crates/queryflux-engine-adapters/src/adbc/databricks.rs | Adds Databricks REST introspection for health/reconcile. |
| crates/queryflux-engine-adapters/src/adbc/bigquery.rs | Adds BigQuery reconcile via INFORMATION_SCHEMA metadata queries. |
| crates/queryflux-e2e-tests/src/harness.rs | Updates test harness LiveConfig construction for new fields. |
| crates/queryflux-core/src/config.rs | Implements variant expansion + default probe query logic + tests. |
| crates/queryflux-core/src/config_json.rs | Plumbs persisted health/reconcile query keys into cluster config parsing. |
Files not reviewed (1)
- website/package-lock.json: Generated file
Comments suppressed due to low confidence (1)
crates/queryflux-persistence/src/postgres/mod.rs:1706
try_acquireis not concurrency-safe: theCOUNT(*) FROM cluster_capacity_leasescheck is not locked/serialized, so two concurrent transactions can both observe the count belowmax_rqand both insert a lease, exceeding the intended capacity limit. This breaks fleet-wide admission correctness under contention.
// Admission in a single statement: grant a lease iff the current lease
// count is below the limit. Leases live in cluster_capacity_leases;
// cluster_capacity_counters.running is updated only by reconcile sweeps.
let result = sqlx::query_scalar::<_, bool>(
r#"
WITH up AS (
SELECT 1
WHERE (SELECT COUNT(*) FROM cluster_capacity_leases WHERE cluster_name = $1) < $4
),
ins AS (
INSERT INTO cluster_capacity_leases (query_id, cluster_name, instance_id)
SELECT $3, $1, $2
WHERE EXISTS (SELECT 1 FROM up)
ON CONFLICT (query_id) DO NOTHING
RETURNING 1
)
SELECT EXISTS (SELECT 1 FROM ins)
"#,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function healthCheckQueryPlaceholder(driver: string, saasDriver: boolean): string { | ||
| return ( | ||
| defaultHealthCheckQuery(driver) ?? | ||
| (saasDriver | ||
| ? driver === "databricks" | ||
| ? "Built-in REST health check (leave empty)" | ||
| : "Leave empty for built-in driver introspection" | ||
| : "Leave empty to skip health checks") | ||
| ); | ||
| } |
| function syncSaasVariants(rows: typeof variantRows) { | ||
| setVariantRows(rows); | ||
| const errors = validateVariantRows(rows, driver); | ||
| setVariantFieldErrors(errors); | ||
| persisted.variants = rowsToVariants(rows, driver); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (saasDriver) return; | ||
| if (variantsJson.trim()) { | ||
| try { | ||
| const parsed = JSON.parse(variantsJson); | ||
| if (!Array.isArray(parsed)) { | ||
| setVariantsError("Variants must be a JSON array"); | ||
| return; | ||
| } | ||
| persisted.variants = parsed; | ||
| setVariantsError(null); | ||
| } catch { | ||
| setVariantsError("Invalid JSON"); | ||
| } | ||
| } else { | ||
| persisted.variants = []; | ||
| setVariantsError(null); | ||
| } | ||
| }, [variantsJson, persisted, saasDriver]); | ||
|
|
||
| useEffect(() => { | ||
| const cfg = persisted.config as Record<string, unknown>; | ||
| if (healthCheckQuery.trim()) cfg.healthCheckQuery = healthCheckQuery.trim(); | ||
| else delete cfg.healthCheckQuery; | ||
| }, [healthCheckQuery, persisted.config]); | ||
|
|
||
| useEffect(() => { | ||
| const cfg = persisted.config as Record<string, unknown>; | ||
| if (reconcileQuery.trim()) cfg.reconcileQuery = reconcileQuery.trim(); | ||
| else delete cfg.reconcileQuery; | ||
| }, [reconcileQuery, persisted.config]); |
| // Expand variants: insert a ClusterConfig for each expanded name. | ||
| let variants: Vec<queryflux_core::config::ClusterVariant> = | ||
| serde_json::from_value(r.variants.clone()).unwrap_or_default(); | ||
| if variants.is_empty() { | ||
| let mut base_cfg = base_cfg; | ||
| queryflux_core::config::apply_default_probe_queries( | ||
| &mut base_cfg, | ||
| &r.engine_key, | ||
| &r.config, | ||
| ); | ||
| clusters.insert(r.name.clone(), base_cfg); | ||
| } else if let Ok(expanded) = queryflux_core::config::expand_cluster_variants( | ||
| &r.name, | ||
| &r.config, | ||
| &r.engine_key, | ||
| &variants, | ||
| base_cfg.health_check_query.as_deref(), | ||
| base_cfg.reconcile_query.as_deref(), | ||
| ) { | ||
| for exp in expanded { | ||
| let mut variant_cfg = base_cfg.clone(); | ||
| variant_cfg.max_running_queries = exp.max_running_queries.or(max_running); | ||
| variant_cfg.health_check_query = exp.health_check_query; | ||
| variant_cfg.reconcile_query = exp.reconcile_query; | ||
| clusters.insert(exp.expanded_name, variant_cfg); | ||
| } | ||
| } |
| "@cmfcmf/docusaurus-search-local": "^2.0.1", | ||
| "@docusaurus/core": "3.9.2", | ||
| "@docusaurus/preset-classic": "3.9.2", | ||
| "@docusaurus/theme-mermaid": "^3.9.2", | ||
| "@mdx-js/react": "^3.0.0", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/queryflux-persistence/src/postgres/mod.rs (1)
1679-1717: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftRace:
try_acquirecan over-admit under concurrent calls. TheCOUNT(*) < limitgate isn’t serialized, so two transactions can both see spare capacity and both insert a lease, exceedingmax_running_queries. Serialize admission per cluster here, or switch back to a row-locked counter update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/queryflux-persistence/src/postgres/mod.rs` around lines 1679 - 1717, `PostgresStore::try_acquire` can over-admit because the `COUNT(*) < max_rq` check and insert are not serialized under concurrency. Fix the admission path by adding per-cluster serialization (for example, locking a cluster-specific row before checking/inserting) or by restoring the row-locked counter update approach so only one caller can admit at a time. Keep the fix within `try_acquire` and the `cluster_capacity_leases` admission flow.website/docs/architecture/system-map.md (1)
282-288: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix table column-count mismatch (MD056).
The header declares 3 columns (
Metric | Type | Labels) but thequeryflux_running_queriesandqueryflux_queued_queriesrows add a 4th cell; per the linter, the extra description text will be dropped/misrendered.📝 Proposed fix
-| `queryflux_running_queries` | Gauge | `cluster_group`, `cluster_name` | Running queries per cluster; in distributed mode reflects reconcile-published engine ground truth | -| `queryflux_queued_queries` | Gauge | `cluster_group` | Queries waiting for a free cluster slot | +| `queryflux_running_queries` | Gauge | `cluster_group`, `cluster_name` | +| `queryflux_queued_queries` | Gauge | `cluster_group` | + +`queryflux_running_queries` reflects reconcile-published engine ground truth in distributed mode. `queryflux_queued_queries` counts queries waiting for a free cluster slot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/docs/architecture/system-map.md` around lines 282 - 288, The markdown table has a column-count mismatch because the Metric/Type/Labels header only defines 3 columns, while the `queryflux_running_queries` and `queryflux_queued_queries` rows include extra description text as a 4th cell. Update the table in `system-map.md` so every row matches the 3-column structure, either by moving the descriptive text out of the table or by redesigning the table consistently; use the `queryflux_running_queries` and `queryflux_queued_queries` entries as the spots to fix.Source: Linters/SAST tools
🧹 Nitpick comments (8)
crates/queryflux-persistence/src/in_memory.rs (1)
842-858: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd matching in-memory test coverage for
publish_running_count/active_count.Postgres persistence gained
pg_capacity_publish_running_countcovering the publish→read round trip, but no equivalent test was added here forInMemoryPersistence.♻️ Suggested test
#[tokio::test] async fn capacity_publish_running_count_round_trip() { let store = InMemoryPersistence::new(); assert_eq!(store.active_count("cluster1").await.unwrap(), 0); store.publish_running_count("cluster1", 42).await.unwrap(); assert_eq!(store.active_count("cluster1").await.unwrap(), 42); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/queryflux-persistence/src/in_memory.rs` around lines 842 - 858, Add an in-memory test that exercises the publish→read round trip for running counts, mirroring the new Postgres coverage. Create a test in InMemoryPersistence that calls active_count, then publish_running_count, then active_count again for the same cluster name and asserts the updated value is returned. Use the existing InMemoryPersistence API methods active_count and publish_running_count so the coverage stays aligned with the persistence behavior.crates/queryflux/src/main.rs (3)
2404-2430: 🚀 Performance & Scalability | 🔵 TrivialO(records × expanded_configs) scan when building per-variant
ClusterConfigentries.For each cluster record with variants, this iterates the entire
expanded_configsmap and filters by string-prefix (expanded_name.starts_with(&format!("{}::", r.name))), even thoughexpanded_to_parent(built earlier in this function) already maps each expanded name directly to its parent record. A single pass driven byexpanded_configs/expanded_to_parentwould avoid the repeated full-map scan per record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/queryflux/src/main.rs` around lines 2404 - 2430, The per-variant ClusterConfig insertion path in main.rs is doing an O(records × expanded_configs) prefix scan, which is unnecessary. Update the branch that handles non-empty variants to use the existing expanded_to_parent mapping (or a single pass over expanded_configs keyed by parent) so each expanded name is matched directly to its parent record instead of calling starts_with on every entry. Keep the logic in the cluster_configs population block and preserve the same max_running_queries, health_check_query, and reconcile_query overrides when building each variant_cfg.
2054-2061: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a named struct instead of a positional 4-tuple for
ExpandedVariantConfig.
(serde_json::Value, Option<u64>, Option<String>, Option<String>)is destructured positionally at several call sites (e.g.(_, variant_max, hcq, rq),(cfg, _, _, _)), which is error-prone to extend/reorder and hard to read at each call site.♻️ Suggested refactor
-/// Per expanded variant: merged JSON config, optional max concurrency, optional probe SQL. -type ExpandedVariantConfig = ( - serde_json::Value, - Option<u64>, - Option<String>, - Option<String>, -); +/// Per expanded variant: merged JSON config, optional max concurrency, optional probe SQL. +struct ExpandedVariantConfig { + merged_config: serde_json::Value, + max_running_queries: Option<u64>, + health_check_query: Option<String>, + reconcile_query: Option<String>, +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/queryflux/src/main.rs` around lines 2054 - 2061, Replace the positional 4-tuple alias used by ExpandedVariantConfig with a named struct carrying the merged JSON config, optional max concurrency, and the two optional SQL fields. Update the destructuring sites in the code that currently pattern-match on the tuple (for example the places using (_, variant_max, hcq, rq) and (cfg, _, _, _)) to read named fields instead, making the flow in main and related variant-handling logic easier to extend and less error-prone.
1633-1718: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHealth-check and engine-reconcile sweeps run per-cluster network calls sequentially.
Both this loop and the health-check task (line ~1604-1625) iterate
targetswith a per-cluster.awaiton the adapter (which, for ADBC SaaS backends, is a real HTTP round-trip to Databricks/Snowflake/BigQuery). With variant expansion,targetscan now contain many more entries than before (one per expanded warehouse), so a full sweep's wall-clock time grows linearly with variant count and could start exceeding the 30s tick interval for larger fleets, delaying reconciliation for later clusters in the list and holding theengine-reconcilesweep lock open longer than necessary.Consider fetching concurrently, e.g. with
futures::future::join_allbounded by a semaphore, instead of a sequentialforloop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/queryflux/src/main.rs` around lines 1633 - 1718, The health-check and engine-reconcile sweeps in the interval task are doing per-cluster adapter calls sequentially inside the `for` loops, which makes wall-clock time grow with `targets` size and can overrun the 30s cadence. Update the loop in `main` for the health-check task and the `engine-reconcile` `tokio::spawn` block to fetch cluster counts concurrently rather than awaiting each `fetch_engine_running_count`/backend read one by one, using a bounded concurrency approach such as `join_all` plus a semaphore. Keep the existing reconcile/publish logic in `apply_reconcile_to_cluster_state`, `publish_running_count`, and `active_count`, but drive it from concurrently collected per-cluster results.queryflux-studio/components/add-cluster-dialog.tsx (1)
222-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the duplicated
adbcDriverderivation.
selected.adbcDriver ?? flat.driver ?? ""(Line 222) andselected?.adbcDriver ?? flat.driver ?? ""(Line 264) compute the same value in two places. Hoisting a singleadbcDriver(e.g. viauseMemo, using optional chaining so it's safe to reuse insidehandleSavetoo) removes the duplication and avoids future drift if the fallback logic changes.Also applies to: 264-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@queryflux-studio/components/add-cluster-dialog.tsx` at line 222, The `adbcDriver` value is derived twice in `AddClusterDialog`, once in the render path and again inside `handleSave`, which can drift over time. Hoist the shared `selected?.adbcDriver ?? flat.driver ?? ""` logic into a single reusable value (for example with `useMemo`) and use that one symbol everywhere the driver is needed, including the save handler, so the fallback behavior stays consistent.queryflux-studio/components/cluster-config/adbc-saas-variants-editor.tsx (1)
61-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider inline, per-row error display.
Validation errors from
validateVariantRowsare rendered as one flat list at the bottom (Lines 126-132), disconnected from the specific row/field that failed (name format, duplicate name, missing sub-resource, invalid max). With several rows this makes it hard to tell which row needs fixing.Consider highlighting the offending input (e.g. red border) and/or showing the message directly under the relevant row, which would require
validateVariantRowsto return row-scoped errors (e.g. keyed byrow.id) instead of a flatstring[].Also applies to: 126-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@queryflux-studio/components/cluster-config/adbc-saas-variants-editor.tsx` around lines 61 - 66, The current validation flow in adbc-saas-variants-editor.tsx surfaces all `validateVariantRows` failures as one flat list, which makes it hard to map errors back to the affected row or field. Update `validateVariantRows` to return row-scoped errors keyed by something stable like `row.id`, then use those in the row rendering path in the variant editor to highlight the offending inputs and/or show the specific message directly under the matching row instead of only rendering a bottom summary.crates/queryflux-engine-adapters/src/adbc/mod.rs (1)
619-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared spawn_blocking/query helpers.
execute_custom_health_check/health_checkandexecute_custom_reconcile_query/the Trino/StarRocks/ClickHouse branches offetch_running_query_countall repeat the identicalpool.get()→new_statement()→set_sql_query()→execute()→collect_batches()pipeline, varying only by SQL string and cell-extraction. Centralizing this into one or two private helpers would reduce ~80 lines of near-duplicate code and make future engine additions less error-prone.♻️ Sketch of a shared helper
async fn run_count_query(pool: AdbcPool, sql: String) -> Option<u64> { tokio::task::spawn_blocking(move || { let mut conn = pool.get().ok()?; let mut stmt = conn.new_statement().ok()?; stmt.set_sql_query(&sql).ok()?; let batches = collect_batches(stmt.execute().ok()?).ok()?; batches.iter().find_map(|b| { sql_helpers::cell_u64(b, "running", 0).or_else(|| batch_first_cell_as_u64(b)) }) }) .await .ok()? }Then
execute_custom_reconcile_query, and each engine's branch infetch_running_query_count, become one-liners calling this helper with the appropriate SQL.Also applies to: 693-726
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/queryflux-engine-adapters/src/adbc/mod.rs` around lines 619 - 673, The Trino, StarRocks, and ClickHouse branches in fetch_running_query_count repeat the same pool.get() → new_statement() → set_sql_query() → execute() → collect_batches() flow, so extract that shared logic into a private helper used by fetch_running_query_count and the related custom health/reconcile query paths. Keep engine-specific differences limited to the SQL string and cell extraction, and reuse existing symbols like fetch_running_query_count, execute_custom_health_check, execute_custom_reconcile_query, and batch_first_cell_as_u64 to simplify future engine additions.crates/queryflux-engine-adapters/src/lib.rs (1)
347-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent no-op when custom health/reconcile SQL is configured for Async adapters.
If an operator sets
healthCheckQuery/reconcileQueryon a Trino/Athena (Async) cluster via the new Studio UI, it is silently dropped here with no warning — the custom SQL is simply never executed. Consider logging once (e.g. at config-load/adapter-build time, or on first call here) so operators can tell the configured query isn't taking effect, rather than debugging silently-stale reconcile counts.🔔 Proposed fix: warn when custom SQL is ignored
pub async fn execute_custom_health_check(&self, sql: &str) -> bool { match self { Self::Sync(a) => a.execute_custom_health_check(sql).await, Self::Async(a) => { - // Async adapters (Trino/Athena) don't have a simple SQL execution path - // for custom health checks. Fall back to the default health_check(). - let _ = sql; + // Async adapters (Trino/Athena) don't have a simple SQL execution path + // for custom health checks. Fall back to the default health_check(). + tracing::warn!("custom health check SQL is not supported for this adapter kind; falling back to default health_check()"); a.health_check().await } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/queryflux-engine-adapters/src/lib.rs` around lines 347 - 368, The Async adapter paths in execute_custom_health_check and execute_custom_reconcile_query currently ignore configured custom SQL for Trino/Athena without any indication, so add a warning when these options are present but cannot be executed. Update the Adapter/Async branch handling to log once either when the adapter is built or on the first call to these methods, making it clear that healthCheckQuery/reconcileQuery will be ignored for Async adapters and that health_check() or None is being used instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/queryflux-core/src/config_json.rs`:
- Around line 149-151: `UpsertClusterConfig::from_core` is not carrying probe
SQL through to persistence, so `health_check_query` and `reconcile_query` are
being dropped before the cluster JSONB write. Update the conversion logic in
`from_core` to populate the persisted JSON fields that
`cluster_config_from_persisted_json` already reads (`healthCheckQuery` and
`reconcileQuery`), using the corresponding values from the core config so
YAML-seeded clusters keep their custom probe queries across Postgres
round-trips.
In `@crates/queryflux-core/src/config.rs`:
- Around line 1188-1206: The ADBC variant merge in the config expansion logic
can panic when `merged` is not a JSON object because
`merged.as_object_mut().unwrap()` is called unconditionally after `deep_merge`.
Update the variant-building path in `config.rs` to handle non-object `config`
values safely by checking whether `merged` is an object before injecting
`dbKwargs`, and if it is not, either return an error or skip the ADBC-specific
`dbKwargs` mapping; keep the fix localized to the merge block that uses
`variant.overrides`, `deep_merge`, and `variant_field_to_db_kwarg`.
In `@crates/queryflux-engine-adapters/src/adbc/bigquery.rs`:
- Around line 66-69: The BigQuery adapter health_check implementation is a stub
that always returns true, so it never validates connectivity or auth; update
BigQueryAdapter::health_check to perform a cheap metadata-only probe instead of
unconditionally succeeding. Reuse the existing BigQuery client path and a
low-cost metadata query pattern (same class used for reconcile) so the method
can detect unreachable projects, auth failures, or network issues while keeping
the “no warehouse wake” behavior.
- Around line 15-24: The project ID fallback in try_from_adbc_config is parsing
the URI incorrectly by using split('/') on the full uri, which can capture the
scheme instead of the BigQuery project ID. Update the URI fallback logic in
try_from_adbc_config to parse the path segment from the BigQuery ADBC URI format
(bigquery://.../<project_id>, possibly with an optional host) and use that value
only if db_kwarg lookups for project_id/project are missing.
In `@crates/queryflux-engine-adapters/src/adbc/redshift.rs`:
- Around line 30-32: The Redshift adapter’s health_check currently always
returns true without probing the database, so replace the stub in Redshift with
a real liveness check that issues a cheap query against stv_recents and returns
false on failure. Use the existing health_check method on the Redshift adapter
to run the probe through the current connection/client path, and surface any
query or connection error by mapping it to a false result.
In `@crates/queryflux/src/main.rs`:
- Around line 279-314: Variant expansion errors are being dropped in the cluster
config building path, so failed calls to expand_cluster_variants can make
records disappear without any visibility. Update the relevant
cluster-construction branches in main.rs to match the sibling adapter/reload
paths by handling the Err case explicitly, logging a tracing::error! with the
cluster name and error details before continuing, and keep the fallback behavior
consistent with the surrounding config assembly logic. Apply the same fix at
both expand_cluster_variants call sites so missing clusters can be traced back
to the failing expansion.
- Around line 2310-2337: The `max_q` handling in the expanded-variant branch is
swallowing validation failures from `max_running_queries_u64_from_db` by falling
back silently, unlike the non-variant branch that propagates errors with `?`.
Update the `max_q` logic in `main.rs` so the expanded-config path uses the same
failure behavior as the adjacent branch (or at least emits a warning before
falling back), keeping `ClusterState` rebuild behavior consistent and avoiding
masked misconfiguration for variant members.
In `@queryflux-studio/app/clusters/clusters-grid.tsx`:
- Around line 1298-1304: VariantsSection is reading only
persisted.config.driver, while save() resolves the effective driver from
editFlat.driver ?? persisted.config.driver, so the rendered/validated variants
can get out of sync with the value being saved. Update VariantsSection to use
the same resolved driver logic as save() (or pass the resolved driver down from
the parent) and use that value consistently when computing saasDriver,
variantsToRows, and any subResourceFieldSpec-based validation/labels.
- Around line 1289-1356: The variant and query inputs are mutating the shared
`persisted` object in place, so `cancelEdit()` cannot discard changes and
invalid JSON is not enforced at save time. Update `VariantsSection` to stop
writing directly to `persisted.variants` and `persisted.config` inside
`syncSaasVariants` and the three `useEffect`s; instead route changes through
`setPersisted` or new callbacks from `ClusterDialog` so edits produce a fresh
object. Also make `save()` reject when `variantsError` is set, and ensure
`cancelEdit()` restores the last clean snapshot rather than relying on the
mutated state.
In `@website/docs/architecture/cluster-variants-and-health.md`:
- Around line 155-158: The fenced code blocks in the architecture docs are
missing a language identifier, triggering MD040. Update the affected markdown
fences in the cluster variants and health section to include an explicit
language tag such as text, using the existing fenced blocks that describe the
healthCheckQuery and cluster capacity flows. Keep the content unchanged and
ensure all similar fences in this doc use the same language-tagged style.
---
Outside diff comments:
In `@crates/queryflux-persistence/src/postgres/mod.rs`:
- Around line 1679-1717: `PostgresStore::try_acquire` can over-admit because the
`COUNT(*) < max_rq` check and insert are not serialized under concurrency. Fix
the admission path by adding per-cluster serialization (for example, locking a
cluster-specific row before checking/inserting) or by restoring the row-locked
counter update approach so only one caller can admit at a time. Keep the fix
within `try_acquire` and the `cluster_capacity_leases` admission flow.
In `@website/docs/architecture/system-map.md`:
- Around line 282-288: The markdown table has a column-count mismatch because
the Metric/Type/Labels header only defines 3 columns, while the
`queryflux_running_queries` and `queryflux_queued_queries` rows include extra
description text as a 4th cell. Update the table in `system-map.md` so every row
matches the 3-column structure, either by moving the descriptive text out of the
table or by redesigning the table consistently; use the
`queryflux_running_queries` and `queryflux_queued_queries` entries as the spots
to fix.
---
Nitpick comments:
In `@crates/queryflux-engine-adapters/src/adbc/mod.rs`:
- Around line 619-673: The Trino, StarRocks, and ClickHouse branches in
fetch_running_query_count repeat the same pool.get() → new_statement() →
set_sql_query() → execute() → collect_batches() flow, so extract that shared
logic into a private helper used by fetch_running_query_count and the related
custom health/reconcile query paths. Keep engine-specific differences limited to
the SQL string and cell extraction, and reuse existing symbols like
fetch_running_query_count, execute_custom_health_check,
execute_custom_reconcile_query, and batch_first_cell_as_u64 to simplify future
engine additions.
In `@crates/queryflux-engine-adapters/src/lib.rs`:
- Around line 347-368: The Async adapter paths in execute_custom_health_check
and execute_custom_reconcile_query currently ignore configured custom SQL for
Trino/Athena without any indication, so add a warning when these options are
present but cannot be executed. Update the Adapter/Async branch handling to log
once either when the adapter is built or on the first call to these methods,
making it clear that healthCheckQuery/reconcileQuery will be ignored for Async
adapters and that health_check() or None is being used instead.
In `@crates/queryflux-persistence/src/in_memory.rs`:
- Around line 842-858: Add an in-memory test that exercises the publish→read
round trip for running counts, mirroring the new Postgres coverage. Create a
test in InMemoryPersistence that calls active_count, then publish_running_count,
then active_count again for the same cluster name and asserts the updated value
is returned. Use the existing InMemoryPersistence API methods active_count and
publish_running_count so the coverage stays aligned with the persistence
behavior.
In `@crates/queryflux/src/main.rs`:
- Around line 2404-2430: The per-variant ClusterConfig insertion path in main.rs
is doing an O(records × expanded_configs) prefix scan, which is unnecessary.
Update the branch that handles non-empty variants to use the existing
expanded_to_parent mapping (or a single pass over expanded_configs keyed by
parent) so each expanded name is matched directly to its parent record instead
of calling starts_with on every entry. Keep the logic in the cluster_configs
population block and preserve the same max_running_queries, health_check_query,
and reconcile_query overrides when building each variant_cfg.
- Around line 2054-2061: Replace the positional 4-tuple alias used by
ExpandedVariantConfig with a named struct carrying the merged JSON config,
optional max concurrency, and the two optional SQL fields. Update the
destructuring sites in the code that currently pattern-match on the tuple (for
example the places using (_, variant_max, hcq, rq) and (cfg, _, _, _)) to read
named fields instead, making the flow in main and related variant-handling logic
easier to extend and less error-prone.
- Around line 1633-1718: The health-check and engine-reconcile sweeps in the
interval task are doing per-cluster adapter calls sequentially inside the `for`
loops, which makes wall-clock time grow with `targets` size and can overrun the
30s cadence. Update the loop in `main` for the health-check task and the
`engine-reconcile` `tokio::spawn` block to fetch cluster counts concurrently
rather than awaiting each `fetch_engine_running_count`/backend read one by one,
using a bounded concurrency approach such as `join_all` plus a semaphore. Keep
the existing reconcile/publish logic in `apply_reconcile_to_cluster_state`,
`publish_running_count`, and `active_count`, but drive it from concurrently
collected per-cluster results.
In `@queryflux-studio/components/add-cluster-dialog.tsx`:
- Line 222: The `adbcDriver` value is derived twice in `AddClusterDialog`, once
in the render path and again inside `handleSave`, which can drift over time.
Hoist the shared `selected?.adbcDriver ?? flat.driver ?? ""` logic into a single
reusable value (for example with `useMemo`) and use that one symbol everywhere
the driver is needed, including the save handler, so the fallback behavior stays
consistent.
In `@queryflux-studio/components/cluster-config/adbc-saas-variants-editor.tsx`:
- Around line 61-66: The current validation flow in
adbc-saas-variants-editor.tsx surfaces all `validateVariantRows` failures as one
flat list, which makes it hard to map errors back to the affected row or field.
Update `validateVariantRows` to return row-scoped errors keyed by something
stable like `row.id`, then use those in the row rendering path in the variant
editor to highlight the offending inputs and/or show the specific message
directly under the matching row instead of only rendering a bottom summary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68f9f6bd-86b1-4994-bd93-54c0b69f8de6
⛔ Files ignored due to path filters (1)
website/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
crates/queryflux-core/src/config.rscrates/queryflux-core/src/config_json.rscrates/queryflux-e2e-tests/src/harness.rscrates/queryflux-engine-adapters/src/adbc/bigquery.rscrates/queryflux-engine-adapters/src/adbc/databricks.rscrates/queryflux-engine-adapters/src/adbc/introspection.rscrates/queryflux-engine-adapters/src/adbc/mod.rscrates/queryflux-engine-adapters/src/adbc/redshift.rscrates/queryflux-engine-adapters/src/adbc/snowflake.rscrates/queryflux-engine-adapters/src/adbc/sql_helpers.rscrates/queryflux-engine-adapters/src/adbc/test_fixtures.rscrates/queryflux-engine-adapters/src/lib.rscrates/queryflux-frontend/src/state.rscrates/queryflux-persistence/src/cluster_config.rscrates/queryflux-persistence/src/in_memory.rscrates/queryflux-persistence/src/lib.rscrates/queryflux-persistence/src/postgres/migrations/20260704000001_cluster_variants.sqlcrates/queryflux-persistence/src/postgres/mod.rscrates/queryflux/src/main.rsqueryflux-studio/README.mdqueryflux-studio/app/clusters/clusters-grid.tsxqueryflux-studio/components/add-cluster-dialog.tsxqueryflux-studio/components/cluster-config/adbc-health-reconcile-fields.tsxqueryflux-studio/components/cluster-config/adbc-saas-variants-editor.tsxqueryflux-studio/components/cluster-config/index.tsqueryflux-studio/lib/adbc-probe-defaults.tsqueryflux-studio/lib/adbc-saas-variants.tsqueryflux-studio/lib/api-types.tsqueryflux-studio/lib/cluster-persist-form.tswebsite/docs/architecture/adding-support/backend.mdwebsite/docs/architecture/cluster-variants-and-health.mdwebsite/docs/architecture/observability.mdwebsite/docs/architecture/overview.mdwebsite/docs/architecture/routing-and-clusters.mdwebsite/docs/architecture/system-map.mdwebsite/docs/configuration.mdwebsite/docs/intro.mdwebsite/docs/studio.mdwebsite/docusaurus.config.tswebsite/package.jsonwebsite/sidebars.ts
| variants: Vec::new(), | ||
| health_check_query: json_str(config, "healthCheckQuery"), | ||
| reconcile_query: json_str(config, "reconcileQuery"), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'pub fn from_core($$$) -> $RET { $$$ }' --lang rust crates/queryflux-persistence/src/cluster_config.rs
rg -n 'healthCheckQuery|reconcileQuery' crates/queryflux-persistence/src/cluster_config.rsRepository: lakeops-org/queryflux
Length of output: 11758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== cluster_config.rs (relevant slices) ==\n'
sed -n '1,260p' crates/queryflux-persistence/src/cluster_config.rs
printf '\n== config_json.rs (relevant slices) ==\n'
sed -n '1,240p' crates/queryflux-core/src/config_json.rs
printf '\n== search for healthCheckQuery/reconcileQuery across repo ==\n'
rg -n 'healthCheckQuery|reconcileQuery|health_check_query|reconcile_query' cratesRepository: lakeops-org/queryflux
Length of output: 27024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
p = Path('crates/queryflux-persistence/src/cluster_config.rs')
text = p.read_text()
for needle in ['healthCheckQuery', 'reconcileQuery', 'queryAuth', 'variants']:
print(f'-- {needle} --')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start=max(1,i-3); end=min(len(text.splitlines()), i+3)
for j in range(start, end+1):
print(f'{j}:{text.splitlines()[j-1]}')
print()
PYRepository: lakeops-org/queryflux
Length of output: 4166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== call sites of UpsertClusterConfig::from_core ==\n'
rg -n 'UpsertClusterConfig::from_core|from_core\(&cfg\)|from_core\(cfg\)' crates
printf '\n== cluster persistence context ==\n'
sed -n '1,260p' crates/queryflux-persistence/src/lib.rs
printf '\n== cluster config core fields / defaults ==\n'
sed -n '880,1125p' crates/queryflux-core/src/config.rs
printf '\n== seed / persisted load path mentioning cluster_configs.config ==\n'
rg -n 'cluster_configs\.config|cluster_configs|cluster_config_from_persisted_json|try_from_config_json|persisted_json' crates/queryflux* -A3 -B3Repository: lakeops-org/queryflux
Length of output: 50377
Persist probe queries in the cluster JSONB.
UpsertClusterConfig::from_core drops health_check_query and reconcile_query, even though cluster_config_from_persisted_json reads healthCheckQuery and reconcileQuery back out. YAML-seeded clusters with custom probe SQL will lose that config on the first Postgres write and reload with defaults.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/queryflux-core/src/config_json.rs` around lines 149 - 151,
`UpsertClusterConfig::from_core` is not carrying probe SQL through to
persistence, so `health_check_query` and `reconcile_query` are being dropped
before the cluster JSONB write. Update the conversion logic in `from_core` to
populate the persisted JSON fields that `cluster_config_from_persisted_json`
already reads (`healthCheckQuery` and `reconcileQuery`), using the corresponding
values from the core config so YAML-seeded clusters keep their custom probe
queries across Postgres round-trips.
| let mut merged = base_config.clone(); | ||
| if variant.overrides.is_object() { | ||
| deep_merge(&mut merged, &variant.overrides); | ||
|
|
||
| if is_adbc { | ||
| let db_kwargs = merged | ||
| .as_object_mut() | ||
| .unwrap() | ||
| .entry("dbKwargs") | ||
| .or_insert_with(|| serde_json::Value::Object(serde_json::Map::new())); | ||
| if let Some(kwargs_obj) = db_kwargs.as_object_mut() { | ||
| for (key, val) in variant.overrides.as_object().unwrap() { | ||
| if let Some(db_key) = variant_field_to_db_kwarg(driver, key) { | ||
| kwargs_obj.insert(db_key.to_string(), val.clone()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## config.rs around expand_cluster_variants\n'
sed -n '1140,1225p' crates/queryflux-core/src/config.rs
printf '\n## locate base_config and build_live_config callers\n'
rg -n "base_config|build_live_config|expand_cluster_variants|cluster_config_from_persisted_json|UpsertClusterConfig::from_core" crates/queryflux-core/src -S
printf '\n## relevant persisted-config conversion code\n'
sed -n '1,260p' crates/queryflux-core/src/config_json.rs
sed -n '1,260p' crates/queryflux-core/src/cluster_config.rsRepository: lakeops-org/queryflux
Length of output: 14661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## call sites and surrounding context in config.rs\n'
sed -n '1920,2315p' crates/queryflux-core/src/config.rs
printf '\n## cluster config JSON parsing / persistence helpers\n'
sed -n '1,260p' crates/queryflux-core/src/config_json.rs
printf '\n## search for JSON object assumptions around config handling\n'
rg -n "as_object_mut\(\)\.unwrap\(\)|as_object\(\)|Value::Object|serde_json::json!\(\{" crates/queryflux-core/src -S
printf '\n## find build_live_config and persisted config readers\n'
rg -n "build_live_config|cluster_configs\.config|persisted_json|from_persisted_json|expand_cluster_variants" crates/queryflux-core/src -SRepository: lakeops-org/queryflux
Length of output: 30704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## search entire repo for expand_cluster_variants call sites\n'
rg -n "expand_cluster_variants\(" . -S
printf '\n## inspect engine registry / config build path\n'
sed -n '1,240p' crates/queryflux-core/src/engine_registry.rs
sed -n '1,260p' crates/queryflux-core/src/config.rs
printf '\n## inspect ClusterConfig definition and serde behavior\n'
rg -n "pub struct ClusterConfig|#[[:space:]]*derive\\(|impl .*ClusterConfig" crates/queryflux-core/src/config.rs -n -S
sed -n '820,980p' crates/queryflux-core/src/config.rsRepository: lakeops-org/queryflux
Length of output: 28920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## main.rs call sites for expand_cluster_variants\n'
sed -n '260,340p' crates/queryflux/src/main.rs
sed -n '430,500p' crates/queryflux/src/main.rs
sed -n '850,920p' crates/queryflux/src/main.rs
sed -n '2100,2155p' crates/queryflux/src/main.rs
printf '\n## any direct parsing into serde_json::Value for cluster configs\n'
rg -n "serde_json::Value|from_value::<serde_json::Value>|to_value\\(|config_json|variants" crates/queryflux/src/main.rs crates/queryflux-core/src -SRepository: lakeops-org/queryflux
Length of output: 27826
Guard the ADBC variant merge against non-object configs. merged.as_object_mut().unwrap() will panic if a persisted config value isn’t an object. Since the reload/build path passes r.config through directly, one bad record can abort variant expansion; return an error or skip the dbKwargs injection instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/queryflux-core/src/config.rs` around lines 1188 - 1206, The ADBC
variant merge in the config expansion logic can panic when `merged` is not a
JSON object because `merged.as_object_mut().unwrap()` is called unconditionally
after `deep_merge`. Update the variant-building path in `config.rs` to handle
non-object `config` values safely by checking whether `merged` is an object
before injecting `dbKwargs`, and if it is not, either return an error or skip
the ADBC-specific `dbKwargs` mapping; keep the fix localized to the merge block
that uses `variant.overrides`, `deep_merge`, and `variant_field_to_db_kwarg`.
| pub fn try_from_adbc_config( | ||
| cluster_name: &ClusterName, | ||
| uri: &str, | ||
| db_kwargs: &[(String, String)], | ||
| pool: AdbcPool, | ||
| ) -> Option<BigQueryIntrospection> { | ||
| let project_id = sql_helpers::db_kwarg(db_kwargs, "project_id") | ||
| .or_else(|| sql_helpers::db_kwarg(db_kwargs, "project")) | ||
| .or_else(|| uri.split('/').next().map(str::to_string)) | ||
| .filter(|p| !p.is_empty())?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for real BigQuery ADBC URI examples/conventions used elsewhere in the repo (docs, tests, config samples).
rg -n -i "bigquery" --type=rust -C3 -g '!target' | rg -i "uri|://"
rg -n -i "bigquery" website/docs -C3Repository: lakeops-org/queryflux
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bigquery.rs =="
cat -n crates/queryflux-engine-adapters/src/adbc/bigquery.rs | sed -n '1,120p'
echo
echo "== repo search: BigQuery / ADBC / project_id / uri conventions =="
rg -n -i "bigquery|adbc|project_id|project=" crates website docs . \
-g '!target' -g '!node_modules' -g '!dist' -g '!build' \
| sed -n '1,220p'
echo
echo "== explicit URI token search =="
rg -n "://|bigquery://" . -g '!target' -g '!node_modules' -g '!dist' -g '!build' | sed -n '1,220p'Repository: lakeops-org/queryflux
Length of output: 46260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from urllib.parse import urlsplit
samples = [
"project/dataset",
"bigquery://project/dataset",
"bigquery:///project/dataset",
"https://example.com/x/y",
"scheme:project/dataset",
]
for s in samples:
print(s, "=> split('/')=", s.split('/')[0], "urlsplit=", urlsplit(s))
PYRepository: lakeops-org/queryflux
Length of output: 885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call sites of try_from_adbc_config =="
rg -n "try_from_adbc_config\(" crates -g '!target' -g '!node_modules' | sed -n '1,80p'
echo
echo "== BigQuery docs around virtual fields / built-in introspection =="
sed -n '88,110p' website/docs/architecture/cluster-variants-and-health.md
echo
sed -n '176,205p' website/docs/architecture/cluster-variants-and-health.md
echo
echo "== remainder of bigquery.rs tests =="
sed -n '109,170p' crates/queryflux-engine-adapters/src/adbc/bigquery.rsRepository: lakeops-org/queryflux
Length of output: 4544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== bigquery adapter wiring =="
rg -n "BigQueryIntrospection|build_introspection|uri:" crates/queryflux-engine-adapters/src -C3 | sed -n '1,160p'Repository: lakeops-org/queryflux
Length of output: 12852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call sites of try_from_adbc_config =="
rg -n "try_from_adbc_config\(" crates -g '!target' -g '!node_modules' | sed -n '1,80p'
echo
echo "== BigQuery docs around virtual fields / built-in introspection =="
sed -n '88,110p' website/docs/architecture/cluster-variants-and-health.md
echo
sed -n '176,205p' website/docs/architecture/cluster-variants-and-health.md
echo
echo "== remainder of bigquery.rs tests =="
sed -n '109,170p' crates/queryflux-engine-adapters/src/adbc/bigquery.rs
echo
echo "== bigquery adapter wiring =="
rg -n "BigQueryIntrospection|build_introspection|uri:" crates/queryflux-engine-adapters/src -C3 | sed -n '1,160p'Repository: lakeops-org/queryflux
Length of output: 17238
🌐 Web query:
Apache ADBC BigQuery URI format project_id dataset scheme
💡 Result:
The Apache ADBC BigQuery driver uses the following URI format [1][2]: bigquery://[Host]:[Port]/[ProjectID]?[Key]=[Value]&[Key]=[Value]... Scheme: The required scheme is bigquery:// [1][2]. Host and Port: These are optional. The host defaults to bigquery.googleapis.com, and the port defaults to 443 [1][2]. Project ID: This is a required component of the URI path [1][2]. Dataset ID: While the Project ID is part of the URI path, the Dataset ID is typically provided as a query parameter (e.g.,?DatasetId=my_dataset) [1][2]. Examples: Using Application Default Credentials: bigquery:///my-project-123 Specifying a Dataset ID and Location: bigquery://bigquery.googleapis.com/my-project-123?OAuthType=0&DatasetId=analytics&Location=US [1][2] Notes: - Reserved characters in any URI element must be URI-encoded (e.g., @ becomes %40) [1][2]. - Additional configuration options can be passed as query parameters [1][2]. - For programmatic access, the Project ID and Dataset ID can also be set via driver-specific options such as adbc.bigquery.sql.project_id and adbc.bigquery.sql.dataset_id [3].
Citations:
- 1: https://docs.adbc-drivers.org/drivers/bigquery/index.html
- 2: https://docs.adbc-drivers.org/drivers/bigquery/v1.11.2.html
- 3: https://arrow.apache.org/adbc/23/python/api/adbc_driver_bigquery.html
Parse the BigQuery project ID from the URI path, not split('/'). BigQuery ADBC URIs use bigquery://.../<project_id> (often with an optional host), so this fallback can read bigquery: instead of the project ID and build a reconcile query against the wrong project.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/queryflux-engine-adapters/src/adbc/bigquery.rs` around lines 15 - 24,
The project ID fallback in try_from_adbc_config is parsing the URI incorrectly
by using split('/') on the full uri, which can capture the scheme instead of the
BigQuery project ID. Update the URI fallback logic in try_from_adbc_config to
parse the path segment from the BigQuery ADBC URI format
(bigquery://.../<project_id>, possibly with an optional host) and use that value
only if db_kwarg lookups for project_id/project are missing.
| async fn health_check(&self) -> bool { | ||
| // Metadata queries are cheap; no warehouse to wake. | ||
| true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
health_check never actually checks anything.
Returning true unconditionally means a BigQuery cluster will always report healthy even if unreachable (auth failure, network partition, wrong project), since no query is ever attempted. The comment references cost/wake concerns, but a metadata-only query (the same class used for reconcile) would validate connectivity without those costs.
🩺 Proposed fix: run a cheap metadata probe
async fn health_check(&self) -> bool {
- // Metadata queries are cheap; no warehouse to wake.
- true
+ let pool = self.pool.clone();
+ let sql = format!("SELECT 1 FROM `{}`.INFORMATION_SCHEMA.SCHEMATA LIMIT 1", self.project_id);
+ tokio::task::spawn_blocking(move || sql_helpers::query_batches(&pool, &sql).is_some())
+ .await
+ .unwrap_or(false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn health_check(&self) -> bool { | |
| // Metadata queries are cheap; no warehouse to wake. | |
| true | |
| } | |
| async fn health_check(&self) -> bool { | |
| let pool = self.pool.clone(); | |
| let sql = format!("SELECT 1 FROM `{}`.INFORMATION_SCHEMA.SCHEMATA LIMIT 1", self.project_id); | |
| tokio::task::spawn_blocking(move || sql_helpers::query_batches(&pool, &sql).is_some()) | |
| .await | |
| .unwrap_or(false) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/queryflux-engine-adapters/src/adbc/bigquery.rs` around lines 66 - 69,
The BigQuery adapter health_check implementation is a stub that always returns
true, so it never validates connectivity or auth; update
BigQueryAdapter::health_check to perform a cheap metadata-only probe instead of
unconditionally succeeding. Reuse the existing BigQuery client path and a
low-cost metadata query pattern (same class used for reconcile) so the method
can detect unreachable projects, auth failures, or network issues while keeping
the “no warehouse wake” behavior.
| async fn health_check(&self) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
health_check never actually checks anything.
Same issue as BigQueryIntrospection::health_check — always returns true without attempting any query, so an unreachable Redshift cluster (network/auth/paused serverless endpoint) will still be reported healthy.
🩺 Proposed fix: run a cheap probe against stv_recents
async fn health_check(&self) -> bool {
- true
+ let pool = self.pool.clone();
+ tokio::task::spawn_blocking(move || sql_helpers::query_batches(&pool, "SELECT 1").is_some())
+ .await
+ .unwrap_or(false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn health_check(&self) -> bool { | |
| true | |
| } | |
| async fn health_check(&self) -> bool { | |
| let pool = self.pool.clone(); | |
| tokio::task::spawn_blocking(move || sql_helpers::query_batches(&pool, "SELECT 1").is_some()) | |
| .await | |
| .unwrap_or(false) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/queryflux-engine-adapters/src/adbc/redshift.rs` around lines 30 - 32,
The Redshift adapter’s health_check currently always returns true without
probing the database, so replace the stub in Redshift with a real liveness check
that issues a cheap query against stv_recents and returns false on failure. Use
the existing health_check method on the Redshift adapter to run the probe
through the current connection/client path, and surface any query or connection
error by mapping it to a false result.
| let base_cfg = queryflux_core::engine_registry::cluster_config_from_persisted_json( | ||
| engine.clone(), | ||
| r.enabled, | ||
| max_running, | ||
| &r.config, | ||
| auth.clone(), | ||
| query_auth.clone(), | ||
| ); | ||
|
|
||
| // Expand variants: insert a ClusterConfig for each expanded name. | ||
| let variants: Vec<queryflux_core::config::ClusterVariant> = | ||
| serde_json::from_value(r.variants.clone()).unwrap_or_default(); | ||
| if variants.is_empty() { | ||
| let mut base_cfg = base_cfg; | ||
| queryflux_core::config::apply_default_probe_queries( | ||
| &mut base_cfg, | ||
| &r.engine_key, | ||
| &r.config, | ||
| ); | ||
| clusters.insert(r.name.clone(), base_cfg); | ||
| } else if let Ok(expanded) = queryflux_core::config::expand_cluster_variants( | ||
| &r.name, | ||
| &r.config, | ||
| &r.engine_key, | ||
| &variants, | ||
| base_cfg.health_check_query.as_deref(), | ||
| base_cfg.reconcile_query.as_deref(), | ||
| ) { | ||
| for exp in expanded { | ||
| let mut variant_cfg = base_cfg.clone(); | ||
| variant_cfg.max_running_queries = exp.max_running_queries.or(max_running); | ||
| variant_cfg.health_check_query = exp.health_check_query; | ||
| variant_cfg.reconcile_query = exp.reconcile_query; | ||
| clusters.insert(exp.expanded_name, variant_cfg); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Variant-expansion failures are silently swallowed here, unlike the sibling code paths.
When expand_cluster_variants returns Err in these two spots, the else if let Ok(expanded) = ... / if let Ok(expanded) = ... patterns simply skip the cluster with no tracing::error! and no fallback insertion — the record just vanishes from config.clusters / initial_config_json. Compare this to the adapter-building pass (line ~472-476) and reload's build_live_config (line ~2152-2155), which both log tracing::error!(cluster = %record.name, error = %err, ...) on the same failure. An operator debugging a "missing cluster" or "group references unknown cluster" warning elsewhere would have no signal that variant expansion actually failed and why.
♻️ Suggested fix (apply at both sites)
- } else if let Ok(expanded) = queryflux_core::config::expand_cluster_variants(
+ } else {
+ match queryflux_core::config::expand_cluster_variants(
&r.name,
&r.config,
&r.engine_key,
&variants,
base_cfg.health_check_query.as_deref(),
base_cfg.reconcile_query.as_deref(),
- ) {
- for exp in expanded {
+ ) {
+ Ok(expanded) => for exp in expanded {
let mut variant_cfg = base_cfg.clone();
variant_cfg.max_running_queries = exp.max_running_queries.or(max_running);
variant_cfg.health_check_query = exp.health_check_query;
variant_cfg.reconcile_query = exp.reconcile_query;
clusters.insert(exp.expanded_name, variant_cfg);
+ },
+ Err(err) => tracing::error!(cluster = %r.name, error = %err, "Startup: variant expansion failed — cluster omitted"),
}
}Also applies to: 857-895
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/queryflux/src/main.rs` around lines 279 - 314, Variant expansion
errors are being dropped in the cluster config building path, so failed calls to
expand_cluster_variants can make records disappear without any visibility.
Update the relevant cluster-construction branches in main.rs to match the
sibling adapter/reload paths by handling the Err case explicitly, logging a
tracing::error! with the cluster name and error details before continuing, and
keep the fallback behavior consistent with the surrounding config assembly
logic. Apply the same fix at both expand_cluster_variants call sites so missing
clusters can be traced back to the failing expansion.
| // For expanded variants, use the variant-specific max_running_queries if set. | ||
| let max_q = | ||
| if let Some((_, variant_max, _, _)) = expanded_configs.get(member_name.as_str()) { | ||
| variant_max.unwrap_or_else(|| { | ||
| max_running_queries_u64_from_db(member_name, record.max_running_queries) | ||
| .unwrap_or(None) | ||
| .unwrap_or(group_config.max_running_queries) | ||
| }) | ||
| } else { | ||
| max_running_queries_u64_from_db(member_name, record.max_running_queries)? | ||
| .unwrap_or(group_config.max_running_queries) | ||
| }; | ||
| // For expanded variants, use merged config for endpoint resolution. | ||
| let effective_config = expanded_configs | ||
| .get(member_name.as_str()) | ||
| .map(|(cfg, _, _, _)| cfg) | ||
| .unwrap_or(&record.config); | ||
| let endpoint = json_str(effective_config, "endpoint"); | ||
| let cluster_cid = cluster_ids_by_name | ||
| .get(member_name.as_str()) | ||
| .copied() | ||
| .or_else(|| expanded_to_parent.get(member_name.as_str()).map(|r| r.id)); | ||
| let group_cid = group_ids_by_name.get(group_name.as_str()).copied(); | ||
|
|
||
| // When the JSONB + engine_key fingerprint is unchanged, rebuild `ClusterState` from | ||
| // the current record anyway (group membership, IDs, endpoint, max_q may still change) | ||
| // but copy health and queue counters from the previous generation. | ||
| let cfg_json = serde_json::to_string(&(record.engine_key.as_str(), &record.config)) | ||
| let cfg_json = serde_json::to_string(&(record.engine_key.as_str(), effective_config)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Inconsistent error handling for max_running_queries_u64_from_db between variant and non-variant branches.
The variant branch swallows the Result via .unwrap_or(None), silently falling back to group_config.max_running_queries on a validation error:
max_running_queries_u64_from_db(member_name, record.max_running_queries)
.unwrap_or(None)
.unwrap_or(group_config.max_running_queries)The adjacent non-variant branch on the very next lines propagates the same call's error with ?, which aborts the whole reload and preserves the previous known-good LiveConfig — the documented safety contract for this function (see the "keeping previous config" bail-out at line ~2566-2570). For expanded-variant clusters, an invalid max_running_queries value on the base record is masked instead of triggering that same safe reload rejection.
Recommend propagating the error consistently (or at minimum logging a warning before falling back) so variant-cluster misconfiguration doesn't silently diverge from the base-cluster failure mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/queryflux/src/main.rs` around lines 2310 - 2337, The `max_q` handling
in the expanded-variant branch is swallowing validation failures from
`max_running_queries_u64_from_db` by falling back silently, unlike the
non-variant branch that propagates errors with `?`. Update the `max_q` logic in
`main.rs` so the expanded-config path uses the same failure behavior as the
adjacent branch (or at least emits a warning before falling back), keeping
`ClusterState` rebuild behavior consistent and avoiding masked misconfiguration
for variant members.
| function VariantsSection({ | ||
| persisted, | ||
| baseClusterName, | ||
| onPatchFlat: _onPatchFlat, | ||
| }: { | ||
| persisted: ClusterConfigRecord; | ||
| baseClusterName: string; | ||
| onPatchFlat: (patch: Record<string, string>) => void; | ||
| }) { | ||
| const isAdbc = persisted.engineKey === "adbc"; | ||
| const driver = ((persisted.config as Record<string, unknown>).driver as string) ?? ""; | ||
| const saasDriver = isSaasVariantDriver(driver); | ||
|
|
||
| const [variantRows, setVariantRows] = useState(() => | ||
| saasDriver ? variantsToRows(persisted.variants, driver) : [], | ||
| ); | ||
| const [variantsJson, setVariantsJson] = useState(() => | ||
| !saasDriver && persisted.variants && (persisted.variants as unknown[]).length > 0 | ||
| ? JSON.stringify(persisted.variants, null, 2) | ||
| : "", | ||
| ); | ||
| const [healthCheckQuery, setHealthCheckQuery] = useState( | ||
| () => ((persisted.config as Record<string, unknown>).healthCheckQuery as string) ?? "", | ||
| ); | ||
| const [reconcileQuery, setReconcileQuery] = useState( | ||
| () => ((persisted.config as Record<string, unknown>).reconcileQuery as string) ?? "", | ||
| ); | ||
| const [variantsError, setVariantsError] = useState<string | null>(null); | ||
| const [variantFieldErrors, setVariantFieldErrors] = useState<string[]>([]); | ||
|
|
||
| function syncSaasVariants(rows: typeof variantRows) { | ||
| setVariantRows(rows); | ||
| const errors = validateVariantRows(rows, driver); | ||
| setVariantFieldErrors(errors); | ||
| persisted.variants = rowsToVariants(rows, driver); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (saasDriver) return; | ||
| if (variantsJson.trim()) { | ||
| try { | ||
| const parsed = JSON.parse(variantsJson); | ||
| if (!Array.isArray(parsed)) { | ||
| setVariantsError("Variants must be a JSON array"); | ||
| return; | ||
| } | ||
| persisted.variants = parsed; | ||
| setVariantsError(null); | ||
| } catch { | ||
| setVariantsError("Invalid JSON"); | ||
| } | ||
| } else { | ||
| persisted.variants = []; | ||
| setVariantsError(null); | ||
| } | ||
| }, [variantsJson, persisted, saasDriver]); | ||
|
|
||
| useEffect(() => { | ||
| const cfg = persisted.config as Record<string, unknown>; | ||
| if (healthCheckQuery.trim()) cfg.healthCheckQuery = healthCheckQuery.trim(); | ||
| else delete cfg.healthCheckQuery; | ||
| }, [healthCheckQuery, persisted.config]); | ||
|
|
||
| useEffect(() => { | ||
| const cfg = persisted.config as Record<string, unknown>; | ||
| if (reconcileQuery.trim()) cfg.reconcileQuery = reconcileQuery.trim(); | ||
| else delete cfg.reconcileQuery; | ||
| }, [reconcileQuery, persisted.config]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
State mutated in place instead of via setPersisted; Cancel doesn't revert, invalid JSON doesn't block Save.
syncSaasVariants and the three useEffects here write directly onto persisted.variants / persisted.config.healthCheckQuery / persisted.config.reconcileQuery (the same object held in ClusterDialog's useState), instead of going through setPersisted or a callback prop.
Two concrete problems:
cancelEdit()(elsewhere in this file) never resetspersisted. Since the mutation happens on every keystroke (before Save is even clicked), clicking Cancel does not actually discard variant/health/reconcile edits — they remain on the object. A later, unrelated Save (e.g. just renaming the cluster) will silently persist the "canceled" values, becausebuildClusterUpsertFromForm(lib/cluster-persist-form.tslines 290-291) falls back torecord.variants/ mergesrecord.config, i.e. this same mutated object.- For non-SaaS drivers, invalid JSON only sets a local
variantsErrorstring;save()(lines 496-509) never checks it, so a user can save while the JSON textarea shows "Invalid JSON" — the stale last-validpersisted.variantsgets silently persisted with no save-time warning.
Recommend lifting variants/health/reconcile state to ClusterDialog via callbacks (onVariantsChange, onHealthCheckQueryChange, etc.) that call setPersisted with a new object, and have cancelEdit() restore a pristine snapshot (or refetch) instead of relying on in-place mutation.
💡 Illustrative direction (not a complete fix)
function syncSaasVariants(rows: typeof variantRows) {
setVariantRows(rows);
const errors = validateVariantRows(rows, driver);
setVariantFieldErrors(errors);
- persisted.variants = rowsToVariants(rows, driver);
+ onVariantsChange(rowsToVariants(rows, driver));
}onVariantsChange (new prop) would call setPersisted(prev => prev ? { ...prev, variants: v } : prev) in the parent, and cancelEdit() would restore the last-fetched snapshot instead of leaving mutations in place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@queryflux-studio/app/clusters/clusters-grid.tsx` around lines 1289 - 1356,
The variant and query inputs are mutating the shared `persisted` object in
place, so `cancelEdit()` cannot discard changes and invalid JSON is not enforced
at save time. Update `VariantsSection` to stop writing directly to
`persisted.variants` and `persisted.config` inside `syncSaasVariants` and the
three `useEffect`s; instead route changes through `setPersisted` or new
callbacks from `ClusterDialog` so edits produce a fresh object. Also make
`save()` reject when `variantsError` is set, and ensure `cancelEdit()` restores
the last clean snapshot rather than relying on the mutated state.
| const isAdbc = persisted.engineKey === "adbc"; | ||
| const driver = ((persisted.config as Record<string, unknown>).driver as string) ?? ""; | ||
| const saasDriver = isSaasVariantDriver(driver); | ||
|
|
||
| const [variantRows, setVariantRows] = useState(() => | ||
| saasDriver ? variantsToRows(persisted.variants, driver) : [], | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
VariantsSection reads a stale driver (persisted.config.driver), diverging from save()'s driver resolution.
save() computes driver = editFlat.driver ?? persisted.config.driver (lines 496-498) to account for an in-progress driver edit, but VariantsSection (line 1299) only ever reads persisted.config.driver. If the user changes the driver field while editing, the variant rows/labels rendered and validated inside VariantsSection use the old driver's subResourceFieldSpec, while save() re-validates against the new one — producing inconsistent UI vs. save-time behavior.
🔧 Suggested fix
<VariantsSection
persisted={persisted}
baseClusterName={editClusterName}
onPatchFlat={onPatchFlat}
+ driver={editFlat.driver ?? String((persisted.config as Record<string, unknown>).driver ?? "")}
/> function VariantsSection({
persisted,
baseClusterName,
+ driver,
onPatchFlat: _onPatchFlat,
}: {
persisted: ClusterConfigRecord;
baseClusterName: string;
+ driver: string;
onPatchFlat: (patch: Record<string, string>) => void;
}) {
const isAdbc = persisted.engineKey === "adbc";
- const driver = ((persisted.config as Record<string, unknown>).driver as string) ?? "";
const saasDriver = isSaasVariantDriver(driver);Also applies to: 496-509
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@queryflux-studio/app/clusters/clusters-grid.tsx` around lines 1298 - 1304,
VariantsSection is reading only persisted.config.driver, while save() resolves
the effective driver from editFlat.driver ?? persisted.config.driver, so the
rendered/validated variants can get out of sync with the value being saved.
Update VariantsSection to use the same resolved driver logic as save() (or pass
the resolved driver down from the parent) and use that value consistently when
computing saasDriver, variantsToRows, and any subResourceFieldSpec-based
validation/labels.
| ``` | ||
| 1. custom healthCheckQuery (if set) → execute via ADBC pool | ||
| 2. adapter.health_check() → AdbcIntrospection or SELECT 1 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to fenced code blocks (MD040).
Static analysis flags these fenced blocks as missing a language tag.
📝 Suggested fix
-```
+```text
1. custom healthCheckQuery (if set) → execute via ADBC pool
2. adapter.health_check() → AdbcIntrospection or SELECT 1
```diff
-```
+```text
1. Distributed + not sweep lock owner → read cluster_capacity_counters.running (CapacityStore::active_count)
2. Distributed + sweep lock owner → backend reconcile → publish_running_count → local state
3. Single instance → backend reconcile → local state only
</details>
Also applies to: 166-170
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @website/docs/architecture/cluster-variants-and-health.md around lines 155 -
158, The fenced code blocks in the architecture docs are missing a language
identifier, triggering MD040. Update the affected markdown fences in the cluster
variants and health section to include an explicit language tag such as text,
using the existing fenced blocks that describe the healthCheckQuery and cluster
capacity flows. Keep the content unchanged and ensure all similar fences in this
doc use the same language-tagged style.
</details>
<!-- fingerprinting:phantom:poseidon:beignet -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:0cc9258685f17a553d279f3d -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
cluster variants, ADBC health/reconcile, and distributed running counts
Expand one persisted cluster config into runtime warehouses (base::variant),
with built-in SaaS introspection and optional healthCheckQuery/reconcileQuery.
In distributed mode, reconcile publishes engine ground truth to
cluster_capacity_counters.running; fleet admission stays on capacity leases.
Includes Studio editors, Postgres migration, and architecture docs.
Summary by CodeRabbit
New Features
Bug Fixes