Centralized job locking into one Locker+JobBuffer per process#122
Draft
stephenbinns wants to merge 3 commits into
Draft
Centralized job locking into one Locker+JobBuffer per process#122stephenbinns wants to merge 3 commits into
stephenbinns wants to merge 3 commits into
Conversation
Replaces the per-worker-thread Locker (each thread independently running its own recursive-CTE lock query) with a single Locker per process that batch-locks jobs into a shared JobBuffer, which worker threads then pull from without ever querying Postgres themselves. This collapses lock queries from one-per-worker-thread to one-per-process, addressing lock contention that scales worse as worker count grows. Also fixes two related correctness issues surfaced while building this: pg_try_advisory_lock is reentrant within a session, so the Locker now tracks and excludes job_ids it already holds from every lock query (otherwise the cursor-reset-on-empty retry could re-lock and double-dispatch an in-flight job); and the poll backoff now always waits a full cycle when the buffer isn't full, rather than busy-looping when nothing is found. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds an architecture: :legacy|:centralized option to WorkerGroup.start, a matching -a/--architecture CLI flag (and QUE_ARCHITECTURE env var) on bin/que, and documents both in the README. Defaults to :legacy (the existing per-worker-thread locking behaviour) so the new centralized Locker+JobBuffer path from the previous commit is strictly opt-in until it's been proven out, per the staged rollout this was originally planned around. The pre-centralization Locker/Worker are preserved verbatim as LegacyLocker/LegacyWorker to back the :legacy path, reusing the existing Locker/Worker Prometheus metric constants rather than declaring duplicates. Also parametrizes the integration suite to run against both architectures, and fixes a latent test-isolation gap (InterruptibleSleepJob.log was never reset between examples) that only surfaced once the same example ran twice in one process. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restructures the previous two commits for a cleaner diff: Locker and Worker are now byte-identical to master again (the pre-centralization, per-worker-thread implementation), and the new centralized Locker+JobBuffer architecture lives entirely in new CentralizedLocker/ CentralizedWorker classes instead of overwriting the originals. WorkerGroup's architecture: toggle now selects between Locker/Worker (:legacy) and CentralizedLocker/CentralizedWorker (:centralized). CentralizedLocker/CentralizedWorker reuse Locker/Worker's existing Prometheus metric constants rather than redeclaring them, so dashboards don't change based on which architecture is selected. Also reverts lock_job's cursor predicate back to `>=` (matching master exactly - it's only used by the untouched legacy path, which never needed the `>` fix). find_job_to_lock keeps the `>` fix since it's shared with the new batched adapter path where it's still required for correctness; added a comment explaining why the two queries now differ. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
stephenbinns
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As an opt in we can replace the per-worker-thread Locker (each thread independently running its own recursive-CTE lock query) with a single Locker per process that batch-locks jobs into a shared JobBuffer, which worker threads then pull from without ever querying Postgres themselves. This collapses lock queries from one-per-worker-thread to one-per-process, addressing lock contention that scales worse as worker count grows.
Also fixes two related correctness issues surfaced while building this: pg_try_advisory_lock is reentrant within a session, so the Locker now tracks and excludes job_ids it already holds from every lock query (otherwise the cursor-reset-on-empty retry could re-lock and double-dispatch an in-flight job); and the poll backoff now always waits a full cycle when the buffer isn't full, rather than busy-looping when nothing is found.