-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(plugin-server): Add capability to use deferred overrides writer and worker #19112
Changes from 3 commits
347c425
14541c5
94ef79c
a4174e6
3bc2290
e2a7487
7c1ad87
dbfb158
9a7a729
e24f427
6c4b2d9
f239cd9
9784c7b
8d24b2a
c812221
432bddc
fa1fc26
b7de72b
4d1114c
fb43fbd
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 |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import { status } from './status' | ||
import { sleep } from './utils' | ||
|
||
export class PeriodicTask { | ||
private promise: Promise<void> | ||
private running = true | ||
private abortController: AbortController | ||
|
||
constructor(task: () => Promise<void> | void, intervalMs = 1000, minimumWaitMs = 1000) { | ||
tkaemming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.abortController = new AbortController() | ||
|
||
const abortRequested = new Promise((resolve) => { | ||
this.abortController.signal.addEventListener('abort', resolve, { once: true }) | ||
}) | ||
|
||
this.promise = new Promise<void>(async (resolve, reject) => { | ||
try { | ||
status.debug('🔄', 'Periodic task starting...', { task }) | ||
while (!this.abortController.signal.aborted) { | ||
const startTimeMs = +Date.now() | ||
await task() | ||
const waitTimeMs = Math.max(intervalMs - startTimeMs, minimumWaitMs) | ||
tkaemming marked this conversation as resolved.
Show resolved
Hide resolved
|
||
status.debug('🔄', `Next evaluation in ${waitTimeMs / 1000}s`, { task }) | ||
await Promise.race([sleep(waitTimeMs), abortRequested]) | ||
} | ||
status.info('✅', 'Periodic task stopped by request.', { task }) | ||
resolve() | ||
} catch (error) { | ||
status.warn('⚠️', 'Error in periodic task!', { task, error }) | ||
reject(error) | ||
} finally { | ||
this.running = false | ||
} | ||
}) | ||
} | ||
|
||
public isRunning(): boolean { | ||
return this.running | ||
} | ||
|
||
public async stop(): Promise<void> { | ||
this.abortController.abort() | ||
try { | ||
await this.promise | ||
} catch {} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -712,7 +712,7 @@ export class DeferredPersonOverrideWriter { | |||||||||||||||||||||||||||||||||||||
* @param lockId the lock identifier/key used to ensure that only one | ||||||||||||||||||||||||||||||||||||||
* process is updating the overrides at a time | ||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||
constructor(private postgres: PostgresRouter, private lockId: number) {} | ||||||||||||||||||||||||||||||||||||||
constructor(private postgres: PostgresRouter, private lockId: number = 567) {} | ||||||||||||||||||||||||||||||||||||||
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. is that intentional? how is this lockId used? 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. Yes, this is intentional; this is for the advisory lock acquired here that ensures only one process at a time is processing pending overrides to avoid race conditions in transitive overrides processing from overlapping transactions (also described on line 712 above): posthog/plugin-server/src/worker/ingestion/person-state.ts Lines 758 to 768 in 7e2065a
It's annoying that these are numbers and some varchar/text type that could be more descriptive, but that's what the Postgres API provides: https://www.postgresql.org/docs/current/explicit-locking.html#ADVISORY-LOCKS The value could also be provided via a setting, but I couldn't think of a reason to make that configurable so I avoided that for now. I guess this could be a class property rather than an instance property though, I had initially made it an instance property so that the functional test runner could be running while other unit/integration tests were run — but thinking about it more, those are still sharing the same tables on the test database so they probably should conflict with each other. It's possible that this could get replaced with a table lock on the overrides (not pending overrides) table but I want to defer that at least until the immediate overrides path is fully removed so that we don't lock the overrides table by accident while it's still being used in the ingest consumer transaction during the changeover. 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. Made this static with e2a7487. 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. It's minor, but it may be worth putting a "yep, this is how the PG API works, we're just picking a consistent int yadda yadda" line alongside your existing comment. I have to admit I read it and was still head-tilt at the magic number. 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. How does this look? fb43fbd posthog/plugin-server/src/worker/ingestion/person-state.ts Lines 754 to 760 in fb43fbd
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||
* Enqueue an override for deferred processing. | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { PeriodicTask } from '../../src/utils/periodic-task' | ||
|
||
describe('PeriodicTask', () => { | ||
describe('updates completion status', () => { | ||
it('on success', async () => { | ||
const fn = jest.fn() | ||
const task = new PeriodicTask(fn) | ||
expect(fn).toBeCalled() | ||
expect(task.isRunning()).toEqual(true) | ||
await task.stop() | ||
expect(task.isRunning()).toEqual(false) | ||
}) | ||
|
||
it('on failure', async () => { | ||
const fn = jest.fn(() => { | ||
throw new Error() | ||
}) | ||
const task = new PeriodicTask(fn) | ||
expect(fn).toBeCalled() | ||
await task.stop() | ||
expect(task.isRunning()).toEqual(false) | ||
}) | ||
}) | ||
}) |
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.
This is off by default, keeping it from inadvertently being enabled for hobby deploys.