Skip to content
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

fix(#4065): make low_prio queue for caching #4200

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

elasticspoon
Copy link
Collaborator

@elasticspoon elasticspoon commented Mar 17, 2024

Resolves #4065

Description

Adds a "low_priority" delayed job queue. It will currently only have DataCache jobs.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Job specs were edited to make sure the job is landing the right queue.

Note:

I could not for the life of me figure out how to test that the priority was working.

Based on the delayed job code I believe priority is set when the job is queued, that is when it is written to the database table "delayed_job_queue".

https://github.com/collectiveidea/delayed_job/blob/b66bb64437a8606a414a390531ac73108911434e/lib/delayed/backend/job_preparer.rb#L13

ActiveJob::TestHelper override the queue_adapter from delayed_job to the ActiveJob adapter. This means that priority never gets set from the queue in tests. So if we test: assert_enqueued_with(priority: some_value) it will always fail. We would have to explicitly set a priority, instead of using the queue priority which doesn't really make sense.

Just to sanity check the behavior I ran the following code in rails c:

5.times { |i|  HistoricalDataCacheJob.perform_later(org_id: Organization.last.id, type: i) }
NotifyPartnerJob.perform_later(Request.last.id)

the DataCache Jobs were correctly set with the low prio:

  Delayed::Backend::ActiveRecord::Job Create (0.6ms)  INSERT INTO "delayed_jobs" ("priority", "attempts", "handler", "last_error", "run_at", "locked_at", "failed_at", "locked_by", "queue", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) RETURNING "id"  [["priority", 10], ["attempts", 0], ["handler", "--- !ruby/object:ActiveJob::QueueAdapters::DelayedJobAdapter::JobWrapper\njob_data:\n  job_class: HistoricalDataCacheJob\n  job_id: cbc1f289-70c5-480c-9dbf-b9a48ec269c2\n  provider_job_id:\n  queue_name: low_priority\n  priority:\n  arguments:\n  - org_id: 2\n    type: 0\n    _aj_ruby2_keywords:\n    - org_id\n    - type\n  executions: 0\n  exception_executions: {}\n  locale: en\n  timezone: UTC\n  enqueued_at: '2024-03-17T21:18:50Z'\n"], ["last_error", nil], ["run_at", "2024-03-17 21:18:50.659691"], ["locked_at", nil], ["failed_at", nil], ["locked_by", nil], ["queue", "low_priority"], ["created_at", "2024-03-17 21:18:50.659754"], ["updated_at", "2024-03-17 21:18:50.659754"]]
  TRANSACTION (2.2ms)  COMMIT

NotifyPartnerJob correctly fired off before the caching jobs despite being queued later:

16:55:06 worker.1 | [Worker(host:laptop pid:1071210)] Job NotifyPartnerJob [feac18aa-5d43-464a-9ff4-9ab1a238a027] from DelayedJob(default) with arguments: [200] (id=37) (queue=default) RUNNING
16:55:09 worker.1 | 2024-03-17T16:55:09-0400: [Worker(host:laptop pid:1071210)] Job HistoricalDataCacheJob [38641404-f54d-4b02-8b64-749271c6a6e7] from DelayedJob(low_priority) with arguments: [{"org_id"=>2, "type"=>"0", "_aj_ruby2_keywords"=>["org_id", "type"]}] (id=32) (queue=low_priority) RUNNING

@elasticspoon elasticspoon marked this pull request as ready for review March 17, 2024 21:35
Right now we have a single default queue for Delayed Worker.
This queue is shared by both HistoricalDataCacheJob and other jobs.
We should separate HistoricalDataCacheJob at least into a
separate queue so that if it gets backed we still send emails.

This implements a single low_priority queue that only
the historical data cache is in, and then all the rest
can remain at a higher-priority in default.

Note: lower prio number => higher prio.

Also priority cannot be explicitly tested in job specs,
the test queue adapter intercepts the job before
delayed_job sets the priority from the queue.

It does work tho, I promise :)
@elasticspoon elasticspoon changed the title fix: make low_prio queue for caching fix(#4065): make low_prio queue for caching Mar 17, 2024
dorner
dorner previously requested changes Mar 18, 2024
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

I'd just fix the comment, and then I think this is good to merge - maybe @awwaiid wants to take a look.

Not !?!?!?!? worthy
Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@awwaiid awwaiid requested a review from dorner March 26, 2024 20:15
@awwaiid awwaiid dismissed dorner’s stale review March 26, 2024 20:15

Change was applied!

@awwaiid awwaiid merged commit 01f8880 into rubyforgood:main Mar 26, 2024
19 checks passed
@elasticspoon elasticspoon deleted the 4065-add-queues branch April 13, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There should be separate Delayed Worker queues based on priority / importance
3 participants