feat(query-orchestrator): evaluate interval refresh keys from local time - #11614
feat(query-orchestrator): evaluate interval refresh keys from local time#11614ovr wants to merge 2 commits into
Conversation
…ime behind a flag
Interval and cron based `refreshKey` values can now be computed from the API
instance clock instead of via `SELECT FLOOR(...) as refresh_key`, gated by
`CUBEJS_REFRESH_KEY_LOCAL_TIME` (default `false`).
The compiler already derived every input in JS — `interval`, `dayOffset` and
`utcOffset` come from `parseSecondDuration`/`calcIntervalForCronString`. SQL only
contributed `now()`. So `BaseQuery.everyRefreshKeyParts()` is extracted as the
single source of truth that both the rendered SQL and a serializable descriptor
derive from, and when the flag is on that descriptor rides in the refresh-key
tuple's options element. `QueryCache.loadRefreshKey` and
`PreAggregationLoadCache.keyQueryResult` then short-circuit it.
Why: the default `{ every: '10 seconds' }` cube refresh key has
`renewalThreshold: 10`, so it is re-issued to Cube Store on essentially every
request. That query has no `TableScan`, so `is_data_select_query` is false and it
takes the `QueryPlan::Meta` path — correctly bypassing `SqlResultCache`, but still
paying parser + plan + optimize + `collect`, plus
`MetaStoreSchemaProvider::new(get_tables_with_path(false))` on every call, plus a
WebSocket round trip. All to learn the wall clock.
Scope: cube-level `cacheKeyQueries` and pre-aggregation `invalidateKeyQueries`,
including the `10 seconds` and `1 hour` defaults. Excluded are `refreshKey.sql`
and `incremental` keys — the latter are wrapped in
`CASE WHEN NOW() < <dateTo + updateWindow>` against an allocated partition-range
param, and their options drive `renewalThresholdOutsideUpdateWindow` shortening
for freshly sealed partitions.
Flag off is byte-identical. The gate is applied at emit time as well as consume
time, so the tuples and every hash derived from them are unchanged and no
persisted cache is invalidated on upgrade. The existing `everyRefreshKeySql`
assertions pass unedited, which is what proves the emitted SQL did not move.
Clock skew is why this is flagged. Two nodes straddling an interval boundary
produce different `contentVersion`s, so the same pre-aggregation gets built twice,
recurring at each boundary. `externalRefresh` bounds the blast radius — non-builder
API instances never run these queries — so a single refresh worker is safe;
multiple workers or `CUBEJS_PRE_AGGREGATIONS_BUILDER=true` API instances are not.
Flipping the flag also forces one pre-aggregation rebuild each way, since `pg`
returns `numeric` as a string and Cube Store as a number. Table names are
unaffected; `getStructureVersion` excludes invalidation keys.
Two findings worth recording:
- `preAggregationInvalidateKeyQueries` is memoized against a `queryCache` whose key
did not include the flag, so a flag-on query's compiled refresh keys leaked to a
flag-off query. Fixed by adding `localRefreshKey` to the key list next to the
analogous `convertTzForRawTimeDimension`. A test caught this, not review.
- The `this.queryResults` memo in `keyQueryResult` is load-bearing, not an
optimization. One pre-aggregation load reads the invalidation keys three times
(`contentVersion`, the returned `refreshKeyValues`, the refresh queue key), so
re-reading the clock would let a load crossing a boundary look a table up under
one content version and enqueue it under another.
Also annotates a pre-existing mismatch in `hasKeyQueryResult`, which hashes the
whole 3-element tuple while `keyQueryResult` stores under `[query, values]` — it
already returns false unconditionally. Left as a TODO because aligning the keys
would change sync-vs-background refresh semantics for every pre-aggregation.
| Suite | Result |
| --- | --- |
| `cubejs-backend-shared` `test/env.test.ts` | 25/25 |
| `cubejs-query-orchestrator` `test/unit` | 84/84, 5 suites |
| `cubejs-schema-compiler` `dist/test/unit` | 772/774, 37/38 suites |
The two schema-compiler failures are `error-reporter` snapshots that already fail
on a clean tree, confirmed by stashing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
javascript
Conflict in PreAggregationLoadCache.keyQueryResult, from #11608 ("normalize pre-aggregation SQL to tuples"), which dropped the `Array.isArray` normalization on the line this branch built on. Resolved by taking master's tuple destructuring and keeping the local `redisKey` binding: const [query, values, queryOptions] = sqlQuery; const redisKey = this.queryCache.queryRedisKey([query, values]); The same normalization was dropped from `QueryCache.loadRefreshKey`; git merged that one cleanly and the local-refresh-key short-circuit still sits directly below it. #11608 also narrows `renewQuery`/`startRenewCycle` to accept `string` rather than `string | QueryWithParams` and splits `replacePreAggregationTableNames` into a SQL-only variant. Neither touches the refresh-key path, and the now-guaranteed tuple shape only reinforces the `hasKeyQueryResult` TODO added on this branch. | Suite | Result | | --- | --- | | `cubejs-backend-shared` `test/env.test.ts` | 25/25 | | `cubejs-query-orchestrator` `test/unit` | 84/84, 5 suites | | `cubejs-schema-compiler` `dist/test/unit` | 772/774, 37/38 suites | The orchestrator run includes #11608's own `ReplacePreAggregationTableNames.test.ts`. The two schema-compiler failures are `error-reporter` snapshots that already fail on master. `yarn tsc` and eslint are clean in the orchestrator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @ovr's task in 7m 3s —— View job Review complete — solid, well-scoped change. 1 medium, 5 low/nit (posted inline).Full reviewTodos
What I verifiedThe core refactor holds up. The Opt-out coverage is correct at all four emit sites: The floor-arithmetic claim in
I also confirmed Findings
Smaller observations (not worth inline threads)
PraiseThe Security: nothing to flag. The descriptor is compiler-produced numbers, validated before use, and never reaches SQL. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // A locally evaluated key is free, so there is nothing to cache and no queue to | ||
| // wait on. The value is quantized to the interval, so repeated calls agree | ||
| // except across a boundary — the same window the cached SQL result had. | ||
| const local = this.localRefreshKeyResult(queryOptions); | ||
| if (local) { | ||
| return local; | ||
| } | ||
|
|
There was a problem hiding this comment.
refreshKeyRenewalThreshold is silently bypassed on the local path.
The SQL path below resolves renewalThreshold: this.options.refreshKeyRenewalThreshold || queryOptions?.renewalThreshold || 2 * 60, and PreAggregationLoadCache.keyQueryResult does the same (PreAggregationLoadCache.ts:211). Because the cached refresh-key value is only re-read every renewalThreshold seconds, the observed invalidation cadence today is effectively max(interval, renewalThreshold). The local path returns before any of that, so the cadence becomes exactly interval.
For the defaults this is a no-op (refreshKeyRenewalThresholdForInterval returns min(interval/10, 300), always below the interval). But orchestratorOptions.queryCacheOptions.refreshKeyRenewalThreshold is a supported user knob (optionsValidate.ts:128), and someone who set it to e.g. 300 against the default every: '10 seconds' key is deliberately throttling invalidation 30×. Turning this flag on would silently restore 10s invalidation of their query cache and pre-aggregation content versions — i.e. a large increase in real DB queries, which is the opposite of the PR's intent.
Cheapest fix is to quantize to the effective threshold rather than the raw interval, e.g. pass Math.max(descriptor.interval, this.options.refreshKeyRenewalThreshold || queryOptions?.renewalThreshold || 0) into evaluateLocalRefreshKey. At minimum this interaction should be called out in the docs.
|
|
||
| <Warning> | ||
|
|
||
| Set this variable to the same value on **every** API instance and refresh worker, and keep |
There was a problem hiding this comment.
Two operational consequences worth adding to this warning:
-
Flipping the flag invalidates every pre-aggregation once.
SELECT FLOOR(...) as refresh_keycomes back as a string from several drivers (Postgresbigint/numeric, BigQueryINT64), whileevaluateLocalRefreshKeyreturns a JSnumber.PreAggregationLoader.contentVersionhashes the invalidation key values, so[{refresh_key: "12345"}]and[{refresh_key: 12345}]produce different content versions and every rollup rebuilds on the first request after the toggle. That is a one-time cost, but on a large deployment it is worth warning about (and it applies in both directions). -
Invalidation becomes fleet-synchronized. Today each node caches its refresh-key result independently, so the moment a node notices a new interval is staggered by whenever it last fetched. With synchronized clocks every node flips at the exact same instant, so the primary-query caches across the fleet all miss simultaneously at each boundary. Not incorrect, but a thundering-herd shape that operators should expect.
| interval: number; | ||
| utcOffset: number; | ||
| dayOffset: number; | ||
| cron?: boolean; |
There was a problem hiding this comment.
Nit: cron is never read — neither evaluateLocalRefreshKey nor isValidLocalRefreshKey looks at it, and everyRefreshKeyParts only uses it internally in everyRefreshKeySql to pick the branch. It rides along in every serialized invalidateKeyQueries tuple for no consumer. Either drop it from the descriptor (keep it as a local in everyRefreshKeySql) or add a comment saying it is retained deliberately for debuggability.
| // TODO hashes the whole [query, values, options] tuple, while keyQueryResult stores | ||
| // under [query, values]. Every compiler-produced invalidateKeyQueries entry carries an | ||
| // options object, so this always returns false, which pins PreAggregationLoader to the | ||
| // background-refresh branch whenever waitForRenew is false. Aligning the keys would | ||
| // change sync-vs-background refresh semantics for every pre-aggregation, so it needs to | ||
| // land on its own rather than riding along with an unrelated change. |
There was a problem hiding this comment.
The analysis here is correct (queryRedisKey on a 3-element tuple never matches the 2-element key keyQueryResult stores under, so notLoadedKey at PreAggregationLoader.ts:134 is always truthy), but this is a pre-existing bug in an unrelated method and a six-line TODO is easy to lose in a diff. Consider filing it as an issue and shortening this to a one-line // TODO(#NNNNN) pointer, so it doesn't read as something this PR introduced or is responsible for.
| }); | ||
|
|
||
| expect(executed).toBe(0); | ||
| expect(result).toEqual([{ refresh_key: Math.floor(Date.now() / 1000 / 600) }]); |
There was a problem hiding this comment.
This asserts exact equality against a clock read after the call, so it fails whenever the test happens to straddle a 600-second boundary. Rare, but these are the flakes that show up once a month in CI and cost an hour to diagnose. Same pattern at PreAggregations.test.ts:327.
jest.spyOn(Date, 'now').mockReturnValue(...) — which the boundary-stability test in PreAggregations.test.ts already uses — removes the race entirely.
| // Logged on first use rather than from the constructor, where subclass field | ||
| // initialization has not run yet and `this.logger` may not be usable. |
There was a problem hiding this comment.
Nit: the stated reason isn't right. logger is a TypeScript parameter property, so it is assigned before the constructor body runs — this.logger(...) from the constructor would work. The actual (and better) justification for lazy logging is that the warning should only fire when a descriptor is genuinely used, not on every QueryCache construction in a multi-tenant process. Worth rewording so the comment doesn't send the next reader looking for an initialization-order hazard that isn't there.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11614 +/- ##
==========================================
+ Coverage 59.32% 59.40% +0.07%
==========================================
Files 228 228
Lines 18206 18234 +28
Branches 3672 3679 +7
==========================================
+ Hits 10801 10831 +30
+ Misses 6858 6855 -3
- Partials 547 548 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
2 participants
Check List
Description of Changes Made
Adds
CUBEJS_REFRESH_KEY_LOCAL_TIME(defaultfalse): when enabled, interval and cron basedrefresh_keyvalues are computed from the API instance's own clock instead of via aSELECT FLOOR(...) as refresh_keyround trip — the default{ every: '10 seconds' }key carriesrenewalThreshold: 10, so today it is re-issued to Cube Store on nearly every request just to read the wall clock. The compiler already derived every input in JS (interval,dayOffset,utcOffset), soBaseQuery.everyRefreshKeyParts()is extracted as the single source of truth behind both the rendered SQL and a serializable descriptor, whichQueryCache.loadRefreshKeyandPreAggregationLoadCache.keyQueryResultthen short-circuit. Onlyevery-based keys are affected;refreshKey.sqlandincrementalkeys still run against the database, since the latter are wrapped inCASE WHEN NOW() < <dateTo>against an allocated partition-range param. The flag is gated at emit time as well as consume time, so with it off the refresh-key tuples and every hash derived from them are byte-identical — the existingeveryRefreshKeySqlassertions pass unedited, which is what proves the emitted SQL did not move. It ships behind a flag because clock skew matters: two nodes straddling an interval boundary compute differentcontentVersions and can build the same pre-aggregation twice, so a single refresh worker is safe while multiple builders are not.Test results:
cubejs-backend-shared25/25,cubejs-query-orchestratortest/unit84/84,cubejs-schema-compilerdist/test/unit772/774 (the two failures areerror-reportersnapshots that already fail on a clean master, confirmed by stashing). Not yet smoke-tested against a live deployment.🤖 Generated with Claude Code