1051826 - double-check tidings task ratelimits

Status: RESOLVED FIXED

(support.mozilla.org :: Code Quality, task, P1)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

There's a tidings task that seems to spawn more often than 1/m, but it has a ratelimit of 1/m which means we're piling these tasks up.

This bug covers:

1. figuring out what is creating those tasks and making that match the ratelimit

2. (or) ditching the task altogether and turning it into a cron job that runs every 10 minutes or something like that

Comment 1

[Ricky Rosario [:rrosario, :r1cky]]

Adding to next sprint too. Guessing at 2pts.

Comment 2

[Ricky Rosario [:rrosario, :r1cky]]

What about removing the ratelimit altogether? I assume this was about not hitting the mail server too often, but they should be able to handle us. They are web scale.

Comment 3

[Ricky Rosario [:rrosario, :r1cky]]

reducing to 1pt per IRC

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

Option 2 was to ditch the task altogether and use a cron job instead. That wouldn't require a ratelimit. I haven't looked at any of the code, but it seems to me like that's the better architectural option for this anyhow.

Comment 5

[Ricky Rosario [:rrosario, :r1cky]]

The claim_watches task is called every time a new user account is activated. It goes through all anonymous watches in tidings that match the user's email address and points them to the created user. The rate limit is protecting the database (I guesss? there is nothing else) but that query looks pretty simple. I think we should just remove the rate limit since accounts aren't really created that often anyway. A few hundred per day at worst. We shouldn't really hit this limit ever anyway unless the queues are clogged up. I am almost tempted to just leave it. Hmmm

Comment 6

[Will Kahn-Greene [:willkg] ET needinfo? me]

I'd change this from creating a task every time we activate a new user account to kicking off a cron job every 10 minutes that does the work.

Less celery = more sanity.

Comment 7

[Ricky Rosario [:rrosario, :r1cky]]

(In reply to Will Kahn-Greene [:willkg] from comment #6)
> I'd change this from creating a task every time we activate a new user
> account to kicking off a cron job every 10 minutes that does the work.

That's good but I'm not exactly sure how we would know which users we are doing this for without creating a another queue? Another option is simply to do that right in the request context and be done. I guess we can verify that the query isn't crazy slow.

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

Oh, whoops. I didn't read the code very well. My bad.

Yeah, now that I've re-read the code, I agree: either leave it as is or nix the ratelimit.

Sorry about that!

Comment 9

[Ricky Rosario [:rrosario, :r1cky]]

The rate limit was removed from the task in django-tidings:
https://github.com/erikrose/django-tidings/commit/60da8179bc3b61a3fc41b4487730bda674a4a8ee

The pull request to update django-tidings in kitsune is here:
https://github.com/mozilla/kitsune/pull/2078

Comment 10

[Ricky Rosario [:rrosario, :r1cky]]

Landed on master:
https://github.com/mozilla/kitsune/commit/b8f36d634cdc390d748e90e3ac940c78e620d64f

Comment 11

[Ricky Rosario [:rrosario, :r1cky]]

Deployed to prod.