-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(query-orchestrator): evaluate interval refresh keys from local time #11614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,9 +192,17 @@ export class PreAggregationLoadCache { | |
|
|
||
| public async keyQueryResult(sqlQuery: QueryWithParams, waitForRenew: boolean, priority: number) { | ||
| const [query, values, queryOptions] = sqlQuery; | ||
| const redisKey = this.queryCache.queryRedisKey([query, values]); | ||
|
|
||
| if (!this.queryResults[this.queryCache.queryRedisKey([query, values])]) { | ||
| this.queryResults[this.queryCache.queryRedisKey([query, values])] = await this.queryCache.cacheQueryResult( | ||
| if (!this.queryResults[redisKey]) { | ||
| // A locally evaluated key needs no cache driver or queue, but it must still go | ||
| // through this per-request memo. One load reads the invalidation keys several | ||
| // times (contentVersion, the returned refreshKeyValues, the refresh queue key); | ||
| // re-reading the clock could straddle an interval boundary and have the load | ||
| // look a table up under one content version and enqueue it under another. | ||
| const local = this.queryCache.localRefreshKeyResult(queryOptions); | ||
|
|
||
| this.queryResults[redisKey] = local ?? await this.queryCache.cacheQueryResult( | ||
| query, | ||
| values, | ||
| [query, values], | ||
|
|
@@ -212,9 +220,15 @@ export class PreAggregationLoadCache { | |
| } | ||
| ); | ||
| } | ||
| return this.queryResults[this.queryCache.queryRedisKey([query, values])]; | ||
| return this.queryResults[redisKey]; | ||
| } | ||
|
|
||
| // 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. | ||
|
Comment on lines
+226
to
+231
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The analysis here is correct ( |
||
| public hasKeyQueryResult(keyQuery) { | ||
| return !!this.queryResults[this.queryCache.queryRedisKey(keyQuery)]; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,12 @@ import { ContinueWaitError } from './ContinueWaitError'; | |
| import { LocalCacheDriver } from './LocalCacheDriver'; | ||
| import { DriverFactory, DriverFactoryByDataSource } from './DriverFactory'; | ||
| import { LoadPreAggregationResult, PreAggregationDescription } from './PreAggregations'; | ||
| import { getCacheHash, extractRequestUUID } from './utils'; | ||
| import { | ||
| getCacheHash, | ||
| extractRequestUUID, | ||
| evaluateLocalRefreshKey, | ||
| isValidLocalRefreshKey, | ||
| } from './utils'; | ||
| import { CacheAndQueryDriverType, MetadataOperationType } from './QueryOrchestrator'; | ||
|
|
||
| export type CacheQueryResultOptions = { | ||
|
|
@@ -44,12 +49,28 @@ export type CacheQueryResultOptions = { | |
| renewCycle?: boolean, | ||
| }; | ||
|
|
||
| /** | ||
| * Everything needed to evaluate an `every` based refreshKey without touching a | ||
| * database: `FLOOR((utcOffset + unixTimestamp - dayOffset) / interval)`. | ||
| * | ||
| * `utcOffset` is frozen at compile time, exactly as it already is when baked into | ||
| * the emitted SQL string — the orchestrator must never recompute it, or the local | ||
| * and SQL paths would stop agreeing across a DST transition. | ||
| */ | ||
| export type LocalRefreshKeyDescriptor = { | ||
| interval: number; | ||
| utcOffset: number; | ||
| dayOffset: number; | ||
| cron?: boolean; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
| }; | ||
|
|
||
| type QueryOptions = { | ||
| external?: boolean; | ||
| renewalThreshold?: number; | ||
| updateWindowSeconds?: number; | ||
| renewalThresholdOutsideUpdateWindow?: number; | ||
| incremental?: boolean; | ||
| localRefreshKey?: LocalRefreshKeyDescriptor; | ||
| }; | ||
|
|
||
| export type QueryWithParams = [ | ||
|
|
@@ -144,6 +165,7 @@ export interface QueryCacheOptions { | |
| cacheAndQueueDriver: CacheAndQueryDriverType; | ||
| maxInMemoryCacheEntries?: number; | ||
| skipExternalCacheAndQueue?: boolean; | ||
| localRefreshKey?: boolean; | ||
| } | ||
|
|
||
| export class QueryCache { | ||
|
|
@@ -155,6 +177,10 @@ export class QueryCache { | |
|
|
||
| protected memoryCache: LRUCache<string, CacheEntry>; | ||
|
|
||
| protected readonly localRefreshKeyEnabled: boolean; | ||
|
|
||
| protected localRefreshKeyLogged: boolean = false; | ||
|
|
||
| public constructor( | ||
| protected readonly cachePrefix: string, | ||
| protected readonly driverFactory: DriverFactoryByDataSource, | ||
|
|
@@ -181,6 +207,31 @@ export class QueryCache { | |
| this.memoryCache = new LRUCache<string, CacheEntry>({ | ||
| max: options.maxInMemoryCacheEntries || 10000 | ||
| }); | ||
|
|
||
| this.localRefreshKeyEnabled = options.localRefreshKey ?? getEnv('refreshKeyLocalTime'); | ||
| } | ||
|
|
||
| /** | ||
| * Evaluates an interval based refresh key locally, or returns null when the SQL | ||
| * path must be used: the feature is off, the key is one we cannot reproduce | ||
| * (`refreshKey.sql`, incremental), or the descriptor is malformed. | ||
| */ | ||
| public localRefreshKeyResult(queryOptions?: QueryOptions): [{ refresh_key: number }] | null { | ||
| if (!this.localRefreshKeyEnabled || !isValidLocalRefreshKey(queryOptions?.localRefreshKey)) { | ||
| return null; | ||
| } | ||
|
|
||
| // Logged on first use rather than from the constructor, where subclass field | ||
| // initialization has not run yet and `this.logger` may not be usable. | ||
|
Comment on lines
+224
to
+225
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the stated reason isn't right. |
||
| if (!this.localRefreshKeyLogged) { | ||
| this.localRefreshKeyLogged = true; | ||
| this.logger('Local refresh key evaluation enabled', { | ||
| warning: 'Interval based refresh keys are evaluated from this instance clock. ' + | ||
| 'Clocks must be in sync across all API instances and refresh workers.', | ||
| }); | ||
| } | ||
|
|
||
| return evaluateLocalRefreshKey(<LocalRefreshKeyDescriptor>queryOptions?.localRefreshKey); | ||
| } | ||
|
|
||
| public getCacheDriver(): CacheDriverInterface { | ||
|
|
@@ -864,6 +915,14 @@ export class QueryCache { | |
| public async loadRefreshKey(q: QueryWithParams, expireSecs: number, options: LoadRefreshKeyOptions) { | ||
| const [query, values, queryOptions] = q; | ||
|
|
||
| // 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; | ||
| } | ||
|
|
||
|
Comment on lines
+918
to
+925
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The SQL path below resolves For the defaults this is a no-op ( Cheapest fix is to quantize to the effective threshold rather than the raw interval, e.g. pass |
||
| return this.cacheQueryResult( | ||
| query, | ||
| values, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.