WIP: Redis sentinel support

[gergelypolonkai]

Documented solution for #40

This is basically the original commit of @sh4nks (rebased onto the current master) with some documentation added. It still needs some testing and possibly some additions, but it seems to be a good start.

[coveralls]

Coverage decreased (-4.3%) to 63.992% when pulling a018d6d on gergelypolonkai:redis-sentinel into 34a84f7 on sh4nks:master.

[gergelypolonkai]

While live testing this, i found there are a lot of things missing; for example there is no way to authenticate with the sentinel instances. I’ll add this soon to this PR.

[sh4nks]

This is looking good already! Let me know when it's ready to be merged.

[gergelypolonkai]

It would be awesome if someone could give it a go (other than me, i mean). Other than that, it’s pretty much done except some tests maybe.

[sh4nks]

I'll try it out as I do have some spare time this afternoon :)

However, I am not exactly sure how to set up Redis-Sentinel - any tips?

[gergelypolonkai]

Last time i used this article to do it. For testing purposes it’s enough to set up one sentinel with two nodes. If you want to go for 100% (ie. production), you might need at least three sentinels, though.

[sh4nks]

I do have some fixes for your PR. Either you apply them or you could also set the PR to allow commits from the maintainers (https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork)

The patch in question btw:

From a23a652a6c28a6d07f45b5ac9143d4abd49519cc Mon Sep 17 00:00:00 2001
From: Peter Justin <peter.justin@outlook.com>
Date: Fri, 22 Feb 2019 12:17:30 +0100
Subject: [PATCH 1/3] Fix incorrect variable name for the read_client

---
 flask_caching/backends/clients.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/flask_caching/backends/clients.py b/flask_caching/backends/clients.py
index 069686b..9464460 100644
--- a/flask_caching/backends/clients.py
+++ b/flask_caching/backends/clients.py
@@ -44,7 +44,7 @@ class RedisSentinelCache(RedisCache):
             sentinel_kwargs=sentinel_kwargs, **kwargs)
 
         self._write_client = sentinel.master_for(master)
-        self._read_clients = sentinel.slave_for(master)
+        self._read_client = sentinel.slave_for(master)
 
         self.key_prefix = key_prefix or ''
 
-- 
2.20.1


From dbf7d52dec99591d1ca650a74b39354998f414fa Mon Sep 17 00:00:00 2001
From: Peter Justin <peter.justin@outlook.com>
Date: Fri, 22 Feb 2019 12:19:26 +0100
Subject: [PATCH 2/3] Initialize the inherited class instead of the BaseCache
 class

Otherwise the self._read_client and self._write_client variables won't
be initalized.
---
 flask_caching/backends/clients.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/flask_caching/backends/clients.py b/flask_caching/backends/clients.py
index 9464460..60c6878 100644
--- a/flask_caching/backends/clients.py
+++ b/flask_caching/backends/clients.py
@@ -18,7 +18,7 @@ from flask_caching._compat import PY2, range_type
 class RedisSentinelCache(RedisCache):
     def __init__(self, sentinels=None, master=None, password=None,
                  db=0, default_timeout=300, key_prefix=None, **kwargs):
-        BaseCache.__init__(self, default_timeout)
+        super(RedisSentinelCache, self).__init__(default_timeout, **kwargs)
 
         try:
             import redis.sentinel
-- 
2.20.1


From aa15971d29fae5980fb71ccb1d83abcde05992e2 Mon Sep 17 00:00:00 2001
From: Peter Justin <peter.justin@outlook.com>
Date: Fri, 22 Feb 2019 12:22:20 +0100
Subject: [PATCH 3/3] Use super()-style for initializing inherited parent
 classes

---
 flask_caching/backends/cache.py   | 24 +++++++++++++-----------
 flask_caching/backends/clients.py |  4 ++--
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/flask_caching/backends/cache.py b/flask_caching/backends/cache.py
index 847c0a1..fae715a 100644
--- a/flask_caching/backends/cache.py
+++ b/flask_caching/backends/cache.py
@@ -56,21 +56,23 @@
     :copyright: (c) 2014 by the Werkzeug Team, see AUTHORS for more details.
     :license: BSD, see LICENSE for more details.
 """
+import errno
+import hashlib
 import os
+import platform
 import re
-import errno
 import tempfile
-import platform
-import hashlib
 from time import time
+
+from werkzeug._compat import (integer_types, iteritems, string_types,
+                              text_type, to_native)
+from werkzeug.posixemulation import rename
+
 try:
     import cPickle as pickle
 except ImportError:  # pragma: no cover
     import pickle
 
-from werkzeug._compat import iteritems, string_types, text_type, \
-    integer_types, to_native
-from werkzeug.posixemulation import rename
 
 
 def _items(mappingorseq):
@@ -284,7 +286,7 @@ class SimpleCache(BaseCache):
     """
 
     def __init__(self, threshold=500, default_timeout=300):
-        BaseCache.__init__(self, default_timeout)
+        super(SimpleCache, self).__init__(default_timeout)
         self._cache = {}
         self.clear = self._cache.clear
         self._threshold = threshold
@@ -380,7 +382,7 @@ class MemcachedCache(BaseCache):
     """
 
     def __init__(self, servers=None, default_timeout=300, key_prefix=None):
-        BaseCache.__init__(self, default_timeout)
+        super(MemcachedCache, self).__init__(default_timeout)
         if servers is None or isinstance(servers, (list, tuple)):
             if servers is None:
                 servers = ['127.0.0.1:11211']
@@ -563,7 +565,7 @@ class RedisCache(BaseCache):
 
     def __init__(self, host='localhost', port=6379, password=None,
                  db=0, default_timeout=300, key_prefix=None, **kwargs):
-        BaseCache.__init__(self, default_timeout)
+        super(RedisCache, self).__init__(default_timeout)
         if host is None:
             raise ValueError('RedisCache host parameter may not be None')
         if isinstance(host, string_types):
@@ -712,7 +714,7 @@ class FileSystemCache(BaseCache):
 
     def __init__(self, cache_dir, threshold=500, default_timeout=300,
                  mode=0o600, hash_method=hashlib.md5):
-        BaseCache.__init__(self, default_timeout)
+        super(FileSystemCache, self).__init__(default_timeout)
         self._path = cache_dir
         self._threshold = threshold
         self._mode = mode
@@ -877,7 +879,7 @@ class UWSGICache(BaseCache):
         the cache.
     """
     def __init__(self, default_timeout=300, cache=''):
-        BaseCache.__init__(self, default_timeout)
+        super(UWSGICache, self).__init__(default_timeout)
 
         if platform.python_implementation() == 'PyPy':
             raise RuntimeError("uWSGI caching does not work under PyPy, see "
diff --git a/flask_caching/backends/clients.py b/flask_caching/backends/clients.py
index 60c6878..904171d 100644
--- a/flask_caching/backends/clients.py
+++ b/flask_caching/backends/clients.py
@@ -10,9 +10,9 @@
     :license: BSD, see LICENSE for more details.
 """
 import pickle
-from flask_caching.backends.cache import BaseCache, MemcachedCache, RedisCache
 
 from flask_caching._compat import PY2, range_type
+from flask_caching.backends.cache import MemcachedCache, RedisCache
 
 
 class RedisSentinelCache(RedisCache):
@@ -52,7 +52,7 @@ class RedisSentinelCache(RedisCache):
 class SASLMemcachedCache(MemcachedCache):
     def __init__(self, servers=None, default_timeout=300, key_prefix=None,
                  username=None, password=None, **kwargs):
-        BaseCache.__init__(self, default_timeout)
+        super(SASLMemcachedCache, self).__init__(default_timeout, **kwargs)
 
         if servers is None:
             servers = ['127.0.0.1:11211']
-- 
2.20.1

[gergelypolonkai]

The allow maintainers to edit checkbox is ticked, so i guess you should be able to add commits.

I think the refactoring/import reorganising related stuff should be separate from this PR, but that might be just my OCD speaking :)

The BaseCache.__init__() call might be justified, as RedisCache.__init__() does things we don’t want RedisSentinelCache.__init__() to do. Namely checking the host argument, which is not passed to RedisSentinelCache at all (it needs sentinel_hosts instead). The current soluotion, however, makes PyLint to yell at me for not calling the constructor of the parent class, and that makes me feel uncomfortable. But unless we want to completely duplicate the methods of RedisCache, we might instead create a RedisCacheBase class that might be the copy of the current RedisCache without __init__, then recreate RedisCache with its current __init__, and keep the RedisSentinelCache as is (with the new base class). Another option might be to completely ditch RedisSentinelCache and modify RedisCache so it can connect to a sentinel setup. That, however, might be slightly harder work.

You renaming self._read_clients to self._read_client is valid from a grammatic PoV, but might be wrong otherwise. While _write_client will always be one specific client (as told by the sentinels), _read_client is a connection pool with one or more slaves. I’m still OK with renaming it, though, i’m just saying :)

[sh4nks]

The BaseCache.__init__() call might be justified, as RedisCache.__init__() does things we don’t want RedisSentinelCache.__init__() to do.

But it does not work with having BaseCache.__init__(). I did the tests using the example app in examples/.

Ok, then lets just exclude the last patch ([PATCH 3/3] Use super()-style for initializing inherited parent) from this PR.

My bad, I actually just wanted to pass the same arguments as were passed before.

You renaming self._read_clients to self._read_client is valid from a grammatic PoV, but might be wrong otherwise. While _write_client will always be one specific client (as told by the sentinels), _read_client is a connection pool with one or more slaves. I’m still OK with renaming it, though, i’m just saying :)

I was just doing the fastest way of fixing the things - because in RedisCache, everywhere self._read_client is used instead of self._read_clients.

New patch:

From 9f2eebef66f60b32861e39b1d6ad074b6c918d57 Mon Sep 17 00:00:00 2001
From: Peter Justin <peter.justin@outlook.com>
Date: Fri, 22 Feb 2019 12:17:30 +0100
Subject: [PATCH 1/2] Fix incorrect variable name for read_clients

---
 flask_caching/backends/cache.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/flask_caching/backends/cache.py b/flask_caching/backends/cache.py
index 847c0a1..f815fa4 100644
--- a/flask_caching/backends/cache.py
+++ b/flask_caching/backends/cache.py
@@ -579,7 +579,7 @@ class RedisCache(BaseCache):
         else:
             client = host
 
-        self._write_client = self._read_client = client
+        self._write_client = self._read_clients = client
         self.key_prefix = key_prefix or ''
 
     def _normalize_timeout(self, timeout):
@@ -615,12 +615,12 @@ class RedisCache(BaseCache):
             return value
 
     def get(self, key):
-        return self.load_object(self._read_client.get(self.key_prefix + key))
+        return self.load_object(self._read_clients.get(self.key_prefix + key))
 
     def get_many(self, *keys):
         if self.key_prefix:
             keys = [self.key_prefix + key for key in keys]
-        return [self.load_object(x) for x in self._read_client.mget(keys)]
+        return [self.load_object(x) for x in self._read_clients.mget(keys)]
 
     def set(self, key, value, timeout=None):
         timeout = self._normalize_timeout(timeout)
@@ -667,12 +667,12 @@ class RedisCache(BaseCache):
         return self._write_client.delete(*keys)
 
     def has(self, key):
-        return self._read_client.exists(self.key_prefix + key)
+        return self._read_clients.exists(self.key_prefix + key)
 
     def clear(self):
         status = False
         if self.key_prefix:
-            keys = self._read_client.keys(self.key_prefix + '*')
+            keys = self._read_clients.keys(self.key_prefix + '*')
             if keys:
                 status = self._write_client.delete(*keys)
         else:
-- 
2.20.1


From e13ab0e4d8bbc54dfd6b6cf6beee1bfc0bc83046 Mon Sep 17 00:00:00 2001
From: Peter Justin <peter.justin@outlook.com>
Date: Fri, 22 Feb 2019 12:19:26 +0100
Subject: [PATCH 2/2] Initialize the inherited class instead of the BaseCache
 class

Otherwise the self._read_clients and self._write_client variables won't
be initalized.
---
 flask_caching/backends/clients.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/flask_caching/backends/clients.py b/flask_caching/backends/clients.py
index 069686b..4ca735e 100644
--- a/flask_caching/backends/clients.py
+++ b/flask_caching/backends/clients.py
@@ -18,7 +18,7 @@ from flask_caching._compat import PY2, range_type
 class RedisSentinelCache(RedisCache):
     def __init__(self, sentinels=None, master=None, password=None,
                  db=0, default_timeout=300, key_prefix=None, **kwargs):
-        BaseCache.__init__(self, default_timeout)
+        super(RedisSentinelCache, self).__init__(default_timeout)
 
         try:
             import redis.sentinel
-- 
2.20.1

[gergelypolonkai]

Oh, i take a day out and you merged it already? 😃 Thanks a lot!

Do you think you will release it any time soon?

[sh4nks]

Yeah I'll push out a new release soon. I still have a couple of changes in the pipeline that I would like to have in the release.