Add worker queue #630
Add worker queue #630
Conversation
|
It's unclear to me why tests are failing in CI -- they are passing locally. Update: pretty sure the issue is that the tests themselves don't trigger migrations for the job queue (whereas in my case the migrations had already been run). Second Update: yep! I'm opting to just mock the queue library to avoid any interaction with the worker queue in our tests. |
slifty
force-pushed
the
559-add-worker-queue
branch
from
9fa5727
to
30d4c8e
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
==========================================
- Coverage 94.77% 92.92% -1.86%
==========================================
Files 74 80 +6
Lines 1071 1130 +59
Branches 169 180 +11
==========================================
+ Hits 1015 1050 +35
- Misses 51 75 +24
Partials 5 5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I haven't tested this yet; just a quick review!
Also @slifty would you mind talking through how you decided on graphile-worker? I'm 100% on board with that choice, just curious to hear your reasoning!
slifty
force-pushed
the
559-add-worker-queue
branch
2 times, most recently
from
4a60962
to
e014f49
Compare
There was a problem hiding this comment.
As you called out in the description, this needs to be rebased on main now that #628 is merged.
I was able to run this locally, and both see successful jobs by using the new bulk upload API endpoint, and failed jobs by using the SQL interface to add a job without a payload.
I think we want to configure logging as part of this, but otherwise this is looking great!
slifty
force-pushed
the
559-add-worker-queue
branch
3 times, most recently
from
b58a76d
to
6886b2d
Compare
There was a problem hiding this comment.
This looks good to me! I was able to run this locally again, now with prettier logs. The common database connection pool seems to be working well, and I was able to drop the database schema (drop schema graphile_worker cascade;) and recreate via npm run migrate:dev.
Need to fix the failing test, but that should be pretty minor, so +1! Nice work, @slifty.
slifty
force-pushed
the
559-add-worker-queue
branch
2 times, most recently
from
f8b68ee
to
efea25d
Compare
| import { jobQueueLogger } from '../jobQueue'; | ||
| import type { JobHelpers } from 'graphile-worker'; | ||
|
|
||
| export const getMockJobHelpers = (): JobHelpers => ({ |
There was a problem hiding this comment.
@jasonaowen this is what I did for mocking job helpers -- we only use the logger helper, and since this is JUST for test code I think it's OK to be a little more "fast and loose" / not try to mock all the other things that JobHelpers can do. Worst case a future test fails and we expand the mock at that point.
We want to process bulk uploads asynchronously, and so this means we need a worker queue. Since this initial task is going to be infrequent, we felt it did not warrant adding a new dependency to our stack (e.g. reddis or rabbitmq), and so we're going to just leverage postgres as our backend. I explored two tools for this purpose: pg-boss, and graphile-worker. Both packages appeared to meet our technical needs, and both seem to be maintained and similarly popular. I decided to go with graphile-worker as it appears to be part of a suite of related tools and the primary developer appears to have built a community around that suite of tools (and earns some income to support the development of the project). Issue #559 Set up a queue / processing system for handling bulk proposal uploads
slifty
force-pushed
the
559-add-worker-queue
branch
from
efea25d
to
00bc5ce
Compare
|
I'll merge this after CI completes (I did add that |
This PR adds support to the PDC for a worker queue and creates a "shell" worker for bulk upload processing (the worker doesn't actually do anything yet).
This builds on the work of #628 and shouldn't be reviewed until that has been merged and this PR has been rebased.
Related to #559