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

Add Redis Streams option for job delivery #451

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ajac-zero
Copy link

This pull request adds a basic implementation of Redis Streams, in order to avoid polling for new jobs in the worker and reduce latency, in accordance with objective 4 of issue #437.

To create a worker that listens to a Redis Stream, we can use the cli or specify it in the code directly.

CLI:
arq worker.WorkerSettings --stream

Code:

class WorkerSettings:
    functions = [...]
    stream = True
    ...

On the client, they must specify that they want to deliver a job to a worker through a Redis Stream.

redis = await create_pool(RedisSettings())
await redis.enqueue_job('hello_world', _use_stream=True)

Here are the results of a very simple benchmark that showcases the potential of using Redis Streams for improved latency.

Polling:
Captura de pantalla 2024-05-04 a la(s) 8 12 28 a m
Average time: 0.268s

Streaming:
Captura de pantalla 2024-05-04 a la(s) 8 15 34 a m
Average time: 0.012s

Copy link

codecov bot commented May 4, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 95.00%. Comparing base (94cd878) to head (1b44875).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
- Coverage   96.27%   95.00%   -1.28%     
==========================================
  Files          11       11              
  Lines        1074     1100      +26     
  Branches      209      197      -12     
==========================================
+ Hits         1034     1045      +11     
- Misses         19       31      +12     
- Partials       21       24       +3     
Files Coverage Δ
arq/constants.py 100.00% <100.00%> (ø)
arq/cli.py 96.49% <60.00%> (-3.51%) ⬇️
arq/connections.py 88.81% <33.33%> (-1.25%) ⬇️
arq/worker.py 94.88% <43.47%> (-2.29%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1315583...1b44875. Read the comment docs.

@gaby
Copy link

gaby commented May 7, 2024

@ajac-zero This may need a unit-test with stream enabled.

@ajac-zero
Copy link
Author

Sure thing @gaby . I was wondering how I should go about that...

What I started doing was add a stream parameter to the worker tests, and then wrap them so they run twice, once with stream and once without.

But I feel this might not be the best way to do things, maybe I should focus on some vital tests? What do you suggest?

@gaby
Copy link

gaby commented May 15, 2024

@ajac-zero That's probably a great starting point, running current tests with "stream" set to false. Then running the test suite with "stream" set to True. This will require setting the source of the data to use Streams.

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.

None yet

2 participants