Closed mzealey closed 5 months ago
Yeah, your point is totally correct! Since I'm about to have some free time in the next few days, I'll try looking into it.
To my knowledge, you can setup your redis bucket with either connection pool or redis-sentinel supported like this
async def create_sentinel_redis_bucket(rates: List[Rate]):
from redis.sentinel import Sentinel
sentinel = Sentinel([('localhost', 26379)], socket_timeout=0.1)
master = sentinel.master_for('mymaster', socket_timeout=0.1)
bucket_key = f"test-bucket/{id_generator()}"
assert master.ping()
master.delete(bucket_key)
bucket = RedisBucket.init(rates, master, bucket_key)
assert bucket.count() == 0
return bucket
Or with connection pool, either sync or async
async def create_redis_bucket(rates: List[Rate]):
from redis import ConnectionPool
from redis import Redis
pool = ConnectionPool.from_url(getenv("REDIS", "redis://localhost:6379"))
redis_db = Redis(connection_pool=pool)
bucket_key = f"test-bucket/{id_generator()}"
redis_db.delete(bucket_key)
bucket = RedisBucket.init(rates, redis_db, bucket_key)
assert bucket.count() == 0
return bucket
async def create_async_redis_bucket(rates: List[Rate]):
from redis.asyncio import ConnectionPool as AsyncConnectionPool
from redis.asyncio import Redis as AsyncRedis
pool = AsyncConnectionPool.from_url(getenv("REDIS", "redis://localhost:6379"))
redis_db: AsyncRedis = AsyncRedis(connection_pool=pool)
bucket_key = f"test-bucket/{id_generator()}"
await redis_db.delete(bucket_key)
bucket = await RedisBucket.init(rates, redis_db, bucket_key)
assert await bucket.count() == 0
return bucket
Yes, however I believe above in your sentinel pool example it returns a connection to the master from the current sentinel configuration, however if that master goes down the connections will break and not automatically switch over to the new master.
Hence needing to call master_for() each time we go into a redis section to pull a (potentially new) master connection out of the pool
So if apparently redis' Sentinel class does not retry on master failover for us automatically, can we can create a supporting class for it and pass it to the RedisBucket class?
I think this might work
class SentinelRedis:
from redis.sentinel import Sentinel
"""A class that wraps around redis sentinel and automatically resolves the master node"""
def __init__(self, sentinel: Sentinel):
self.sentinel = sentinel
self.available_methods = [
"evalsha",
"zremrangebyscore",
"delete",
"zcard",
"zrange"
]
def __getattr__(self, name):
if name not in self.available_methods:
raise AttributeError
master = None
while master is None:
try:
found_master = self.sentinel.discover_master("mymaster")
master = found_master
except Exception as err:
logger.error("Error while discovering master: %s", err)
return getattr(master, name)
And then...
async def create_sentinel_redis_bucket(rates: List[Rate]) -> AbstractBucket:
from redis.sentinel import Sentinel
redis_db = SentinelRedis(Sentinel([("localhost", 26379)], socket_timeout=0.1))
bucket = RedisBucket.init(rates, redis_db, "test-bucket")
return bucket
OK reading through the code in redis 5 it looks like I can do something like:
sentinel = Sentinel(...)
master_pool = SentinelConnectionPool("service", sentinel, is_master=True, ...)
and pass that pool in and hopefully it auto-reconnects to the master as required.
I got this working. Probably add the snippet to the documentation if you believe the setup is correct
from redis.sentinel import Sentinel
from redis.sentinel import SentinelConnectionPool
from redis import Redis
sentinel = Sentinel([('localhost', 26379)], socket_timeout=0.1)
pool = SentinelConnectionPool("mymaster", sentinel, ssl=False, check_connection=True, is_master=True, decode_responses=True)
redis_db = Redis(connection_pool=pool)
bucket_key = f"test-sentinel/{id_generator()}"
bucket = RedisBucket.init(rates, redis_db, bucket_key)
OK We're still using limiter 2.5.0 (not 2.6.0 as I said in my initial ticket) so I think it's slightly different with what is required to go in. If you are doing the above then just .master_for() will work because it effectively does your pool/redis_db steps. But for 2.5.0 I believe the connection pool is passed in rather than the Redis
object itself.
Either way I tihnk this is resolved now thank you :)
Hi there, In 2.6.0 we are using redis connection pooling to handle ratelimiting. I see in 3.x this has been removed but the redis connection could easily go away in which case I think you cannot correctly handle retries?
Either way, in 2.6.0 there is also no obvious way to support Redis Sentinel for cluster management.
Perhaps a better redis interface would be for the caller to supply an (async)contextmanager reference which will return a redis connection to the redis rate limiter interface, and then it's down to the app to figure out retries/reconnects and whether they use connection pooling, clustering, sentinel etc?