feat(query-orchestrator): honour refreshKeyRenewalThreshold on locally evaluated refresh keys - #11720
feat(query-orchestrator): honour refreshKeyRenewalThreshold on locally evaluated refresh keys#11720ovr wants to merge 1 commit into
Conversation
`CUBEJS_REFRESH_KEY_LOCAL_TIME` used to switch itself off whenever `queryCacheOptions.refreshKeyRenewalThreshold` was set, because the threshold caches the refresh key query result and that cache is also what bounds how often an `every`-based key can advance — a locally computed key has no cache entry to age out, so it would have advanced on every interval boundary and multiplied pre-aggregation rebuilds. Reproduce the bound with arithmetic instead: sample the clock at the threshold granularity (`snapToRenewalThreshold`) and evaluate the key at that instant. The two options now compose, and a deployment that sets a threshold keeps local evaluation. | | before | after | | --- | --- | --- | | threshold unset | local, no query | local, no query | | threshold set | falls back to SQL per key | local, advances once per window | Snapping keeps the value inside the key's own series — `144` really is a 10 minute key's value at the start of day two — and every instance samples the same instant, where the SQL path's phase depended on when each cache entry happened to be written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Claude finished @ovr's task in 2m 38s —— View job Review: approach is sound — 0 blocking, 1 medium (synchronized refresh boundaries), 3 lowFull review — expand for detailsTodo
SummaryThe core idea — reproduce the cache-derived rate bound arithmetically instead of vetoing local evaluation — is correct and a genuine improvement over the
Findings below, none blocking. Medium — refresh keys now all flip at the same instant
Low
Nits (no action needed)
Tests & docsCoverage is good: the helper is tested directly (boundaries, monotonicity, the Not verifiedI could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export function snapToRenewalThreshold(nowMs: number, thresholdSeconds?: number): number { | ||
| if (!Number.isFinite(thresholdSeconds) || <number>thresholdSeconds <= 0) { | ||
| return nowMs; | ||
| } | ||
|
|
||
| const thresholdMs = <number>thresholdSeconds * 1000; | ||
|
|
||
| return Math.floor(nowMs / thresholdMs) * thresholdMs; | ||
| } |
There was a problem hiding this comment.
Synchronized boundaries replace staggered ones — worth acknowledging the ops trade-off.
Math.floor(nowMs / thresholdMs) * thresholdMs aligns to the Unix epoch, so with the common refreshKeyRenewalThreshold: 86400 every every-based refresh key, in every cube, tenant and timezone, flips at exactly 00:00 UTC. The SQL path it replaces bounded the rate the same way but phased each key by whenever its cache entry happened to be written, which spread the invalidations out.
The PR description frames the cross-instance determinism as a win, and it is — but the flip side is that all the pre-aggregation rebuilds a threshold was meant to throttle now land in one burst. On a deployment with many pre-aggregations that burst is what the queue sees at midnight.
You can keep determinism and the staggering by phasing per key rather than globally, e.g.:
export function snapToRenewalThreshold(nowMs: number, thresholdSeconds?: number, phaseSeed = 0): number {
...
const offset = phaseSeed % thresholdMs;
return Math.floor((nowMs - offset) / thresholdMs) * thresholdMs + offset;
}with phaseSeed derived from the refresh key identity hash (QueryCache.refreshKeyIdentity), which is stable across instances and restarts. If you'd rather not, a sentence in the env-var docs warning that thresholds now align to UTC boundaries would set expectations.
| expect(executed).toBe(0); | ||
| expect(result).toEqual([{ | ||
| refresh_key: String(Math.floor(Math.floor(Date.now() / 1000 / day) * day / descriptor.interval)), | ||
| }]); | ||
| }); |
There was a problem hiding this comment.
Nit: this re-reads the real Date.now() after loadRefreshKey ran, so the assertion is a coin flip if the clock crosses the daily boundary in between — a once-a-day flake at 00:00 UTC. The sibling test you added in PreAggregations.test.ts mocks Date.now; doing the same here would make the expected value a literal instead of a re-derivation of the implementation's own formula, which is also a stronger test.
(The existing 10-minute-interval assertions at lines 604/708 have the same shape, so this is consistent with the file — just noting it since the fix is cheap on a new line.)
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #11720 +/- ##
===========================================
- Coverage 79.93% 59.03% -20.91%
===========================================
Files 498 237 -261
Lines 104742 19012 -85730
Branches 3857 3861 +4
===========================================
- Hits 83727 11223 -72504
+ Misses 20454 7230 -13224
+ Partials 561 559 -2
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:
|
1 participant
Check List
Description of Changes Made
CUBEJS_REFRESH_KEY_LOCAL_TIMEused to disable itself wheneverqueryCacheOptions.refreshKeyRenewalThresholdwas set, since the threshold caches the refresh key query result and that cache is also what bounds how often anevery-based key can advance — a locally computed key has no cache entry to age out, so it would have advanced on every interval boundary and multiplied pre-aggregation rebuilds. This reproduces the bound with arithmetic instead: the newsnapToRenewalThresholdsamples the clock at the threshold granularity and the key is evaluated at that instant, so the two options compose and a deployment with a threshold keeps local evaluation (advancing at most once per window, with no query). Snapping keeps the value inside the key's own series —144really is a 10 minute key's value at the start of day two — and every instance samples the same instant, where the SQL path's phase depended on when each cache entry happened to be written.RefreshSchedulerconsequently stops warming interval keys under a threshold, since there is no longer a cache entry worth warming. Unit tests cover the snapping helper, bothQueryCachepaths and the scheduler, and the env var reference is updated.