Skip to content

Feat(Stream): Use redis stream #5

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

Merged
merged 1 commit into from
May 7, 2025
Merged

Conversation

saber-solooki
Copy link

@saber-solooki saber-solooki commented May 6, 2025

The whole PR got inspired from these two PR: here and here. Also I took a look at this library to get some idea.

First PR try to use redis stream but still using _pool_iteration which is somehow utilizing the old implementation without any benefit. Not sure what stream feature is actually happening there.
However, it has some nice ideas, like passing the flag to decide whether the user wants to use the stream or not.

On the other hand, the second PR has a more accurate usage of stream, but still has some weird ideas for implementation. On one hand, it assume the user only wants to use stream by removing the old implementation while enqueuing the job.

The second thing that I didn't like was registering 3 different task in the worker:

  1. The run_delayed_queue_poller always tries to read jobs from the simple queue and put it in the stream. If you take a close look, it has some logic, some places to keep putting the task in the queue, and again read it here and put it in the stream. STRANGE
  2. The run_stream_reader is the actual method to read tasks from the stream.
  3. The run_idle_consumer_cleanup also tries to remove idle consumers from the Redis. Imagine if we have multiple workers who always try to clean Redis. Strange again. I try to address removing the consumer in the close method like this part of the library. We need to clean up the consumer because we are generating a consumer name each time the worker is brought up. I can't imagine any easy way to consistently generate a unique name for our consumers.

Also I removed _unclaim_job from the implementation since from my understanding, the usage was to unclaim any job that consumer is trying to get while it is already taken by another consumer. Well, that is the whole point of using Redis stream to prevent such scenario and it shouldn't happen unless there is something wrong with Redis.

As a final note, I also added some improvements in some places to make the logic more robust, like if self.use_stream

@saber-solooki saber-solooki merged commit e3ce2be into main May 7, 2025
2 of 11 checks passed
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.

1 participant