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

Author: 4383

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`

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 @@ def __init__(self, arguments):
         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 @@ def _create_client(self):
             "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,

tests/cache/test_memcached_backend.py

@@ -8,11 +8,12 @@
 
 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 @@ def _create_client(self):
         return MockClient(self.url)
 
 
+class MockPyMemcacheBackend(PyMemcacheBackend):
+    def _imports(self):
+        pass
+
+
 class MockMemcacheBackend(MemcachedBackend):
     def _imports(self):
         pass