vutran1710 / PyrateLimiter

⚔️Python Rate-Limiter using Leaky-Bucket Algorithm Family
https://pyratelimiter.readthedocs.io
MIT License
349 stars 36 forks source link

FEATURE REQUEST: Support for Redis Clusters #46

Closed navyamehta closed 3 years ago

navyamehta commented 3 years ago

Hey all! I noticed that PyRateLimiter currently has support for Redis via RedisBucket. I had a feature request for extending this support to managed redis clusters (say AWS ElasticCache) which is likely gonna be the standard in many production systems (since a single instance risks availability). The support for clusters could be added with redis-py-cluster dependency. We could either define a RedisClusterBucket as:

from rediscluster import RedisCluster
from pyrate_limiter import RedisBucket

#Define an extension of the rate limiter RedisBucket for Clustered Redis
class RedisClusterBucket(RedisBucket):
    """A bucket with Redis using List"""

    def get_connection(self):
        """Obtain a connection from redis pool"""
        return RedisCluster(connection_pool=self._pool)

or we can add a boolean flag (clustered or not) in the existing RedisBucket implementation that changes the behavior of get_connection:

def get_connection(self):
    if self.is_clustered:
        return RedisCluster(...)
     else:
         return Redis(...)

Would love to know your thoughts!

vutran1710 commented 3 years ago

Look feasible to me. Ill see what I can do.

vutran1710 commented 3 years ago

I took a look at redis-py-cluster and it appears to me that the lib shares the same apis with redis-py. That means you can start using PyrateLimiter with redis-py-cluster instantly without adding more code to PyrateLimiter itself. I mean, the snippet you provided above should work right out of the box right?

from rediscluster import RedisCluster
from pyrate_limiter import RedisBucket

#Define an extension of the rate limiter RedisBucket for Clustered Redis
class RedisClusterBucket(RedisBucket):
    """A bucket with Redis using List"""

    def get_connection(self):
        """Obtain a connection from redis pool"""
        return RedisCluster(connection_pool=self._pool)
navyamehta commented 3 years ago

Yupp so right now, clustered redis uses the same API as normal redis but uses a ClusterConnectionPool instead of a ConnectionPool. Passing a ClusterConnectionPool to Redis(...) doesnt work - it must be passed to a RedisCluster(...) instance. The snippet I provided does work out of the box (which is what I'm currently using), but I thought it would be nice if direct support for clusters was built into the buckets available in the package itself (i.e. the snippet was available in the package to begin with?). Thanks!

vutran1710 commented 3 years ago

added in https://github.com/vutran1710/PyrateLimiter/pull/47