Don't fetch metadata for plain container listing

Fetch metadata (xattr) for containers in an account ONLY when the client
asks for it (using content-type indicating JSON or XML response).
This avoids a lot of unnecessarry stat() and getxattr() calls whose
results would anyways be unused. The performance gain is obvious in this
case.

This change is restricted to container listing. The same can be extended
to object listing as well (will be sent as a separate change)

Change-Id: Ibff1c5a90519f11053c0b651d8ea3385dda43a2f
Signed-off-by: Prashanth Pai <ppai@redhat.com>
Reviewed-on: http://review.gluster.org/13497
Reviewed-by: Thiago da Silva <thiago@redhat.com>
Tested-by: Thiago da Silva <thiago@redhat.com>

5 files changed, 137 insertions, 4 deletions

diff --git a/gluster/swift/account/utils.py b/gluster/swift/account/utils.py
new file mode 100644
index 0000000..99fe5ea
--- /dev/null
+++ b/gluster/swift/account/utils.py
@@ -0,0 +1,71 @@
+# Copyright (c) 2016 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from swift.account.utils import FakeAccountBroker, get_response_headers
+from swift.common.swob import HTTPOk, HTTPNoContent
+from swift.common.utils import json
+from xml.sax import saxutils
+
+
+def account_listing_response(account, req, response_content_type, broker=None,
+                             limit='', marker='', end_marker='', prefix='',
+                             delimiter=''):
+    """
+    This is an exact copy of swift.account.utis.account_listing_response()
+    except for one difference i.e this method passes response_content_type
+    to broker.list_containers_iter() method.
+    """
+    if broker is None:
+        broker = FakeAccountBroker()
+
+    resp_headers = get_response_headers(broker)
+
+    account_list = broker.list_containers_iter(limit, marker, end_marker,
+                                               prefix, delimiter,
+                                               response_content_type)
+    if response_content_type == 'application/json':
+        data = []
+        for (name, object_count, bytes_used, is_subdir) in account_list:
+            if is_subdir:
+                data.append({'subdir': name})
+            else:
+                data.append({'name': name, 'count': object_count,
+                             'bytes': bytes_used})
+        account_list = json.dumps(data)
+    elif response_content_type.endswith('/xml'):
+        output_list = ['<?xml version="1.0" encoding="UTF-8"?>',
+                       '<account name=%s>' % saxutils.quoteattr(account)]
+        for (name, object_count, bytes_used, is_subdir) in account_list:
+            if is_subdir:
+                output_list.append(
+                    '<subdir name=%s />' % saxutils.quoteattr(name))
+            else:
+                item = '<container><name>%s</name><count>%s</count>' \
+                       '<bytes>%s</bytes></container>' % \
+                       (saxutils.escape(name), object_count, bytes_used)
+                output_list.append(item)
+        output_list.append('</account>')
+        account_list = '\n'.join(output_list)
+    else:
+        if not account_list:
+            resp = HTTPNoContent(request=req, headers=resp_headers)
+            resp.content_type = response_content_type
+            resp.charset = 'utf-8'
+            return resp
+        account_list = '\n'.join(r[0] for r in account_list) + '\n'
+    ret = HTTPOk(body=account_list, request=req, headers=resp_headers)
+    ret.content_type = response_content_type
+    ret.charset = 'utf-8'
+    return ret
diff --git a/gluster/swift/common/DiskDir.py b/gluster/swift/common/DiskDir.py
index e8dba35..31b923a 100644
--- a/gluster/swift/common/DiskDir.py
+++ b/gluster/swift/common/DiskDir.py
@@ -696,7 +696,7 @@ class DiskAccount(DiskCommon):
         return containers
 
     def list_containers_iter(self, limit, marker, end_marker,
-                             prefix, delimiter):
+                             prefix, delimiter, response_content_type=None):
         """
         Return tuple of name, object_count, bytes_used, 0(is_subdir).
         Used by account server.
@@ -709,6 +709,7 @@ class DiskAccount(DiskCommon):
         if containers:
             containers.sort()
         else:
+            # No containers in account, return empty list
             return account_list
 
         if containers and end_marker:
@@ -737,6 +738,19 @@ class DiskAccount(DiskCommon):
                 containers = filter_delimiter(containers, delimiter, prefix,
                                               marker)
 
+        if response_content_type == 'text/plain':
+            # The client is only asking for a plain list of containers and NOT
+            # asking for any extended information about container such as
+            # bytes used or object count.
+            for container in containers:
+                # When response_content_type == 'text/plain', Swift will only
+                # consume the name of the container (first element of tuple).
+                # Refer: swift.account.utils.account_listing_response()
+                account_list.append((container, 0, 0, 0))
+                if len(account_list) >= limit:
+                    break
+            return account_list
+
         count = 0
         for cont in containers:
             list_item = []
diff --git a/gluster/swift/common/constraints.py b/gluster/swift/common/constraints.py
index 7979b43..98e2a27 100644
--- a/gluster/swift/common/constraints.py
+++ b/gluster/swift/common/constraints.py
@@ -97,3 +97,8 @@ __Ring = _ring.Ring
 
 # Replace the original Ring class
 _ring.Ring = ring.Ring
+
+# Monkey patch account_listing_response
+import swift.account.utils
+from gluster.swift.account.utils import account_listing_response as gf_als
+swift.account.utils.account_listing_response = gf_als
diff --git a/gluster/swift/common/utils.py b/gluster/swift/common/utils.py
index ded2f0b..8958717 100644
--- a/gluster/swift/common/utils.py
+++ b/gluster/swift/common/utils.py
@@ -375,7 +375,6 @@ def get_account_details(acc_path):
     Return container_list and container_count.
     """
     container_list = []
-    container_count = 0
 
     if do_isdir(acc_path):
         for name in do_listdir(acc_path):
@@ -383,11 +382,12 @@ def get_account_details(acc_path):
                     or name.lower() == ASYNCDIR \
                     or name.lower() == TRASHCAN \
                     or not do_isdir(os.path.join(acc_path, name)):
+                # Do not include .async_pending, .trashcan and all
+                # non-directories in containers list
                 continue
-            container_count += 1
             container_list.append(name)
 
-    return container_list, container_count
+    return container_list, len(container_list)
 
 
 def _read_for_etag(fp):
diff --git a/test/unit/common/test_diskdir.py b/test/unit/common/test_diskdir.py
index 3b95de8..0ad607b 100644
--- a/test/unit/common/test_diskdir.py
+++ b/test/unit/common/test_diskdir.py
@@ -22,6 +22,7 @@ import unittest
 import shutil
 import tarfile
 import hashlib
+from mock import Mock, patch
 from time import time
 from swift.common.utils import normalize_timestamp
 from gluster.swift.common import utils
@@ -1179,6 +1180,48 @@ class TestAccountBroker(unittest.TestCase):
         self.assertEquals([row[0] for row in listing],
                           ['3-0049-', '3-0049-0049'])
 
+    def test_list_containers_iter_plain_listing(self):
+        broker = self._get_broker(account='a')
+        broker.initialize(self.initial_ts)
+        for cont in xrange(10):
+            # Create 10 containers - lci0 to lci9
+            self._create_container('lci%d' % cont)
+
+        # Check and assert that only name is fetched.
+        listing = broker.list_containers_iter(100, '', None, None,
+                                              '', 'text/plain')
+        self.assertEquals(len(listing), 10)
+        for i, (name, o_count, bytes_used, j) in enumerate(listing):
+            self.assertEqual(name, 'lci%d' % i)
+            self.assertEqual(o_count, 0)
+            self.assertEqual(bytes_used, 0)
+            self.assertEqual(j, 0)
+
+        # Check that limit is still honored.
+        listing = broker.list_containers_iter(5, '', None, None,
+                                              '', 'text/plain')
+        self.assertEquals(len(listing), 5)
+
+        # Confirm that metadata of containers (xattrs) are not fetched when
+        # response_content_type is text/plain
+        _m_r_md = Mock(return_value={})
+        with patch('gluster.swift.common.DiskDir._read_metadata', _m_r_md):
+            listing = broker.list_containers_iter(100, '', None, None,
+                                                  '', 'text/plain')
+            self.assertEquals(len(listing), 10)
+        self.assertFalse(_m_r_md.called)
+
+        # Confirm that metadata of containers (xattrs) are still fetched when
+        # response_content_type is NOT text/plain
+        _m_r_md.reset_mock()
+        with patch('gluster.swift.common.DiskDir._read_metadata', _m_r_md):
+            listing = broker.list_containers_iter(100, '', None, None,
+                                                  '', 'application/json')
+            self.assertEquals(len(listing), 10)
+        self.assertTrue(_m_r_md.called)
+        self.assertEqual(_m_r_md.call_count, 10)
+
+
     def test_double_check_trailing_delimiter(self):
         # Test swift.common.db.AccountBroker.list_containers_iter for an
         # account that has an odd container with a trailing delimiter