launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:memcache-client-refactor into launchpad:master.

Commit message:
Make memcache integration a little more flexible

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/399428

This adds expiry time support to `MemcacheFixture`, and makes the request-timeline memcache client integration optional so that `memcache_client_factory` can be used in lightweight processes.

I extracted this from https://code.launchpad.net/~cjwatson/launchpad/wsgi-ppa-auth/+merge/332125; I hope we'll still end up landing something like that, but even if we don't, this work may well still be useful for something else in the future.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:memcache-client-refactor into launchpad:master.

diff --git a/lib/lp/services/memcache/client.py b/lib/lp/services/memcache/client.py
index a3b5107..8e9e48e 100644
--- a/lib/lp/services/memcache/client.py
+++ b/lib/lp/services/memcache/client.py
@@ -1,67 +1,30 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Launchpad Memcache client."""
 
 __metaclass__ = type
-__all__ = []
+__all__ = [
+    'memcache_client_factory',
+    ]
 
-import logging
 import re
 
-from lazr.restful.utils import get_current_browser_request
 import memcache
 
-from lp.services import features
 from lp.services.config import config
-from lp.services.timeline.requesttimeline import get_request_timeline
 
 
-def memcache_client_factory():
+def memcache_client_factory(timeline=True):
     """Return a memcache.Client for Launchpad."""
     servers = [
         (host, int(weight)) for host, weight in re.findall(
             r'\((.+?),(\d+)\)', config.memcache.servers)]
     assert len(servers) > 0, "Invalid memcached server list %r" % (
         config.memcache.servers,)
-    return TimelineRecordingClient(servers)
-
-
-class TimelineRecordingClient(memcache.Client):
-
-    def __get_timeline_action(self, suffix, key):
-        request = get_current_browser_request()
-        timeline = get_request_timeline(request)
-        return timeline.start("memcache-%s" % suffix, key)
-
-    @property
-    def _enabled(self):
-        configured_value = features.getFeatureFlag('memcache')
-        if configured_value is None:
-            return True
-        else:
-            return configured_value
-
-    def get(self, key):
-        if not self._enabled:
-            return None
-        action = self.__get_timeline_action("get", key)
-        try:
-            return memcache.Client.get(self, key)
-        finally:
-            action.finish()
-
-    def set(self, key, value, time=0, min_compress_len=0):
-        if not self._enabled:
-            return None
-        action = self.__get_timeline_action("set", key)
-        try:
-            success = memcache.Client.set(self, key, value, time=time,
-                min_compress_len=min_compress_len)
-            if success:
-                logging.debug("Memcache set succeeded for %s", key)
-            else:
-                logging.warning("Memcache set failed for %s", key)
-            return success
-        finally:
-            action.finish()
+    if timeline:
+        from lp.services.memcache.timeline import TimelineRecordingClient
+        client_factory = TimelineRecordingClient
+    else:
+        client_factory = memcache.Client
+    return client_factory(servers)
diff --git a/lib/lp/services/memcache/testing.py b/lib/lp/services/memcache/testing.py
index 5590519..45ce145 100644
--- a/lib/lp/services/memcache/testing.py
+++ b/lib/lp/services/memcache/testing.py
@@ -1,4 +1,4 @@
-# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -6,6 +6,8 @@ __all__ = [
     'MemcacheFixture',
     ]
 
+import time as _time
+
 import fixtures
 
 from lp.services.memcache.interfaces import IMemcacheClient
@@ -22,10 +24,20 @@ class MemcacheFixture(fixtures.Fixture):
         self.useFixture(ZopeUtilityFixture(self, IMemcacheClient))
 
     def get(self, key):
-        return self._cache.get(key)
-
-    def set(self, key, val):
-        self._cache[key] = val
+        value, expiry_time = self._cache.get(key, (None, None))
+        if expiry_time and _time.time() >= expiry_time:
+            self.delete(key)
+            return None
+        else:
+            return value
+
+    def set(self, key, val, time=0):
+        # memcached accepts either delta-seconds from the current time or
+        # absolute epoch-seconds, and tells them apart using a magic
+        # threshold.  See memcached/memcached.c:realtime.
+        if time and time <= 60 * 60 * 24 * 30:
+            time = _time.time() + time
+        self._cache[key] = (val, time)
         return 1
 
     def delete(self, key):
diff --git a/lib/lp/services/memcache/tests/test_memcache_client.py b/lib/lp/services/memcache/tests/test_memcache_client.py
index 4c37a26..ef7ead0 100644
--- a/lib/lp/services/memcache/tests/test_memcache_client.py
+++ b/lib/lp/services/memcache/tests/test_memcache_client.py
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the IMemcacheClient utility."""
@@ -8,6 +8,7 @@ __metaclass__ = type
 from lazr.restful.utils import get_current_browser_request
 from zope.component import getUtility
 
+from lp.services.memcache.client import memcache_client_factory
 from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.timeline.requesttimeline import get_request_timeline
 from lp.testing import TestCase
@@ -53,3 +54,30 @@ class MemcacheClientTestCase(TestCase):
         action = timeline.actions[-1]
         self.assertEqual('memcache-get', action.category)
         self.assertEqual('foo', action.detail)
+
+
+class MemcacheClientFactoryTestCase(TestCase):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_with_timeline(self):
+        # memcache_client_factory defaults to returning a client that
+        # records events to a timeline.
+        client = memcache_client_factory()
+        request = get_current_browser_request()
+        timeline = get_request_timeline(request)
+        base_action_count = len(timeline.actions)
+        client.set('foo', 'bar')
+        self.assertEqual('bar', client.get('foo'))
+        self.assertEqual(base_action_count + 2, len(timeline.actions))
+
+    def test_without_timeline(self):
+        # We can explicitly request a client that does not record events to
+        # a timeline.
+        client = memcache_client_factory(timeline=False)
+        request = get_current_browser_request()
+        timeline = get_request_timeline(request)
+        base_action_count = len(timeline.actions)
+        client.set('foo', 'bar')
+        self.assertEqual('bar', client.get('foo'))
+        self.assertEqual(base_action_count, len(timeline.actions))
diff --git a/lib/lp/services/memcache/timeline.py b/lib/lp/services/memcache/timeline.py
new file mode 100644
index 0000000..03c7116
--- /dev/null
+++ b/lib/lp/services/memcache/timeline.py
@@ -0,0 +1,57 @@
+# Copyright 2017-2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Timeline-friendly Launchpad Memcache client."""
+
+__metaclass__ = type
+__all__ = [
+    'TimelineRecordingClient',
+    ]
+
+import logging
+
+from lazr.restful.utils import get_current_browser_request
+import memcache
+
+from lp.services import features
+from lp.services.timeline.requesttimeline import get_request_timeline
+
+
+class TimelineRecordingClient(memcache.Client):
+
+    def __get_timeline_action(self, suffix, key):
+        request = get_current_browser_request()
+        timeline = get_request_timeline(request)
+        return timeline.start("memcache-%s" % suffix, key)
+
+    @property
+    def _enabled(self):
+        configured_value = features.getFeatureFlag('memcache')
+        if configured_value is None:
+            return True
+        else:
+            return configured_value
+
+    def get(self, key):
+        if not self._enabled:
+            return None
+        action = self.__get_timeline_action("get", key)
+        try:
+            return memcache.Client.get(self, key)
+        finally:
+            action.finish()
+
+    def set(self, key, value, time=0, min_compress_len=0):
+        if not self._enabled:
+            return None
+        action = self.__get_timeline_action("set", key)
+        try:
+            success = memcache.Client.set(self, key, value, time=time,
+                min_compress_len=min_compress_len)
+            if success:
+                logging.debug("Memcache set succeeded for %s", key)
+            else:
+                logging.warning("Memcache set failed for %s", key)
+            return success
+        finally:
+            action.finish()