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

RedisSentinelBackend doesn't work with "db" number other than 0 #201

Open
macrenc opened this issue Apr 9, 2021 · 6 comments
Open

RedisSentinelBackend doesn't work with "db" number other than 0 #201

macrenc opened this issue Apr 9, 2021 · 6 comments

Comments

@macrenc
Copy link

macrenc commented Apr 9, 2021

Hello,

we are trying to setup RedisSentinelBackend with a non-default db parameter to specify the database of the Redis server where the cache keys should be stored.

However when specifying db parameter other than 0, following error is raised by the Redis client:

redis.exceptions.ResponseError: unknown command `SELECT`, with args beginning with: `2`

Here is our configuration in a simple local setup (we use YAML format, however it shouldn't really matter here):

  dogpile_cache:
    regions: short_term,jwt_duration,oauth_access_token_duration,hourly
    backend: dogpile.cache.redis_sentinel
    arguments:
      sentinels:
        - [127.0.0.1, 26379]
      db: 2
      distributed_lock: True
      thread_local_lock: False
      service_name: mymaster

Of course everything works fine when parameter db is removed or set to 0.

I presume what is happening is that Redis client is told to use command SELECT 2 when working with a Sentinel node, while this command is not known by Sentinel.

I can reproduce the issue in redis-cli when connected to a Sentinel node:

127.0.0.1:26379> select 2
(error) ERR unknown command `select`, with args beginning with: `2`, 

This seems to be an issue in RedisSentinelBackend of dogpile.cache, because when using e.g. Celery, we are able to specify the db numbers with Sentinel connection strings and keys are then properly created in the db of our choice.

I was also wondering if there could be some workaround for this issue, e.g. using connection_kwargs parameter? I tried passing the db argument to it, however it did not work.

Any help is greatly appreciated, because in our current setup we can't store the cache keys in db 0 of Redis, which currently blocks us from switching to Redis Sentinel setup.

@zzzeek
Copy link
Member

zzzeek commented Apr 9, 2021

hi there -

unfortunately I didn't write this backend and I know very little about it. dogpile doesn't emit those statements so perhaps this issue is actually in the redis.sentinel library? i dont know anything about how Celery interacts with redis.

the backend here is quite simple so feel free to send a PR if you know what it's not doing correctly: https://github.com/sqlalchemy/dogpile.cache/blob/master/dogpile/cache/backends/redis.py#L273

@zzzeek
Copy link
Member

zzzeek commented Apr 9, 2021

CC @sbrunner , the creator of this backend

@macrenc
Copy link
Author

macrenc commented Apr 9, 2021

@zzzeek Thanks a lot for the quick reply.

In this case I will definitely look deeper into this issue next week and at the backend implementation you shared.

Wish you a great weekend!

@jvanasco
Copy link
Member

jvanasco commented Apr 9, 2021

I was also wondering if there could be some workaround for this issue, e.g. using connection_kwargs parameter? I tried passing the db argument to it, however it did not work.

Looking at the code, I THINK you may be able to do a workaround by sending in a fully qualified url kwarg that has the host/port/database

@sbrunner
Copy link
Contributor

Effectively the db should always be 0, he is used to internal communication.
To change the used db you should set connection_kwargs to{'db': <db>}...

@macrenc
Copy link
Author

macrenc commented Apr 12, 2021

Thanks @sbrunner , this works nicely! I've tried that before but initially it didn't work due to how other library (pyramid_dogpile_cache2) is parsing the settings. It seems they do not really support connection_kwargs, so I will have to work around this now but it's no longer an issue in dogpile.cache.

One thing I would suggest is to extend the documentation for the sentinel backend to mention that db should always be set to 0 in this scenario. It would somehow also make sense in my mind to ignore the db parameter in the sentinel backend, since any value other than 0 will lead to a crash. I may find some time later to send a PR if you don't mind.

Thanks again everybody, I think now I will be able to work around this issue somehow.

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

4 participants