Merge lp:~greglange/swift/lp682749 into lp:~hudson-openstack/swift/trunk

[Greg Lange]

Improved caching of results in the proxy server's account_info() and container_info() methods.

[gholt (gholt)]

I'm good with this. I'll wait until Monday to mark it Approved in case anybody else has anything.

review: Approve

[clayg (clay-gerrard)]

on 507 shouldn't we attempts_left += 1 in case of handoffs?

Other than that I'm good with this too.

I really like these tests, nice work glange.

Random thought of the day, this might be wonky, but I think it be kinda cool if in check_account_info_return you did "if is_none is None"

Cause then instead of calling:
self.check_account_info_return(partition, nodes, True)
you would get to say:
self.check_account_info_return(partition, nodes, None)

:)

[Greg Lange (greglange)]

I think we should merge this without the 507 thing or the unit test changes (I like those how they are anyway = ).

If the 507 thing needs to be done, we should do that in a separate bug.

That's outside of what this bug is about.

[clayg (clay-gerrard)]

The 507 thing probably doesn't need to be done - writes use handoffs, reads generally shouldn't. Good call.

review: Approve

=== modified file 'swift/proxy/server.py'
--- swift/proxy/server.py	2010-11-30 23:00:12 +0000
+++ swift/proxy/server.py	2010-12-01 00:06:28 +0000
@@ -88,10 +88,11 @@
         return func(*a, **kw)
     return wrapped
 
+def get_account_memcache_key(account):
+    return 'account/%s' % account
 
 def get_container_memcache_key(account, container):
-    path = '/%s/%s' % (account, container)
-    return 'container%s' % path
+    return 'container/%s/%s' % (account, container)
 
 
 class Controller(object):
@@ -176,13 +177,17 @@
                   if it does not exist
         """
         partition, nodes = self.app.account_ring.get_nodes(account)
-        path = '/%s' % account
-        cache_key = 'account%s' % path
         # 0 = no responses, 200 = found, 404 = not found, -1 = mixed responses
-        if self.app.memcache and self.app.memcache.get(cache_key) == 200:
-            return partition, nodes
+        if self.app.memcache:
+            cache_key = get_account_memcache_key(account)
+            result_code = self.app.memcache.get(cache_key)
+            if result_code == 200:
+                return partition, nodes
+            elif result_code == 404:
+                 return None, None
         result_code = 0
         attempts_left = self.app.account_ring.replica_count
+        path = '/%s' % account
         headers = {'x-cf-trans-id': self.trans_id}
         for node in self.iter_nodes(partition, nodes, self.app.account_ring):
             if self.error_limited(node):
@@ -213,16 +218,16 @@
             except:
                 self.exception_occurred(node, 'Account',
                     'Trying to get account info for %s' % path)
-        if result_code == 200:
-            cache_timeout = self.app.recheck_account_existence
-        else:
-            cache_timeout = self.app.recheck_account_existence * 0.1
-        if self.app.memcache:
+        if self.app.memcache and result_code in (200, 404):
+            if result_code == 200:
+                cache_timeout = self.app.recheck_account_existence
+            else:
+                cache_timeout = self.app.recheck_account_existence * 0.1
             self.app.memcache.set(cache_key, result_code,
                                   timeout=cache_timeout)
         if result_code == 200:
             return partition, nodes
-        return (None, None)
+        return None, None
 
     def container_info(self, account, container):
         """
@@ -239,7 +244,6 @@
         partition, nodes = self.app.container_ring.get_nodes(
                 account, container)
         path = '/%s/%s' % (account, container)
-        cache_key = None
         if self.app.memcache:
             cache_key = get_container_memcache_key(account, container)
             cache_value = self.app.memcache.get(cache_key)
@@ -249,8 +253,10 @@
                 write_acl = cache_value['write_acl']
                 if status == 200:
                     return partition, nodes, read_acl, write_acl
+                elif status == 404:
+                    return None, None, None, None
         if not self.account_info(account)[1]:
-            return (None, None, None, None)
+            return None, None, None, None
         result_code = 0
         read_acl = None
         write_acl = None
@@ -290,11 +296,11 @@
             except:
                 self.exception_occurred(node, 'Container',
                     'Trying to get container info for %s' % path)
-        if result_code == 200:
-            cache_timeout = self.app.recheck_container_existence
-        else:
-            cache_timeout = self.app.recheck_container_existence * 0.1
-        if cache_key and self.app.memcache:
+        if self.app.memcache and result_code in (200, 404):
+            if result_code == 200:
+                cache_timeout = self.app.recheck_container_existence
+            else:
+                cache_timeout = self.app.recheck_container_existence * 0.1
             self.app.memcache.set(cache_key,
                                   {'status': result_code,
                                    'read_acl': read_acl,
@@ -303,7 +309,7 @@
                                   timeout=cache_timeout)
         if result_code == 200:
             return partition, nodes, read_acl, write_acl
-        return (None, None, None, None)
+        return None, None, None, None
 
     def iter_nodes(self, partition, nodes, ring):
         """

=== modified file 'test/unit/proxy/test_server.py'
--- test/unit/proxy/test_server.py	2010-11-30 16:01:05 +0000
+++ test/unit/proxy/test_server.py	2010-12-01 00:06:28 +0000
@@ -88,6 +88,8 @@
                 pass
             if 'slow' in kwargs:
                 headers['content-length'] = '4'
+            if 'headers' in kwargs:
+                headers.update(kwargs['headers'])
             return headers.items()
 
         def read(self, amt=None):
@@ -164,6 +166,9 @@
     def get(self, key):
         return self.store.get(key)
 
+    def keys(self):
+        return self.store.keys()
+
     def set(self, key, value, timeout=0):
         self.store[key] = value
         return True
@@ -201,10 +206,12 @@
 @contextmanager
 def save_globals():
     orig_http_connect = getattr(proxy_server, 'http_connect', None)
+    orig_account_info = getattr(proxy_server.Controller, 'account_info', None)
     try:
         yield True
     finally:
         proxy_server.http_connect = orig_http_connect
+        proxy_server.Controller.account_info = orig_account_info
 
 # tests
 
@@ -212,63 +219,155 @@
 
     def setUp(self):
         self.account_ring = FakeRing()
+        self.container_ring = FakeRing()
+        self.memcache = FakeMemcache()
 
-        app = proxy_server.Application(None, FakeMemcache(),
-            account_ring=self.account_ring, container_ring=FakeRing(),
+        app = proxy_server.Application(None, self.memcache,
+            account_ring=self.account_ring,
+            container_ring=self.container_ring,
             object_ring=FakeRing())
         self.controller = proxy_server.Controller(app)
 
-    def check_account_info_return(self, account, partition, nodes):
-        p, n = self.account_ring.get_nodes(account)
+        self.account = 'some_account'
+        self.container = 'some_container'
+        self.read_acl = 'read_acl'
+        self.write_acl = 'write_acl'
+
+    def check_account_info_return(self, partition, nodes, is_none=False):
+        if is_none:
+            p, n = None, None
+        else:
+            p, n = self.account_ring.get_nodes(self.account)
         self.assertEqual(p, partition)
         self.assertEqual(n, nodes)
 
-    def test_account_info_404_200(self):
-        account = 'test_account_info_404_200'
-
+    # tests if 200 is cached and used
+    def test_account_info_200(self):
         with save_globals():
-            proxy_server.http_connect = fake_http_connect(404, 404, 404)
-            partition, nodes = self.controller.account_info(account)
-            self.assertEqual(partition, None)
-            self.assertEqual(nodes, None)
-
             proxy_server.http_connect = fake_http_connect(200)
-            partition, nodes = self.controller.account_info(account)
-            self.check_account_info_return(account, partition, nodes)
-
+            partition, nodes = self.controller.account_info(self.account)
+            self.check_account_info_return(partition, nodes)
+
+            cache_key = proxy_server.get_account_memcache_key(self.account)
+            self.assertEquals(200, self.memcache.get(cache_key))
+
+            proxy_server.http_connect = fake_http_connect()
+            partition, nodes = self.controller.account_info(self.account)
+            self.check_account_info_return(partition, nodes)
+
+    # tests if 404 is cached and used
     def test_account_info_404(self):
-        account = 'test_account_info_404'
-
-        with save_globals():
-            proxy_server.http_connect = fake_http_connect(404, 404, 404)
-            partition, nodes = self.controller.account_info(account)
-            self.assertEqual(partition, None)
-            self.assertEqual(nodes, None)
-
-            proxy_server.http_connect = fake_http_connect(404, 404, 404)
-            partition, nodes = self.controller.account_info(account)
-            self.assertEqual(partition, None)
-            self.assertEqual(nodes, None)
-
-    def test_account_info_200(self):
-        account = 'test_account_info_200'
-
-        with save_globals():
-            proxy_server.http_connect = fake_http_connect(200)
-            partition, nodes = self.controller.account_info(account)
-            self.check_account_info_return(account, partition, nodes)
-
-    def test_account_info_200_200(self):
-        account = 'test_account_info_200_200'
-
-        with save_globals():
-            proxy_server.http_connect = fake_http_connect(200)
-            partition, nodes = self.controller.account_info(account)
-            self.check_account_info_return(account, partition, nodes)
-
-            proxy_server.http_connect = fake_http_connect(200)
-            partition, nodes = self.controller.account_info(account)
-            self.check_account_info_return(account, partition, nodes)
+        with save_globals():
+            proxy_server.http_connect = fake_http_connect(404, 404, 404)
+            partition, nodes = self.controller.account_info(self.account)
+            self.check_account_info_return(partition, nodes, True)
+
+            cache_key = proxy_server.get_account_memcache_key(self.account)
+            self.assertEquals(404, self.memcache.get(cache_key))
+
+            proxy_server.http_connect = fake_http_connect()
+            partition, nodes = self.controller.account_info(self.account)
+            self.check_account_info_return(partition, nodes, True)
+
+    # tests if some http status codes are not cached
+    def test_account_info_no_cache(self):
+        def test(*status_list):
+            proxy_server.http_connect = fake_http_connect(*status_list)
+            partition, nodes = self.controller.account_info(self.account)
+            self.assertEqual(len(self.memcache.keys()), 0)
+            self.check_account_info_return(partition, nodes, True)
+
+        with save_globals():
+            test(503, 404, 404)
+            test(404, 404, 503)
+            test(404, 507, 503)
+            test(503, 503, 503)
+
+    def check_container_info_return(self, ret, is_none=False):
+        if is_none:
+            partition, nodes, read_acl, write_acl = None, None, None, None
+        else:
+            partition, nodes = self.container_ring.get_nodes(self.account,
+                self.container)
+            read_acl, write_acl = self.read_acl, self.write_acl
+        self.assertEqual(partition, ret[0])
+        self.assertEqual(nodes, ret[1])
+        self.assertEqual(read_acl, ret[2])
+        self.assertEqual(write_acl, ret[3])
+
+    def test_container_info_invalid_account(self):
+        def account_info(self, account):
+            return None, None
+
+        with save_globals():
+            proxy_server.Controller.account_info = account_info
+            ret = self.controller.container_info(self.account,
+                self.container)
+            self.check_container_info_return(ret, True)
+
+    # tests if 200 is cached and used
+    def test_container_info_200(self):
+        def account_info(self, account):
+            return True, True
+
+        with save_globals():
+            headers = {'x-container-read': self.read_acl,
+                'x-container-write': self.write_acl}
+            proxy_server.Controller.account_info = account_info
+            proxy_server.http_connect = fake_http_connect(200,
+                headers=headers)
+            ret = self.controller.container_info(self.account,
+                self.container)
+            self.check_container_info_return(ret)
+
+            cache_key = proxy_server.get_container_memcache_key(self.account,
+                self.container)
+            cache_value = self.memcache.get(cache_key)
+            self.assertEquals(dict, type(cache_value))
+            self.assertEquals(200, cache_value.get('status'))
+
+            proxy_server.http_connect = fake_http_connect()
+            ret = self.controller.container_info(self.account,
+                 self.container)
+            self.check_container_info_return(ret)
+
+    # tests if 404 is cached and used
+    def test_container_info_404(self):
+        def account_info(self, account):
+            return True, True
+
+        with save_globals():
+            proxy_server.Controller.account_info = account_info
+            proxy_server.http_connect = fake_http_connect(404, 404, 404)
+            ret = self.controller.container_info(self.account,
+                self.container)
+            self.check_container_info_return(ret, True)
+
+            cache_key = proxy_server.get_container_memcache_key(self.account,
+                self.container)
+            cache_value = self.memcache.get(cache_key)
+            self.assertEquals(dict, type(cache_value))
+            self.assertEquals(404, cache_value.get('status'))
+
+            proxy_server.http_connect = fake_http_connect()
+            ret = self.controller.container_info(self.account,
+                 self.container)
+            self.check_container_info_return(ret, True)
+
+    # tests if some http status codes are not cached
+    def test_container_info_no_cache(self):
+        def test(*status_list):
+            proxy_server.http_connect = fake_http_connect(*status_list)
+            ret = self.controller.container_info(self.account,
+                self.container)
+            self.assertEqual(len(self.memcache.keys()), 0)
+            self.check_container_info_return(ret, True)
+
+        with save_globals():
+            test(503, 404, 404)
+            test(404, 404, 503)
+            test(404, 507, 503)
+            test(503, 503, 503)
 
 class TestProxyServer(unittest.TestCase):