feat: move matview refresh out of server pods (--matview-refresh-only, matview_refresh_interval "manual") - #5870
hugochinchilla wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (16)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds migration lifecycle flags, one-shot maintenance execution, and manual materialized-view refresh mode. Framework stores, the HTTP transport, Docker entrypoint, schemas, tests, changelogs, and deployment documentation now support these controls. ChangesMigration and maintenance lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds manual materialized-view refresh and a refresh-only execution mode without a concrete merge-blocking issue; it is merge-ready after normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DockerEntrypoint
participant BifrostHTTPMain
participant BifrostHTTPServer
participant PostgreSQLMaintenance
DockerEntrypoint->>BifrostHTTPMain: Pass lifecycle flags
BifrostHTTPMain->>BifrostHTTPServer: RunMaintenance(ctx)
BifrostHTTPServer->>PostgreSQLMaintenance: Load configuration and run selected maintenance
PostgreSQLMaintenance-->>BifrostHTTPMain: Return success or error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description covers the purpose, implementation changes, affected areas, testing steps, configuration and environment changes, breaking-change status, related issues, and security considerations. The omitted UI screenshots section is not applicable. Full details: Docstring CoverageExplanation Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 9 files. (8 skipped: 7 unsupported, 1 too large.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.12.2)Error: can't load config: the Go language version (go1.26) used to build golangci-lint is lower than the targeted Go version (1.27.0) Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/deployment-guides/config-json/storage.mdx`:
- Line 282: Update the matview_refresh_interval documentation to clarify that
"manual" preserves the views, performs one create/repair/refresh cycle at server
startup, and then disables recurring server-pod refreshes; retain the reference
to out-of-band refreshes for ongoing maintenance.
In `@framework/logstore/clickhousemigrate.go`:
- Around line 332-337: Update the ClickHouse migration startup flow around
migrator.SkipStartupMigrations() to fail closed unless it can prove all required
migrations are applied. Add or reuse persisted migration-state tracking or a
non-mutating schema-current preflight, and return a descriptive error when
schema currency cannot be verified; do not return nil solely because
--no-migrate is enabled. Verify the migration behavior for framework/** changes.
In `@transports/config.schema.json`:
- Around line 1902-1903: Update the schema pattern used for the matview refresh
interval to accept valid Go duration strings with fractional numeric values and
multiple repeated unit segments, while continuing to allow the existing “off”
and “manual” values. Anchor the change to the pattern alongside the matview
refresh description and keep unsupported formats excluded.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 101b04f4-4cae-44bd-b3df-66ad0ed421e8
📒 Files selected for processing (17)
docs/architecture/framework/config-store.mdxdocs/deployment-guides/config-json/storage.mdxdocs/deployment-guides/k8s.mdxdocs/quickstart/gateway/setting-up.mdxframework/changelog.mdframework/configstore/migrations.goframework/configstore/migrations_test.goframework/logstore/clickhousemigrate.goframework/logstore/matviews_lock_test.goframework/logstore/migrations.goframework/logstore/postgres.goframework/migrator/skip.gotransports/bifrost-http/main.gotransports/bifrost-http/server/server.gotransports/changelog.mdtransports/config.schema.jsontransports/docker-entrypoint.sh
| | Field | Default | Description | | ||
| |-------|---------|-------------| | ||
| | `matview_refresh_interval` | `"1m"` | How often to refresh dashboard materialized views. Accepts any Go duration string (`"1m"`, `"5m"`, `"1h"`); positive values below `5s` are clamped up to `5s`. Set `"off"` or a zero duration (`"0s"`) to disable matview maintenance entirely. | | ||
| | `matview_refresh_interval` | `"1m"` | How often to refresh dashboard materialized views. Accepts any Go duration string (`"1m"`, `"5m"`, `"1h"`); positive values below `5s` are clamped up to `5s`. Set `"off"` or a zero duration (`"0s"`) to disable matview maintenance entirely. Set `"manual"` to keep the views but refresh them out of band — see [moving materialized view refresh out of server pods](/deployment-guides/k8s#moving-materialized-view-refresh-out-of-server-pods). | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the startup refresh in "manual" mode.
The current text says that "manual" refreshes views out of band. Server pods still create, repair, and refresh the views once at boot. State that only recurring refreshes move out of band.
As per path instructions, "manual" preserves the views and refreshes them once at boot before disabling recurring server-pod refresh.
🤖 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 `@docs/deployment-guides/config-json/storage.mdx` at line 282, Update the
matview_refresh_interval documentation to clarify that "manual" preserves the
views, performs one create/repair/refresh cycle at server startup, and then
disables recurring server-pod refreshes; retain the reference to out-of-band
refreshes for ongoing maintenance.
Source: Path instructions
| // No migration ledger exists for ClickHouse, so unlike the SQL stores we | ||
| // cannot verify the schema is current — --no-migrate just trusts the | ||
| // out-of-band --migrate-only run. | ||
| if migrator.SkipStartupMigrations() { | ||
| logger.Info("[logstore] --no-migrate set; skipping clickhouse migration run") | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Fail closed when the ClickHouse schema state is unknown.
Line 335 returns success without checking whether ClickHouse migration steps were applied. A server started with --no-migrate can then use stale ClickHouse tables after a missed or failed --migrate-only job. The SQL stores reject this state, but ClickHouse does not.
Persist applied ClickHouse migration IDs, or add a non-mutating schema-current preflight. Return an error if the process cannot prove that the schema is current. Do not return nil only because SkipStartupMigrations() is true.
Based on PR objectives, --no-migrate must enforce the out-of-band migration workflow. As per path instructions, verify migration behavior for framework/** changes.
🤖 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 `@framework/logstore/clickhousemigrate.go` around lines 332 - 337, Update the
ClickHouse migration startup flow around migrator.SkipStartupMigrations() to
fail closed unless it can prove all required migrations are applied. Add or
reuse persisted migration-state tracking or a non-mutating schema-current
preflight, and return a descriptive error when schema currency cannot be
verified; do not return nil solely because --no-migrate is enabled. Verify the
migration behavior for framework/** changes.
Source: Path instructions
| "description": "How often to refresh dashboard materialized views. Go duration string (e.g. '1m', '5m', '1h'). Default 1m. Raise this when matview refresh CPU cost is material on the database instance. Positive values below 5s are clamped up to 5s. Set 'off' or a zero duration (e.g. '0s') to disable materialized-view maintenance entirely; dashboard queries fall back to the raw tables. Set 'manual' to keep the views — created, repaired, and refreshed once at boot — but move the recurring refresh out of the server pods to an out-of-band job (a --matview-refresh-only CronJob, pg_cron).", | ||
| "pattern": "^(off|manual|[0-9]+(ns|us|µs|ms|s|m|h))$", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 'func resolveMatViewRefreshInterval|time\.ParseDuration|matViewRefreshManual' framework/logstore
rg -n -C 2 '"matview_refresh_interval"' transports/config.schema.jsonRepository: maximhq/bifrost
Length of output: 7204
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1890,1912p' transports/config.schema.jsonRepository: maximhq/bifrost
Length of output: 2084
Accept fractional and composite Go durations.
resolveMatViewRefreshInterval uses time.ParseDuration(raw), but the schema pattern only allows whole-number single-unit durations. Users can enter valid Go durations such as "1.5s" or "1h30m" and receive the default. Update the pattern to allow fractional numbers and repeated units while keeping "off" and "manual".
🤖 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 `@transports/config.schema.json` around lines 1902 - 1903, Update the schema
pattern used for the matview refresh interval to accept valid Go duration
strings with fractional numeric values and multiple repeated unit segments,
while continuing to allow the existing “off” and “manual” values. Anchor the
change to the pattern alongside the matview refresh description and keep
unsupported formats excluded.
Sources: Coding guidelines, Path instructions
244a01d to
ce1b2a6
Compare
|
|
… serving When multiple pods start at once they all race to run schema migrations, serialized only by an advisory lock with a 1-minute timeout — slow migrations make sibling pods crash on lock timeout. These flags let operators run migrations as a one-shot job (--migrate-only) while server pods boot with --no-migrate, which skips DDL entirely and fails fast with operator guidance if the schema is not current. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The entrypoint previously hardcoded the args passed to /app/main, so the new flags were unreachable in the official image; they are now settable via container args or BIFROST_MIGRATE_ONLY / BIFROST_NO_MIGRATE env vars. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The catch-all branch rotated unrecognized args (set -- "$@" "$1"; shift), keeping $# constant and looping forever — hit by the default CMD /app/main now that parse_args runs for a single argument. Unrecognized args were never forwarded (exec passes a fixed list), so just drop them. Reported by greptile on maximhq#5300. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
--migrate-only now runs the logstore index maintenance pass (GIN and performance index builds, dashboard backfill) synchronously before exiting, instead of spawning the background goroutine and killing it on Close — which left INVALID indexes behind for the next boot to rebuild. --no-migrate skips the pass entirely so server pods never contend on the index advisory lock; the one-shot job owns it. Default single-process mode keeps the background build unchanged. Also log a warning when the index lock can't be acquired instead of silently skipping, and only print "[logstore] pending migrations" when there actually are pending migrations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every server pod refreshes the dashboard materialized views on its own timer; the refresh gate is per-process, so with steady traffic the work scales with replica count. This adds a way to run it once, elsewhere. "manual" is a third mode alongside the "off" that maximhq#5693 shipped, not a redefinition of it: "off" means the views must not exist and the read path stays on the raw tables, while "manual" keeps the views — created, repaired, and refreshed once at boot, with matViewsReady set — and only drops the ticker. An out-of-band refresher (a --matview-refresh-only CronJob, pg_cron) then owns the cadence. --matview-refresh-only implies --no-migrate: the job runs on a cron against a database the --migrate-only job owns. The index-maintenance switch now checks SkipStartupMigrations before OneShotMaintenance so the refresh job doesn't re-run index maintenance on every tick. Renames MigrateOnly to OneShotMaintenance and RunMigrations to RunMaintenance, since both flags now share that path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTszdWT2EDrVntP5A6q6V3
b875ca5 to
8066ee8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Summary
Moves the recurring dashboard materialized-view refresh out of the server pods. Every pod refreshes the matviews on its own timer and the refresh gate is per-process, so with steady traffic the refresh load scales with replica count. This adds a way to run that pass once, elsewhere — a CronJob or
pg_cron— while the serving pods still create, repair, and use the views.Note
Stacked on #5300. The commits below #5300's are its
--migrate-only/--no-migratework; this PR's own change is the last commit. Please merge #5300 first, then this rebases to a single commit.Changes
matview_refresh_interval: "manual"— a third mode alongside the"off"that feat: support matview_refresh_interval "off" to disable logstore matview maintenance #5693 shipped, deliberately not a redefinition of it:matViewsReady"1m"(default)"off"(feat: support matview_refresh_interval "off" to disable logstore matview maintenance #5693)"manual"(new)"off"means the views must not exist and the read path stays on the raw tables — so an out-of-band refresher alone wouldn't help there, sincematViewsReadynever flips and queries never use the views."manual"keeps the views and only drops the ticker.--matview-refresh-only— refreshes all views once and exits, without running migrations or starting the server. AlsoBIFROST_MATVIEW_REFRESH_ONLY=1via the Docker entrypoint. Implies--no-migrate: the job runs on a cron against a database the--migrate-onlyjob owns.The index-maintenance switch now checks
SkipStartupMigrations()beforeOneShotMaintenance(), so a matview refresh job doesn't re-run index maintenance on every tick.migrator.MigrateOnly→OneShotMaintenance,RunMigrations→RunMaintenance— both flags now share that path.The sentinel for
"manual"istime.Duration(-1), which cannot collide with a parsed value: any non-positive duration already resolves to0.matViewMaintenanceDisabledis nowrefreshInterval == 0rather than<= 0, so self-heal stays armed in manual mode (the views are ours to repair).Type of change
Affected areas
How to test
TestResolveMatViewRefreshIntervalDefaultspins that"manual"resolves to the sentinel and that no other input —"", a bad string,"1s","5m","off","0s","-1m"— collides with it.Against a Postgres logstore:
matview_refresh_interval: "manual", start the server. Boot logs show the views created and refreshed once, thenperiodic matview refresh disabled (matview_refresh_interval=manual). Dashboards serve from the views (no raw-table fallback).bifrost-http --matview-refresh-only. It logsmaterialized views refreshed in N ms; exitingand returns 0, without applying migrations."off"is unchanged: no views created, dashboard queries fall back to the raw tables.New config value:
matview_refresh_interval: "manual"(documented indocs/deployment-guides/config-json/storage.mdx). New env var:BIFROST_MATVIEW_REFRESH_ONLY. The k8s guide gains a CronJob example.Breaking changes
"off"keeps the semantics #5693 shipped;"manual"is additive.Related issues
Follow-up to #5693. Stacked on #5300.
Security considerations
None. No changes to auth, secrets, or network surface. The refresh job runs with the same config and credentials as the server pods.