Skip to content
This repository has been archived by the owner on Oct 24, 2024. It is now read-only.

i718 Add auxiliary worker to run different queues #723

Merged
merged 18 commits into from
Feb 23, 2024

Conversation

bkiahstroud
Copy link
Contributor

@bkiahstroud bkiahstroud commented Feb 9, 2024

Story

Refs:

Add a second worker that runs all GoodJob queues. The "base" worker will run all queues except the :auxiliary queue.

This, combined with the high priority assigned to all jobs in the :auxiliary queue, effectively builds a fast lane; all jobs run in the :auxiliary queue will be executed before all other jobs by the auxiliary worker.

The fact that the base worker does not run any jobs in the :auxiliary queue means we can have this fast lane without putting every other job on hold for, potentially, long periods of time.

Expected Behavior Before Changes

A single GoodJob process runs all background job queues (*)

Expected Behavior After Changes

Two GoodJob processes each run different queues:

  • The default worker runs all queues except the :auxiliary queue (-auxiliary)
  • The auxiliary worker runs all queues (*)

Screenshots / Video

Good Job Dashboard 2024-02-22 at 11 31 20 AM

Notes

The reason we use before_enqueue in ApplicationJob is because GoodJob does not support setting a default priority per queue.

Newer versions of GoodJob do support the ability to run queues in the order they're listed, but this currently has problems.

Add a second worker that runs all GoodJob queues. The "base" worker will
run all queues except the :auxiliary queue.

This, combined with the high priority assigned to all jobs in the
:auxiliary queue, effectively builds a fast lane; all jobs run in the
:auxiliary queue will be executed before all other jobs by the auxiliary
worker.

The fact that the base worker does not run any jobs in the :auxiliary
queue means we can have this fast lane without putting every other job
on hold for, potentially, long periods of time.

P.S. The reason we use `before_enqueue` in ApplicationJob is because
GoodJob does not support setting a default priority per queue.
This resolves the "duplicate name" warning when deploying. However, it
may remove all other ENV vars from the base worker, so this change may
not stick around
This is an attempt to resolve this helm upgrade error:
> The order in patch list: ... doesn't match $setElementOrder list: ...
Copy link

gitguardian bot commented Feb 12, 2024

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9527551 Triggered Generic Database Assignment f1aa344 ops/dev-deploy.tmpl.yaml View secret
9527551 Triggered Generic Database Assignment 4afc1df ops/dev-deploy.tmpl.yaml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

Attempting to merge/override the same ENV variable in different
deployments ended up being very difficult. This is perhaps less elegant,
but simpler
Attempting to use the --queues flag didn't work, but using the ENV
variable does
@bkiahstroud bkiahstroud marked this pull request as ready for review February 22, 2024 19:20
Copy link
Contributor

@jeremyf jeremyf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to block this, because there's issues regarding jobs needing specific relative priority.

Copy link
Contributor Author

@bkiahstroud bkiahstroud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremyf I believe the issues you're referring to will be solved by @laritakr's PR, which will be merged right behind mine. Let me know if my understanding of that is off

def set_auxiliary_queue_priority
return unless queue_name.to_sym == :auxiliary

self.priority = ENV.fetch('AUXILIARY_QUEUE_PRIORITY', 100).to_i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting of priority will clobber the nuanced priority of a later PR.

Copy link
Contributor Author

@bkiahstroud bkiahstroud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremyf here are the results of QA'ing locally with these changes combined with LaRita's. No all the Queue Name values are correct for SDAPI, but all the priorities do get adjusted as expected, which makes me think it will work as intended.

Tenant Job Class Queue Name Priority
SDAPI AttachFilesToWorkJob auxiliary 109
Dev AttachFilesToWorkJob ingest -1
SDAPI BatchCreateJob auxiliary 110
Dev BatchCreateJob ingest 0
SDAPI Bulkrax::ImportWorkJob auxiliary 105
Dev Bulkrax::ImportWorkJob ingest -5
SDAPI Bulkrax::ImporterJob auxiliary 90
Dev Bulkrax::ImporterJob ingest -20
SDAPI Bulkrax::ScheduleRelationshipsJob default 160
Dev Bulkrax::ScheduleRelationshipsJob default 50
SDAPI CharacterizeJob auxiliary 140
Dev CharacterizeJob ingest 30
SDAPI ContentDepositEventJob auxiliary 60
Dev ContentDepositEventJob ingest -50
SDAPI CreateDerivativesJob auxiliary 150
Dev CreateDerivativesJob ingest 40
SDAPI CreateWorkJob auxiliary 110
Dev CreateWorkJob ingest 0
SDAPI FileSetAttachedEventJob auxiliary 110
Dev FileSetAttachedEventJob ingest 0
SDAPI Hyrax::GrantEditToMembersJob auxiliary 120
Dev Hyrax::GrantEditToMembersJob ingest 10
SDAPI IiifPrint::Jobs::CreateRelationshipsJob default 90
Dev IiifPrint::Jobs::CreateRelationshipsJob default -20
SDAPI IngestJob auxiliary 120
Dev IngestJob ingest 10
SDAPI InheritPermissionsJob default 110
Dev InheritPermissionsJob default 0
SDAPI StreamNotificationsJob default 110
Dev StreamNotificationsJob default 0

@jeremyf jeremyf self-requested a review February 23, 2024 19:03
@bkiahstroud bkiahstroud merged commit a8b8c5d into main Feb 23, 2024
7 of 8 checks passed
@bkiahstroud bkiahstroud deleted the i718-separate-fast-lane-import-queue-and-worker branch February 23, 2024 19:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants