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 posibility to use RedisCluster as connection Class #2030
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2030 +/- ##
==========================================
- Coverage 93.61% 90.82% -2.79%
==========================================
Files 28 30 +2
Lines 3758 3978 +220
==========================================
+ Hits 3518 3613 +95
- Misses 240 365 +125 ☔ View full report in Codecov by Sentry. |
rq/worker.py
Outdated
timeout_config = {"socket_timeout": self.connection_timeout} | ||
connection.connection_pool.connection_kwargs.update(timeout_config) | ||
return connection | ||
if connection.__class__.__name__ == 'Redis': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind changing this approach of explicit class name check? This runs afoul of Python's duck typing concept (e.g it wouldn't work with FakeRedis).
I'd prefer using try/except to try and detect the type of connection the worker has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. No problem. With try excrpt will be work well.
Is it possible to setup a Redis cluster to test this in CI? |
current_socket_timeout = connection.connection_pool.connection_kwargs.get("socket_timeout") | ||
if current_socket_timeout is None: | ||
timeout_config = {"socket_timeout": self.connection_timeout} | ||
connection.connection_pool.connection_kwargs.update(timeout_config) | ||
return connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd also need to test this so test coverage doesn't go down. An easy way to do this would be to move this logic to a separate setup_connection()
and pass both Redis
and RedisCluster
instances into it so we can easily test this.
Yes, it is possible. Bast way to do this is use docker compose file. Here it : https://github.com/bitnami/containers/blob/main/bitnami%2Fredis-cluster%2Fdocker-compose.yml |
Ok great. Do you mind adding Redis cluster to the test matrix then? I think we can just test it with the latest Python and redis-py version. |
ebbd6bb
to
bab0061
Compare
I add yours previous commnets. I reopen pull request. |
Hi @selwin |
Hi @selwin sorry for pushing you, |
@MykolaBordakov I don't think need separate tests for Redis Cluster. The important thing is making sure that existing tests run on Redis cluster. So what you need to figure out is how to spin up Redis Cluster when CI runs. |
Did I understand your test pipeline correctly?
If we needs to test it with RedisCluster i needs to create a same thing. But, redis cluster connection didn`t set up in such way. |
Hi @selwin
If we needs to test it with RedisCluster i needs to create a same thing. But, redis cluster connection didn`t set up in such way. |
test part still missed
Hi @selwin |
I gave this branch a try. While it seems to solve the original problem, the worker throws these exceptions:
|
Hi @andreasciamanna |
I didn't mention any tests. I'm using this branch in my project and noticed that issue. The worker seems to work anyway, but the exceptions are flooding the logs, and I wonder if they may cause any problems. |
What recis lib version are you using?? Can you try redis lib version 5.0.1?? |
I was using 5.0.0. I've tried 5.0.1 but got the same results. Full error, in case the previous wasn't enough:
|
Thanks, full log is nice. One question, does job works well?? Did all you tasks have been done?? |
Yes. Everything works fine, so far. |
@MykolaBordakov have you tried your branch with a scheduled job? e.g. It seems like if you run the worker with the scheduler (in my case, from the script itself, using
I wouldn't expect the project to require duplication of fixes in the codebase, but it seems to be the case here. |
This issue was fixed by my branch. If you want to use it with scheduler, you needs to replace original rq file after PIP installation of RQ lib. Also, when you install rq scheduler, rq will be installed to from current latest release. When we will merge this branch, you will not do files replacement. Also, i will try to find fix yours previous errors. Have a nice day. |
Here is example how to find path to yours rq lib.
|
I don't understand your last two comments. I've installed rq with I'm supposed to be using your version of
The scheduler is part of Could you please let me know if you tested the scheduler? |
I use rq-schedular, it is a other lib. Here link: |
Is there any advantage in using a separate library when Even if there is, the scheduler in |
|
|
I would be happy to do so, but there are a couple of blockers:
There is a lack of information on how to set the test environment, or maybe it's more of a lack of knowledge from my side. Some more leads from @selwin or anyone else would help. |
Like I said, I have never personally used Redis cluster, though I'm not against adding Redis cluster support to RQ so I will accept contributions in this area. That said, this PR needs to show that it actually works on Redis cluster setup, meaning RQ's CI setup (Github Actions) needs to be updated to run the relevant test suites against a Redis cluster. I have no knowledge on how to set this up, so contributions are welcome. |
Hi. Here my suggest about fixing this issue: #2021
There i add some implementation about using RedisCluster connection type.