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

Cluster mode support #420

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Cluster mode support #420

wants to merge 22 commits into from

Conversation

jeanluciano
Copy link

Adds limited support for redis cluster mode. This was done by overwriting a few methods when RedisSettings has the new cluster_mode flag on.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall seems like a reasonable feature, but there's a fair bit to clear up here.

Should we wait to add cluster support until after #437?

options: --entrypoint redis-server

steps:
- uses: actions/checkout@v2
- name: Test redis cluster
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add cluster tests separately from the main tests as redis-cluster-test.


from .constants import default_queue_name, expires_extra_ms, job_key_prefix, result_key_prefix
from .jobs import Deserializer, Job, JobDef, JobResult, Serializer, deserialize_job, serialize_job
from .utils import timestamp_ms, to_ms, to_unix_ms

logger = logging.getLogger('arq.connections')
logging.basicConfig(level=logging.DEBUG)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logging.basicConfig(level=logging.DEBUG)

@@ -27,7 +33,7 @@ class RedisSettings:
Used by :func:`arq.connections.create_pool` and :class:`arq.worker.Worker`.
"""

host: Union[str, List[Tuple[str, int]]] = 'localhost'
host: Union[str, List[Tuple[str, int]]] = 'test-cluster.aqtke6.clustercfg.use2.cache.amazonaws.com'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
host: Union[str, List[Tuple[str, int]]] = 'test-cluster.aqtke6.clustercfg.use2.cache.amazonaws.com'
host: Union[str, List[Tuple[str, int]]] = 'localhost'

Comment on lines +177 to +179
the_job = Job(job_id, redis=self, _queue_name=_queue_name, _deserializer=self.job_deserializer)
logger.debug(the_job)
return the_job
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert.

class ArqRedisCluster(RedisCluster): # type: ignore
"""
Thin subclass of ``from redis.asyncio.cluster.RedisCluster`` which patches methods of RedisClusterPipeline
to support redis cluster`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to support redis cluster`.
to support redis cluster.

@@ -299,5 +391,5 @@ async def log_redis_info(redis: 'Redis[bytes]', log_func: Callable[[str], Any])
f'redis_version={redis_version} '
f'mem_usage={mem_usage} '
f'clients_connected={clients_connected} '
f'db_keys={key_count}'
f'db_keys={88}'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

@@ -59,7 +59,7 @@ Changelog = 'https://github.com/samuelcolvin/arq/releases'
testpaths = 'tests'
filterwarnings = ['error']
asyncio_mode = 'auto'
timeout = 10

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this file?

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

3 participants