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

Transition basket tasks to use rq #1069

Merged
merged 1 commit into from
Jun 27, 2023
Merged

Transition basket tasks to use rq #1069

merged 1 commit into from
Jun 27, 2023

Conversation

robhudson
Copy link
Contributor

@robhudson robhudson commented Jun 1, 2023

Still some work to be done here...

  • Fully test tasks locally
  • Update documentation (if needed)
  • Add snitch management command
  • Add snitch k8s [CronJob](https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/)
  • Make derived settings from REDIS_URL also Django settings, rather than env variables.

@robhudson robhudson force-pushed the rq-queues branch 3 times, most recently from 084fe06 to 94b2371 Compare June 2, 2023 00:25
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #1069 (60654df) into main (4a30aa0) will increase coverage by 1.11%.
The diff coverage is 94.21%.

@@            Coverage Diff             @@
##             main    #1069      +/-   ##
==========================================
+ Coverage   84.18%   85.29%   +1.11%     
==========================================
  Files          26       26              
  Lines        2491     2231     -260     
==========================================
- Hits         2097     1903     -194     
+ Misses        394      328      -66     
Impacted Files Coverage Δ
...s/management/commands/process_maintenance_queue.py 0.00% <0.00%> (ø)
basket/news/urls.py 100.00% <ø> (ø)
basket/news/admin.py 75.75% <33.33%> (+0.24%) ⬆️
basket/news/views.py 84.06% <50.00%> (+0.08%) ⬆️
basket/news/models.py 89.03% <60.00%> (-2.85%) ⬇️
basket/news/tasks.py 76.06% <90.47%> (+2.11%) ⬆️
basket/base/rq.py 99.00% <99.00%> (ø)
basket/base/decorators.py 100.00% <100.00%> (ø)
basket/base/exceptions.py 100.00% <100.00%> (ø)
basket/base/tasks.py 100.00% <100.00%> (ø)
... and 2 more

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@stevejalim stevejalim left a comment

Choose a reason for hiding this comment

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

Looking good to me. Some questions but curiosity rather than blockers

basket/base/decorators.py Outdated Show resolved Hide resolved
basket/base/management/commands/rqworker.py Show resolved Hide resolved
basket/base/rq.py Show resolved Hide resolved
basket/base/rq.py Show resolved Hide resolved

if job.is_failed:
statsd.incr(f"{task_name}.retry_max")
statsd.incr("news.tasks.retry_max_total")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably being a bit dim, but am failing to visualise what the resulting data looks like for this situation - how does retry_max_total fit with failed jobs -- I get that one MAX_RETRIES is hit the job fails, so is this stat tracking the number of times that exhaustion happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we'd get a count of how many times a specific task hits the max retries, as well as how many times all jobs hit the max retries.


if ignore_error(exc_info[1]):
with sentry_sdk.push_scope() as scope:
scope.set_tag("action", "ignored")
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this use of tagging


assert job.is_failed

# TODO: Determine why the `store_task_exception_handler` is not called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a blocker or OK as a known-unknown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added more tests so there's 100% coverage, it's just broken into separate tests. But I'd like to solve this riddle at some point. So I don't consider it a blocker, just a hard thing to test.

@robhudson robhudson force-pushed the rq-queues branch 5 times, most recently from 5bd0ea3 to d748b36 Compare June 9, 2023 21:53
basket/base/rq.py Outdated Show resolved Hide resolved


def rq_on_success(job, connection, result, *args, **kwargs):
# Don't fire statsd metrics in maintenance mode.
Copy link
Member

Choose a reason for hiding this comment

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

why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In maintenance mode we're going to queue the task in the db rather than do the task, so we'd get timings for doing the db insert rather than timings for the task, which would confuse the timings. And a "success" for the db insert isn't the same as a "success" for the task.

basket/settings.py Outdated Show resolved Hide resolved
bin/run-worker.sh Outdated Show resolved Hide resolved
Copy link
Member

@pmac pmac left a comment

Choose a reason for hiding this comment

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

This is looking fantastic so far! Can't wait to get it landed. I think it's nearly there.

@robhudson robhudson force-pushed the rq-queues branch 6 times, most recently from 344f160 to f7276ad Compare June 20, 2023 22:13
@robhudson robhudson force-pushed the rq-queues branch 2 times, most recently from 5afe1e1 to bf373e5 Compare June 26, 2023 15:50
@robhudson robhudson merged commit 5ca1054 into main Jun 27, 2023
4 checks passed
@robhudson robhudson deleted the rq-queues branch June 27, 2023 17:05
leplatrem added a commit to mozilla-it/ctms-api that referenced this pull request Jul 6, 2023
In mozmeao/basket#1069 Basket tasks were transitionned to rq

This pins Basket to its previous version until we figure out how to migrate our integration test setup.
leplatrem added a commit to mozilla-it/ctms-api that referenced this pull request Jul 6, 2023
In mozmeao/basket#1069 Basket tasks were transitionned to rq

This pins Basket to its previous version until we figure out how to migrate our integration test setup.
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.

3 participants