endlessm / azafea

Service to track device activations and usage metrics
Mozilla Public License 2.0
10 stars 2 forks source link

Don’t use Redis blocking functions #140

Closed liZe closed 3 years ago

liZe commented 3 years ago

The previous code was always triggering a long timeout, even when executed with the exit_on_empty_queues option.

Using sleep() instead of blocking functions gives code that’s more simple, and that avoids this extra timeout during the first loop.

Another change required by non-blocking functions is that we now use a loop for different queues. Even if the loop adds 3 extra (trivial) lines, it also simplifies code, especially for moking Redis.

This is really useful for tests, as most of them are obviously launched with the exit_on_empty_queues option set: it saves 5 seconds for each test relying on Redis queues.

There’s also another branch with a dirty but more simple fix. This PR is probably better.

@wjt This PR looks nice, but I’m afraid to break something just to save 3 minutes for tests. @adarnimrod proposed to try this on a dev server, to check that load and queues are OK. We could also ask Redis and Python gurus to review, if we’re not sure.

liZe commented 3 years ago

So the cost of a faster test suite is latency in the production service.

It’s true, we get an extra latency when the queue is empty. But that’s only latency, not speed.

The other branch doesn’t have this problem, but we don’t get the simplification we have using rpop instead of brpop.

A clean (but more complicated) solution would be to use rpop when launched once, and brpop when launched forever. I’m not sure that it’s worth it.

I can do what you prefer!

liZe commented 3 years ago

After discussing this issue yesterday, I finally think that the best solution is just to use a shorter timing when we want to exit on empty queues. No change in the logic, no special case for tests, and code that’s easy to understand.

I’ve uploaded a third branch that’s close to the first one, but more readable.

@wjt @adarnimrod If it’s OK for you, I can close this PR and just push this trivial commit to master (or open another PR).

wjt commented 3 years ago

Fine by me.

liZe commented 3 years ago

Replaced by #141.