From: David-Sarah Hopwood <david-sarah@jacaranda.org>
Date: Thu, 21 Feb 2013 22:14:27 +0000 (+0000)
Subject: cloud_common.py: generalize ContainerRetryMixin to allow the container class to speci... 
X-Git-Url: https://git.rkrishnan.org/reliability?a=commitdiff_plain;h=7fdb015e0c978fca3168e4596bdadb0e5a6f7ac5;p=tahoe-lafs%2Ftahoe-lafs.git

cloud_common.py: generalize ContainerRetryMixin to allow the container class to specify what to retry.

Signed-off-by: David-Sarah Hopwood <david-sarah@jacaranda.org>
---

diff --git a/src/allmydata/storage/backends/cloud/cloud_common.py b/src/allmydata/storage/backends/cloud/cloud_common.py
index 00004c28..3d38f230 100644
--- a/src/allmydata/storage/backends/cloud/cloud_common.py
+++ b/src/allmydata/storage/backends/cloud/cloud_common.py
@@ -323,16 +323,28 @@ class ContainerListing(object):
                })
 
 
-BACKOFF_SECONDS_FOR_5XX = (0, 2, 10)
+BACKOFF_SECONDS_BEFORE_RETRY = (0, 2, 10)
 
 
 class ContainerRetryMixin:
     """
     I provide a helper method for performing an operation on a cloud container that will retry up to
-    len(BACKOFF_SECONDS_FOR_5XX) times (not including the initial try). If the initial try fails, a
+    len(BACKOFF_SECONDS_FOR_RETRY) times (not including the initial try). If the initial try fails, a
     single incident will be triggered after the operation has succeeded or failed.
+
+    Subclasses should define:
+      ServiceError:
+          The error class to trap (CloudServiceError or similar).
+
+    and can override:
+      _react_to_error(self, response_code):
+          Returns True if the error should be retried. May perform side effects before the retry.
     """
 
+    def _react_to_error(self, response_code):
+        # The default policy is to retry on 5xx errors.
+        return response_code >= 500 and response_code < 600
+
     def _do_request(self, description, operation, *args, **kwargs):
         d = defer.maybeDeferred(operation, *args, **kwargs)
         def _retry(f):
@@ -369,20 +381,22 @@ class ContainerRetryMixin:
         if first_err_and_tb is None:
             first_err_and_tb = (err, tb)
 
-        if trynum > len(BACKOFF_SECONDS_FOR_5XX):
+        if trynum > len(BACKOFF_SECONDS_BEFORE_RETRY):
             # If we run out of tries, raise the error we got on the first try (which *may* have
             # a more useful traceback).
             (first_err, first_tb) = first_err_and_tb
             raise first_err.__class__, first_err, first_tb
 
         fargs = f.value.args
-        if len(fargs) > 0 and int(fargs[0]) >= 500 and int(fargs[0]) < 600:
-            # Retry on 5xx errors.
-            d = task.deferLater(reactor, BACKOFF_SECONDS_FOR_5XX[trynum-1], operation, *args, **kwargs)
-            d.addErrback(self._handle_error, trynum+1, first_err_and_tb, description, operation, *args, **kwargs)
-            return d
-
-        # If we get an error response other than a 5xx, raise that error even if it was on a retry.
+        if len(fargs) > 0:
+            retry = self._react_to_error(int(fargs[0]))
+            if retry:
+                d = task.deferLater(reactor, BACKOFF_SECONDS_BEFORE_RETRY[trynum-1], operation, *args, **kwargs)
+                d.addErrback(self._handle_error, trynum+1, first_err_and_tb, description, operation, *args, **kwargs)
+                return d
+
+        # If we get an error response for which _react_to_error says we should not retry,
+        # raise that error even if the request was itself a retry.
         raise err.__class__, err, tb
 
 
diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py
index 648b38e1..605472a1 100644
--- a/src/allmydata/test/test_storage.py
+++ b/src/allmydata/test/test_storage.py
@@ -1831,7 +1831,7 @@ class ServerWithMockCloudBackend(WithMockCloudBackend, ServerTest, unittest.Test
         return getattr(LogEvent, 'LEVELMAP', {}).get(level, str(level))
 
     def _test_cloud_retry(self, name, failure_count, levels):
-        self.patch(cloud_common, 'BACKOFF_SECONDS_FOR_5XX', (0, 0.1, 0.2))
+        self.patch(cloud_common, 'BACKOFF_SECONDS_BEFORE_RETRY', (0, 0.1, 0.2))
 
         t = {'count': 0}
         old_put_object = MockContainer._put_object