Allow to configure pymemcache's HashClient retrying

These changes allow us to configure pymemcache's HashClient retrying
mechanisms by exposing his public params. Without these changes that we are
limitated to the default values.

Configuring the internal retrying feature of the HashClient can be
useful in a HA context to properly setup the failover.

The internal retrying feature are a good complement to the retrying
wrapper introduced previously.

[1] https://github.com/pinterest/pymemcache/blob/071191455363943e7d5919701465455e78bb64ae/pymemcache/client/hash.py#L66-L71

Change-Id: Id7aa3d363d281daa640ca34a325454d4c18a8ff0

3 files changed, 181 insertions, 19 deletions

diff --git a/docs/build/unreleased/hashclient.rst b/docs/build/unreleased/hashclient.rst
new file mode 100644
index 0000000..f0ceb22
--- /dev/null
+++ b/docs/build/unreleased/hashclient.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: usecase, memcached
+
+    Added support for additional pymemcache ``HashClient`` parameters
+    ``retry_attempts``, ``retry_timeout``, and
+    ``dead_timeout``.
+
+    .. seealso::
+
+        :paramref:`.PyMemcacheBackend.hashclient_retry_attempts`
+
+        :paramref:`.PyMemcacheBackend.hashclient_retry_timeout`
+
+        :paramref:`.PyMemcacheBackend.dead_timeout`
\ No newline at end of file
diff --git a/dogpile/cache/backends/memcached.py b/dogpile/cache/backends/memcached.py
index 66e9d58..8163d70 100644
--- a/dogpile/cache/backends/memcached.py
+++ b/dogpile/cache/backends/memcached.py
@@ -513,13 +513,14 @@ class PyMemcacheBackend(GenericMemcachedBackend):
 
      .. versionadded:: 1.1.4
 
-    :param retry_attempts: how many times to attempt an action before
-     failing. Must be 1 or above. Defaults to None.
+    :param retry_attempts: how many times to attempt an action with
+     pymemcache's retrying wrapper before failing. Must be 1 or above.
+     Defaults to None.
 
      .. versionadded:: 1.1.4
 
     :param retry_delay: optional int|float, how many seconds to sleep between
-     each attempt. Defaults to None.
+     each attempt. Used by the retry wrapper. Defaults to None.
 
      .. versionadded:: 1.1.4
 
@@ -537,6 +538,22 @@ class PyMemcacheBackend(GenericMemcachedBackend):
 
      .. versionadded:: 1.1.4
 
+    :param hashclient_retry_attempts: Amount of times a client should be tried
+     before it is marked dead and removed from the pool in the HashClient's
+     internal mechanisms.
+
+     .. versionadded:: 1.1.5
+
+    :param hashclient_retry_timeout: Time in seconds that should pass between
+     retry attempts in the HashClient's internal mechanisms.
+
+     .. versionadded:: 1.1.5
+
+    :param dead_timeout: Time in seconds before attempting to add a node
+     back in the pool in the HashClient's internal mechanisms.
+
+     .. versionadded:: 1.1.5
+
     """  # noqa E501
 
     def __init__(self, arguments):
@@ -551,6 +568,13 @@ class PyMemcacheBackend(GenericMemcachedBackend):
         self.retry_delay = arguments.get("retry_delay", None)
         self.retry_for = arguments.get("retry_for", None)
         self.do_not_retry_for = arguments.get("do_not_retry_for", None)
+        self.hashclient_retry_attempts = arguments.get(
+            "hashclient_retry_attempts", 2
+        )
+        self.hashclient_retry_timeout = arguments.get(
+            "hashclient_retry_timeout", 1
+        )
+        self.dead_timeout = arguments.get("hashclient_dead_timeout", 60)
         if (
             self.retry_delay is not None
             or self.retry_attempts is not None
@@ -571,12 +595,14 @@ class PyMemcacheBackend(GenericMemcachedBackend):
             "serde": self.serde,
             "default_noreply": self.default_noreply,
             "tls_context": self.tls_context,
+            "retry_attempts": self.hashclient_retry_attempts,
+            "retry_timeout": self.hashclient_retry_timeout,
+            "dead_timeout": self.dead_timeout,
         }
         if self.socket_keepalive is not None:
             _kwargs.update({"socket_keepalive": self.socket_keepalive})
 
         client = pymemcache.client.hash.HashClient(self.url, **_kwargs)
-
         if self.enable_retry_client:
             return pymemcache.client.retrying.RetryingClient(
                 client,
diff --git a/tests/cache/test_memcached_backend.py b/tests/cache/test_memcached_backend.py
index 1882054..3338bd8 100644
--- a/tests/cache/test_memcached_backend.py
+++ b/tests/cache/test_memcached_backend.py
@@ -8,11 +8,12 @@ import weakref
 
 import pytest
 
-from dogpile.cache import make_region
 from dogpile.cache.backends.memcached import GenericMemcachedBackend
 from dogpile.cache.backends.memcached import MemcachedBackend
 from dogpile.cache.backends.memcached import PylibmcBackend
+from dogpile.cache.backends.memcached import PyMemcacheBackend
 from . import eq_
+from . import is_
 from ._fixtures import _GenericBackendTest
 from ._fixtures import _GenericMutexTest
 from ._fixtures import _GenericSerializerTest
@@ -171,20 +172,6 @@ class BMemcachedSerializerTest(
 class PyMemcacheTest(_NonDistributedMemcachedTest):
     backend = "dogpile.cache.pymemcache"
 
-    def test_pymemcache_enable_retry_client_not_set(self):
-        with mock.patch("warnings.warn") as warn_mock:
-            _ = make_region().configure(
-                "dogpile.cache.pymemcache",
-                arguments={"url": "foo", "retry_attempts": 2},
-            )
-            eq_(
-                warn_mock.mock_calls[0],
-                mock.call(
-                    "enable_retry_client is not set; retry options "
-                    "will be ignored"
-                ),
-            )
-
 
 class PyMemcacheDistributedWithTimeoutTest(
     _DistributedMemcachedWithTimeoutTest
@@ -227,6 +214,136 @@ class PyMemcacheRetryTest(_NonDistributedMemcachedTest):
     }
 
 
+class PyMemcacheArgsTest(TestCase):
+    # TODO: convert dogpile to be able to use pytest.fixtures (remove
+    # unittest.TestCase dependency) and use that to set up the mock module
+    # instead of an explicit method call
+    def _mock_pymemcache_fixture(self):
+        self.hash_client = mock.Mock()
+        self.retrying_client = mock.Mock()
+        self.pickle_serde = mock.Mock()
+        pymemcache_module = mock.Mock(
+            serde=mock.Mock(pickle_serde=self.pickle_serde),
+            client=mock.Mock(
+                hash=mock.Mock(HashClient=self.hash_client),
+                retrying=mock.Mock(RetryingClient=self.retrying_client),
+            ),
+        )
+        return mock.patch(
+            "dogpile.cache.backends.memcached.pymemcache", pymemcache_module
+        )
+
+    def test_pymemcache_hashclient_retry_attempts(self):
+        config_args = {
+            "url": "127.0.0.1:11211",
+            "hashclient_retry_attempts": 4,
+        }
+
+        with self._mock_pymemcache_fixture():
+            backend = MockPyMemcacheBackend(config_args)
+            is_(backend._create_client(), self.hash_client())
+            eq_(
+                self.hash_client.mock_calls[0],
+                mock.call(
+                    ["127.0.0.1:11211"],
+                    serde=self.pickle_serde,
+                    default_noreply=False,
+                    tls_context=None,
+                    retry_attempts=4,
+                    retry_timeout=1,
+                    dead_timeout=60,
+                ),
+            )
+            eq_(self.retrying_client.mock_calls, [])
+
+    def test_pymemcache_hashclient_retry_timeout(self):
+        config_args = {"url": "127.0.0.1:11211", "hashclient_retry_timeout": 4}
+        with self._mock_pymemcache_fixture():
+            backend = MockPyMemcacheBackend(config_args)
+            is_(backend._create_client(), self.hash_client())
+            eq_(
+                self.hash_client.mock_calls[0],
+                mock.call(
+                    ["127.0.0.1:11211"],
+                    serde=self.pickle_serde,
+                    default_noreply=False,
+                    tls_context=None,
+                    retry_attempts=2,
+                    retry_timeout=4,
+                    dead_timeout=60,
+                ),
+            )
+            eq_(self.retrying_client.mock_calls, [])
+
+    def test_pymemcache_hashclient_retry_timeout_w_enable_retry(self):
+        config_args = {
+            "url": "127.0.0.1:11211",
+            "hashclient_retry_timeout": 4,
+            "enable_retry_client": True,
+            "retry_attempts": 3,
+        }
+        with self._mock_pymemcache_fixture():
+            backend = MockPyMemcacheBackend(config_args)
+            is_(backend._create_client(), self.retrying_client())
+            eq_(
+                self.hash_client.mock_calls[0],
+                mock.call(
+                    ["127.0.0.1:11211"],
+                    serde=self.pickle_serde,
+                    default_noreply=False,
+                    tls_context=None,
+                    retry_attempts=2,
+                    retry_timeout=4,
+                    dead_timeout=60,
+                ),
+            )
+            eq_(
+                self.retrying_client.mock_calls[0],
+                mock.call(
+                    self.hash_client(),
+                    attempts=3,
+                    retry_delay=None,
+                    retry_for=None,
+                    do_not_retry_for=None,
+                ),
+            )
+
+    def test_pymemcache_dead_timeout(self):
+        config_args = {"url": "127.0.0.1:11211", "hashclient_dead_timeout": 4}
+        with self._mock_pymemcache_fixture():
+            backend = MockPyMemcacheBackend(config_args)
+            backend._create_client()
+            eq_(
+                self.hash_client.mock_calls,
+                [
+                    mock.call(
+                        ["127.0.0.1:11211"],
+                        serde=self.pickle_serde,
+                        default_noreply=False,
+                        tls_context=None,
+                        retry_attempts=2,
+                        retry_timeout=1,
+                        dead_timeout=4,
+                    )
+                ],
+            )
+            eq_(self.retrying_client.mock_calls, [])
+
+    def test_pymemcache_enable_retry_client_not_set(self):
+        config_args = {"url": "127.0.0.1:11211", "retry_attempts": 2}
+
+        with self._mock_pymemcache_fixture():
+            with mock.patch("warnings.warn") as warn_mock:
+                MockPyMemcacheBackend(config_args)
+                eq_(
+                    warn_mock.mock_calls[0],
+                    mock.call(
+                        "enable_retry_client is not set; retry options "
+                        "will be ignored"
+                    ),
+                )
+
+
 class MemcachedTest(_NonDistributedMemcachedTest):
     backend = "dogpile.cache.memcached"
 
@@ -253,6 +370,11 @@ class MockGenericMemcachedBackend(GenericMemcachedBackend):
         return MockClient(self.url)
 
 
+class MockPyMemcacheBackend(PyMemcacheBackend):
+    def _imports(self):
+        pass
+
+
 class MockMemcacheBackend(MemcachedBackend):
     def _imports(self):
         pass