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

Sentinel timout not exposed #235

Open
meseta opened this issue Apr 5, 2021 · 2 comments
Open

Sentinel timout not exposed #235

meseta opened this issue Apr 5, 2021 · 2 comments

Comments

@meseta
Copy link

meseta commented Apr 5, 2021

Sentinel timeout argument is not exposed. This setting we need to be able to adjust because the default aioredis 1.3.1 timeout value is 0.2s, too short for a lot of use-cases

The relevant code is here.

arq/arq/connections.py

Lines 216 to 236 in 88afd61

if settings.sentinel:
addr: Any = settings.host
async def pool_factory(*args: Any, **kwargs: Any) -> Redis:
client = await aioredis.sentinel.create_sentinel_pool(*args, ssl=settings.ssl, **kwargs)
return client.master_for(settings.sentinel_master)
else:
pool_factory = functools.partial(
aioredis.create_pool, create_connection_timeout=settings.conn_timeout, ssl=settings.ssl
)
addr = settings.host, settings.port
try:
pool = await pool_factory(addr, db=settings.database, password=settings.password, encoding='utf8')
pool = ArqRedis(
pool,
job_serializer=job_serializer,
job_deserializer=job_deserializer,
default_queue_name=default_queue_name,
)

I think settings.conn_timeout could be passed to aioredis.sentinel.create_sentinel_pool for the timeout argument. Internally, aioredis 1.3.1 passes this value onto the pools that it creates as well, making it work similarly to settings.conn_timeout when not using sentinel. We're currently working around this issue by monkey-patching the default argument in aioredis prior to any arq code.

In the future aioredis 2.0.0, it seems that sentinels can be instantiated with a socket_timeout argument. It would be worth exposing this to RedisSettings in arq to help with clusters that have higher latency. It may be best to wait for aioredis 2.0.0 before addressing this issue

@samuelcolvin
Copy link
Owner

PR welcome to fix this.

@meseta
Copy link
Author

meseta commented Apr 25, 2021

happy to help if we're still using arq in a couple months (nothing wrong with it, we're just switching a lot of stuff to kafka). I suggest waiting for aioredis 2.0.0, which releases imminently (see: aio-libs-abandoned/aioredis-py#930 (comment)), which seems to have substantially changed how sentinel is handled. Happy to take a look if we're still using arq at that point

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

No branches or pull requests

2 participants