Add posibility to use RedisCluster as connection Class #2030

MykolaBordakov · 2024-01-24T18:36:20Z

Hi. Here my suggest about fixing this issue: #2021
There i add some implementation about using RedisCluster connection type.

codecov · 2024-01-24T18:44:35Z

Codecov Report

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

Project coverage is 90.82%. Comparing base (34f83d6) to head (2b92254).
Report is 39 commits behind head on master.

❗ Current head 2b92254 differs from pull request most recent head f6480a8. Consider uploading reports for the commit f6480a8 to get more accurate results

Files	Patch %	Lines
rq/worker.py	42.85%	8 Missing ⚠️
rq/queue.py	20.00%	4 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

selwin · 2024-01-27T10:49:20Z

rq/worker.py

- timeout_config = {"socket_timeout": self.connection_timeout}
- connection.connection_pool.connection_kwargs.update(timeout_config)
- return connection
+ if connection.__class__.__name__ == 'Redis':


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.

Hi. No problem. With try excrpt will be work well.

selwin · 2024-01-27T10:50:43Z

Is it possible to setup a Redis cluster to test this in CI?

selwin · 2024-01-27T10:55:50Z

rq/worker.py

+ 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


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.

MykolaBordakov · 2024-01-27T11:52:26Z

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
I used it for my tests. Code and cluster works well.

selwin · 2024-01-27T13:42:47Z

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.

MykolaBordakov · 2024-03-19T11:03:22Z

I add yours previous commnets. I reopen pull request.

MykolaBordakov · 2024-03-20T13:01:25Z

Hi @selwin
I tried to add redis cluster to test. Can you explain to me, how you create connection by this function : find_empty_redis_database() (init.py from tests folder)? Does you have some real redis DB during test pipeline ?? Can we do same thing for redis cluster ??

MykolaBordakov · 2024-03-21T19:29:47Z

Hi @selwin sorry for pushing you,
Is any information about how to organize redis cluster tests ?

selwin · 2024-03-21T23:59:41Z

@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.

MykolaBordakov · 2024-03-25T13:10:56Z

Did I understand your test pipeline correctly?
You had some code, that creates test Redis connection:

def find_empty_redis_database(ssl=False):
    """Tries to connect to a random Redis database (starting from 4), and
    will use/connect it when no keys are in there.
    """
    for dbnum in range(4, 17):
        connection_kwargs = {'db': dbnum}
        if ssl:
            connection_kwargs['port'] = 9736
            connection_kwargs['ssl'] = True
            connection_kwargs['ssl_cert_reqs'] = None  # disable certificate validation
        testconn = Redis(**connection_kwargs)
        empty = testconn.dbsize() == 0
        if empty:
            return testconn
    assert False, 'No empty Redis database found to run tests in.'

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.
Or, maybe, we does not do any additional tests. And run yours pipeline without redis cluster connection set up?

MykolaBordakov · 2024-04-03T08:13:20Z

Hi @selwin
Did I understand your test pipeline correctly?
You had some code, that creates test Redis connection:

def find_empty_redis_database(ssl=False):
    """Tries to connect to a random Redis database (starting from 4), and
    will use/connect it when no keys are in there.
    """
    for dbnum in range(4, 17):
        connection_kwargs = {'db': dbnum}
        if ssl:
            connection_kwargs['port'] = 9736
            connection_kwargs['ssl'] = True
            connection_kwargs['ssl_cert_reqs'] = None  # disable certificate validation
        testconn = Redis(**connection_kwargs)
        empty = testconn.dbsize() == 0
        if empty:
            return testconn
    assert False, 'No empty Redis database found to run tests in.'

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.
Or, maybe, we does not do any additional tests. And run yours pipeline without redis cluster connection set up?

test part still missed

MykolaBordakov · 2024-04-09T12:51:32Z

Hi @selwin
Can you help me with tests?? I don't quite understand how REDIS is currently being deployed for testing??

andreasciamanna · 2024-04-10T08:18:42Z

I gave this branch a try.

While it seems to solve the original problem, the worker throws these exceptions:

  File "(...)/.venv/lib/python3.12/site-packages/redis/cluster.py", line 2282, in watch
    raise RedisClusterException("method watch() is not implemented")
redis.exceptions.RedisClusterException: method watch() is not implemented

MykolaBordakov · 2024-04-10T09:38:23Z

Hi @andreasciamanna
What tests are doing ?? Or you find solution for adding tests in to CI??
Thank you.

andreasciamanna · 2024-04-10T09:48:38Z

Hi @andreasciamanna What tests are doing ?? Or you find solution for adding tests in to CI?? Thank you.

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.

MykolaBordakov · 2024-04-10T11:16:02Z

Hi @andreasciamanna What tests are doing ?? Or you find solution for adding tests in to CI?? Thank you.

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??

andreasciamanna · 2024-04-10T11:23:38Z

Hi @andreasciamanna What tests are doing ?? Or you find solution for adding tests in to CI?? Thank you.

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:

13:22:11 [Job 2be735ce-921e-4c31-8c52-7aaf8b5fa9fe]: exception raised while executing (apps.queue_worker.update_references)
Traceback (most recent call last):
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 1552, in perform_job
    self.handle_job_success(job=job, queue=queue, started_job_registry=started_job_registry)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 1485, in handle_job_success
    pipeline.watch(job.dependents_key)
  File "/(omitted)/.venv/lib/python3.12/site-packages/redis/cluster.py", line 2282, in watch
    raise RedisClusterException("method watch() is not implemented")
redis.exceptions.RedisClusterException: method watch() is not implemented

MykolaBordakov · 2024-04-10T11:30:10Z

Hi @andreasciamanna What tests are doing ?? Or you find solution for adding tests in to CI?? Thank you.

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:
13:22:11 [Job 2be735ce-921e-4c31-8c52-7aaf8b5fa9fe]: exception raised while executing (apps.queue_worker.update_references)
Traceback (most recent call last):
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 1552, in perform_job
    self.handle_job_success(job=job, queue=queue, started_job_registry=started_job_registry)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 1485, in handle_job_success
    pipeline.watch(job.dependents_key)
  File "/(omitted)/.venv/lib/python3.12/site-packages/redis/cluster.py", line 2282, in watch
    raise RedisClusterException("method watch() is not implemented")
redis.exceptions.RedisClusterException: method watch() is not implemented

Thanks, full log is nice. One question, does job works well?? Did all you tasks have been done??

andreasciamanna · 2024-04-10T12:16:50Z

Yes. Everything works fine, so far.

andreasciamanna · 2024-04-11T09:11:48Z

@MykolaBordakov have you tried your branch with a scheduled job? e.g. queue.enqueue_in( ... )

It seems like if you run the worker with the scheduler (in my case, from the script itself, using worker.work(with_scheduler=True)), the error shows up again:

Traceback (most recent call last):
  File "/(omitted)/apps/queue_worker.py", line 113, in <module>
    worker.work(with_scheduler=True)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 557, in work
    self._start_scheduler(burst, logging_level, date_format, log_format)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 806, in _start_scheduler
    self.scheduler = RQScheduler(
                     ^^^^^^^^^^^^
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/scheduler.py", line 53, in __init__
    self._connection_class, self._pool_class, self._pool_kwargs = parse_connection(connection)
                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/connections.py", line 12, in parse_connection
    connection_pool_kwargs = connection.connection_pool.connection_kwargs.copy()
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'RedisCluster' object has no attribute 'connection_pool'

I wouldn't expect the project to require duplication of fixes in the codebase, but it seems to be the case here.

MykolaBordakov · 2024-04-11T10:22:14Z

@MykolaBordakov have you tried your branch with a scheduled job? e.g. queue.enqueue_in( ... )

It seems like if you run the worker with the scheduler (in my case, from the script itels, using worker.work(with_scheduler=True)), the error shows up again:

Traceback (most recent call last):
  File "/(omitted)/apps/queue_worker.py", line 113, in <module>
    worker.work(with_scheduler=True)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 557, in work
    self._start_scheduler(burst, logging_level, date_format, log_format)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 806, in _start_scheduler
    self.scheduler = RQScheduler(
                     ^^^^^^^^^^^^
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/scheduler.py", line 53, in __init__
    self._connection_class, self._pool_class, self._pool_kwargs = parse_connection(connection)
                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/connections.py", line 12, in parse_connection
    connection_pool_kwargs = connection.connection_pool.connection_kwargs.copy()
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'RedisCluster' object has no attribute 'connection_pool'

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.

MykolaBordakov · 2024-04-11T10:24:14Z

@MykolaBordakov have you tried your branch with a scheduled job? e.g. queue.enqueue_in( ... )

It seems like if you run the worker with the scheduler (in my case, from the script itels, using worker.work(with_scheduler=True)), the error shows up again:

Traceback (most recent call last):
  File "/(omitted)/apps/queue_worker.py", line 113, in <module>
    worker.work(with_scheduler=True)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 557, in work
    self._start_scheduler(burst, logging_level, date_format, log_format)
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/worker.py", line 806, in _start_scheduler
    self.scheduler = RQScheduler(
                     ^^^^^^^^^^^^
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/scheduler.py", line 53, in __init__
    self._connection_class, self._pool_class, self._pool_kwargs = parse_connection(connection)
                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/(omitted)/.venv/lib/python3.12/site-packages/rq/connections.py", line 12, in parse_connection
    connection_pool_kwargs = connection.connection_pool.connection_kwargs.copy()
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'RedisCluster' object has no attribute 'connection_pool'

I wouldn't expect the project to require duplication of fixes in the codebase, but it seems to be the case here.

Here is example how to find path to yours rq lib.

import rq
print(rq.__file__)

andreasciamanna · 2024-04-11T10:45:54Z

I don't understand your last two comments.

I've installed rq with pip install git+https://github.com/MykolaBordakov/rq.git@support-redis-cluster.

I'm supposed to be using your version of rq.

Also, when you install rq scheduler,

The scheduler is part of rq and is not a separate package.

Could you please let me know if you tested the scheduler?

MykolaBordakov · 2024-04-11T11:44:14Z

I don't understand your last two comments.

I've installed rq with pip install git+https://github.com/MykolaBordakov/rq.git@support-redis-cluster.

I'm supposed to be using your version of rq.

Also, when you install rq scheduler,

The scheduler is part of rq and is not a separate package.

Could you please let me know if you tested the scheduler?

I use rq-schedular, it is a other lib. Here link:
https://pypi.org/project/rq-scheduler-ng/
There are rq lib in dependencies. If you install it, you automaticaly install rq from master branch.

andreasciamanna · 2024-04-11T11:47:58Z

Is there any advantage in using a separate library when rq already provides a scheduler?

Even if there is, the scheduler in rq would still be broken when using RedisCluster, meaning this PR shouldn't be merged until the problem is resolved, right?

MykolaBordakov · 2024-04-11T11:48:26Z

with_scheduler
Ou, sorry. I think i understood yours problem. I will check scheduler code.

MykolaBordakov · 2024-04-11T12:02:25Z

Is there any advantage in using a separate library when rq already provides a scheduler?

Even if there is, the scheduler in rq would still be broken when using RedisCluster, meaning this PR shouldn't be merged until the problem is resolved, right?
Yes, you are right. I need to fix scheduler before merge. Also, needs to create tests for my code to CI. Maybe, you can help me with tests?? :)

andreasciamanna · 2024-04-11T12:27:15Z

[...] needs to create tests for my code to CI. Maybe you can help me with tests?? :)

I would be happy to do so, but there are a couple of blockers:

I have zero experience with GitHub actions: I'm familiar with other CI environments (e.g. GitLab-CI), but GitHub actions is an entirely different approach I never had the need, time or will to learn.
I also don't have much experience with Python in general. It's a language I managed to avoid until recently, so I'm still learning and failing to grasp some of its specifics.

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.
In any case, that docker-compose file shared by @selwin does not help much and he has been in radio silence since his last comment three weeks ago.

Some more leads from @selwin or anyone else would help.

selwin · 2024-04-12T23:51:55Z

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.
In any case, that docker-compose file shared by @selwin does not help much and he has been in radio silence since his last comment three weeks ago.

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.

MykolaBordakov mentioned this pull request Jan 24, 2024

Use RedisCluster as connection argument for rq worker (redis 5.0.1, rq 1.15.1) #2021

Open

selwin reviewed Jan 27, 2024

View reviewed changes

MykolaBordakov closed this Mar 19, 2024

MykolaBordakov force-pushed the support-redis-cluster branch from ebbd6bb to bab0061 Compare March 19, 2024 08:46

MykolaBordakov and others added 2 commits March 19, 2024 12:25

Add posibility to use RedisCluster as connection Class

1c2ce99

add tests

99c0c63

MykolaBordakov reopened this Mar 19, 2024

yurnov and others added 12 commits March 19, 2024 11:16

reformat with black

55935a1

fix duplicated TestWorker

6a5d4b1

another attempt to fix linter

9042f45

attemmpt to fix tests

ca49aa5

remove a extra line that added by mistake

a2ba737

Change sybtax ib worker connection tests

6d2484b

Add RQTestCase

4d48811

Remuve #worker.connection_timeout = 10

8bb43d8

Updade RQTestCase with redis cluster connection

ecbc5fc

Updade RQTestCase with redis cluster connection

c518563

reformat with black

3ee4bfc

reformat with ruff

35d841d

Merge branch 'rq:master' into support-redis-cluster

a38242c

MykolaBordakov added 2 commits March 25, 2024 16:00

Remove broken tests.

a4dd141

Remove broken tests.

7b6ee06

yurnov added 2 commits April 8, 2024 23:12

reformatted with black

2b92254

linter: remove unused import of RedisCluster

f6480a8

test part still missed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add posibility to use RedisCluster as connection Class #2030

Add posibility to use RedisCluster as connection Class #2030

MykolaBordakov commented Jan 24, 2024

codecov bot commented Jan 24, 2024 •

edited

selwin Jan 27, 2024

MykolaBordakov Jan 27, 2024

selwin commented Jan 27, 2024

selwin Jan 27, 2024

MykolaBordakov commented Jan 27, 2024 •

edited

selwin commented Jan 27, 2024

MykolaBordakov commented Mar 19, 2024

MykolaBordakov commented Mar 20, 2024

MykolaBordakov commented Mar 21, 2024

selwin commented Mar 21, 2024

MykolaBordakov commented Mar 25, 2024

MykolaBordakov commented Apr 3, 2024

MykolaBordakov commented Apr 9, 2024

andreasciamanna commented Apr 10, 2024

MykolaBordakov commented Apr 10, 2024

andreasciamanna commented Apr 10, 2024

MykolaBordakov commented Apr 10, 2024

andreasciamanna commented Apr 10, 2024

MykolaBordakov commented Apr 10, 2024

andreasciamanna commented Apr 10, 2024

andreasciamanna commented Apr 11, 2024 •

edited

MykolaBordakov commented Apr 11, 2024

MykolaBordakov commented Apr 11, 2024

andreasciamanna commented Apr 11, 2024 •

edited

MykolaBordakov commented Apr 11, 2024

andreasciamanna commented Apr 11, 2024

MykolaBordakov commented Apr 11, 2024

MykolaBordakov commented Apr 11, 2024

andreasciamanna commented Apr 11, 2024

selwin commented Apr 12, 2024

Add posibility to use RedisCluster as connection Class #2030

Are you sure you want to change the base?

Add posibility to use RedisCluster as connection Class #2030

Conversation

MykolaBordakov commented Jan 24, 2024

codecov bot commented Jan 24, 2024 • edited

Codecov Report

selwin Jan 27, 2024

Choose a reason for hiding this comment

MykolaBordakov Jan 27, 2024

Choose a reason for hiding this comment

selwin commented Jan 27, 2024

selwin Jan 27, 2024

Choose a reason for hiding this comment

MykolaBordakov commented Jan 27, 2024 • edited

selwin commented Jan 27, 2024

MykolaBordakov commented Mar 19, 2024

MykolaBordakov commented Mar 20, 2024

MykolaBordakov commented Mar 21, 2024

selwin commented Mar 21, 2024

MykolaBordakov commented Mar 25, 2024

MykolaBordakov commented Apr 3, 2024

MykolaBordakov commented Apr 9, 2024

andreasciamanna commented Apr 10, 2024

MykolaBordakov commented Apr 10, 2024

andreasciamanna commented Apr 10, 2024

MykolaBordakov commented Apr 10, 2024

andreasciamanna commented Apr 10, 2024

MykolaBordakov commented Apr 10, 2024

andreasciamanna commented Apr 10, 2024

andreasciamanna commented Apr 11, 2024 • edited

MykolaBordakov commented Apr 11, 2024

MykolaBordakov commented Apr 11, 2024

andreasciamanna commented Apr 11, 2024 • edited

MykolaBordakov commented Apr 11, 2024

andreasciamanna commented Apr 11, 2024

MykolaBordakov commented Apr 11, 2024

MykolaBordakov commented Apr 11, 2024

andreasciamanna commented Apr 11, 2024

selwin commented Apr 12, 2024

codecov bot commented Jan 24, 2024 •

edited

MykolaBordakov commented Jan 27, 2024 •

edited

andreasciamanna commented Apr 11, 2024 •

edited

andreasciamanna commented Apr 11, 2024 •

edited