feat: retry global cache operations on transient errors (#603)

Chris Rossi · web-flow · commit 2f261b71d7a5 · 2021-02-26T13:56:53.000-05:00
When an operation on the global cache is tried and there is a transient error, such as not being able to connect to a Redis or Memcached server, if the operation is being tried in strict mode, then the operation is now retried a number of times before the transient error is eventually raised to the application layer. Implements #601
diff --git a/packages/google-cloud-ndb/google/cloud/ndb/_cache.py b/packages/google-cloud-ndb/google/cloud/ndb/_cache.py
@@ -16,6 +16,8 @@
 import itertools
 import warnings
 
+from google.api_core import retry as core_retry
+
 from google.cloud.ndb import _batch
 from google.cloud.ndb import context as context_module
 from google.cloud.ndb import tasklets
@@ -132,9 +134,33 @@ def _handle_transient_errors(read=False):
     Will log as warning or reraise transient errors according to `strict_read` and
     `strict_write` attributes of the global cache and whether the operation is a read or
     a write.
+
+    If in strict mode, will retry the wrapped function up to 5 times before reraising
+    the transient error.
     """
 
     def wrap(wrapped):
+        def retry(wrapped, transient_errors):
+            @functools.wraps(wrapped)
+            @tasklets.tasklet
+            def retry_wrapper(*args, **kwargs):
+                sleep_generator = core_retry.exponential_sleep_generator(0.1, 1)
+                attempts = 5
+                for sleep_time in sleep_generator:  # pragma: NO BRANCH
+                    # pragma is required because loop never exits normally, it only gets
+                    # raised out of.
+                    attempts -= 1
+                    try:
+                        result = yield wrapped(*args, **kwargs)
+                        raise tasklets.Return(result)
+                    except transient_errors:
+                        if not attempts:
+                            raise
+
+                    yield tasklets.sleep(sleep_time)
+
+            return retry_wrapper
+
         @functools.wraps(wrapped)
         @tasklets.tasklet
         def wrapper(*args, **kwargs):
@@ -145,17 +171,22 @@ def wrapper(*args, **kwargs):
                     cache.clear()
                     cache.clear_cache_soon = False
 
-                result = yield wrapped(*args, **kwargs)
+                is_read = read
+                if not is_read:
+                    is_read = kwargs.get("read", False)
+
+                strict = cache.strict_read if is_read else cache.strict_write
+                if strict:
+                    function = retry(wrapped, cache.transient_errors)
+                else:
+                    function = wrapped
+
+                result = yield function(*args, **kwargs)
                 raise tasklets.Return(result)
 
             except cache.transient_errors as error:
                 cache.clear_cache_soon = True
 
-                strict_read = read
-                if not strict_read:
-                    strict_read = kwargs.get("read", False)
-                strict = cache.strict_read if strict_read else cache.strict_write
-
                 if strict:
                     raise
 
diff --git a/packages/google-cloud-ndb/google/cloud/ndb/global_cache.py b/packages/google-cloud-ndb/google/cloud/ndb/global_cache.py
@@ -49,11 +49,20 @@ class GlobalCache(object):
     Attributes:
         strict_read (bool): If :data:`False`, transient errors that occur as part of a
             entity lookup operation will be logged as warnings but not raised to the
-            application layer.
+            application layer. If :data:`True`, in the event of transient errors, cache
+            operations will be retried a number of times before eventually raising the
+            transient error to the application layer, if it does not resolve after
+            retrying. Setting this to :data:`True` will cause NDB operations to take
+            longer to complete if there are transient errors in the cache layer.
         strict_write (bool): If :data:`False`, transient errors that occur as part of
             a put or delete operation will be logged as warnings, but not raised to the
-            application layer. Setting this to :data:`True` somewhat increases the risk
-            that other clients might read stale data from the cache.
+            application layer. If :data:`True`, in the event of transient errors, cache
+            operations will be retried a number of times before eventually raising the
+            transient error to the application layer if it does not resolve after
+            retrying. Setting this to :data:`False` somewhat increases the risk
+            that other clients might read stale data from the cache. Setting this to
+            :data:`True` will cause NDB operations to take longer to complete if there
+            are transient errors in the cache layer.
     """
 
     __metaclass__ = abc.ABCMeta
@@ -243,8 +252,12 @@ class RedisCache(GlobalCache):
         strict_read (bool): If :data:`False`, connection errors during read operations
             will be logged with a warning and treated as cache misses, but will not
             raise an exception in the application, with connection errors during reads
-            being treated as cache misses.  If :data:`True`, connection errors will be
-            raised as exceptions in the application. Default: :data:`False`.
+            being treated as cache misses. If :data:`True`, in the event of connection
+            errors, cache operations will be retried a number of times before eventually
+            raising the connection error to the application layer, if it does not
+            resolve after retrying. Setting this to :data:`True` will cause NDB
+            operations to take longer to complete if there are transient errors in the
+            cache layer. Default: :data:`False`.
         strict_write (bool): If :data:`False`, connection errors during write
             operations will be logged with a warning, but will not raise an exception in
             the application. If :data:`True`, connection errors during write will be
@@ -253,7 +266,12 @@ class RedisCache(GlobalCache):
             retrieve stale data from the cache.  If there is a connection error, an
             internal flag will be set to clear the cache the next time any method is
             called on this object, to try and minimize the opportunity for clients to
-            read stale data from the cache.  Default: :data:`True`.
+            read stale data from the cache. If :data:`True`, in the event of connection
+            errors, cache operations will be retried a number of times before eventually
+            raising the connection error to the application layer, if it does not
+            resolve after retrying. Setting this to :data:`True` will cause NDB
+            operations to take longer to complete if there are transient errors in the
+            cache layer. Default: :data:`True`.
     """
 
     transient_errors = (
@@ -274,9 +292,12 @@ def from_environment(cls, strict_read=False, strict_write=True):
             strict_read (bool): If :data:`False`, connection errors during read
                 operations will be logged with a warning and treated as cache misses,
                 but will not raise an exception in the application, with connection
-                errors during reads being treated as cache misses.  If :data:`True`,
-                connection errors will be raised as exceptions in the application.
-                Default: :data:`False`.
+                errors during reads being treated as cache misses. If :data:`True`, in
+                the event of connection errors, cache operations will be retried a
+                number of times before eventually raising the connection error to the
+                application layer, if it does not resolve after retrying. Setting this
+                to :data:`True` will cause NDB operations to take longer to complete if
+                there are transient errors in the cache layer. Default: :data:`False`.
             strict_write (bool): If :data:`False`, connection errors during write
                 operations will be logged with a warning, but will not raise an
                 exception in the application. If :data:`True`, connection errors during
@@ -285,7 +306,12 @@ def from_environment(cls, strict_read=False, strict_write=True):
                 allow other clients to retrieve stale data from the cache.  If there is
                 a connection error, an internal flag will be set to clear the cache the
                 next time any method is called on this object, to try and minimize the
-                opportunity for clients to read stale data from the cache.  Default:
+                opportunity for clients to read stale data from the cache. If
+                :data:`True`, in the event of connection errors, cache operations will
+                be retried a number of times before eventually raising the connection
+                error to the application layer, if it does not resolve after retrying.
+                Setting this to :data:`True` will cause NDB operations to take longer to
+                complete if there are transient errors in the cache layer.  Default:
                 :data:`True`.
 
         Returns:
@@ -398,20 +424,30 @@ class MemcacheCache(GlobalCache):
 
     Args:
         client (pymemcache.Client): Instance of Memcache client to use.
-        strict_read (bool): If :data:`False`, connection errors during read operations
-            will be logged with a warning and treated as cache misses, but will not
-            raise an exception in the application, with connection errors during reads
-            being treated as cache misses.  If :data:`True`, connection errors will be
-            raised as exceptions in the application. Default: :data:`False`.
+        strict_read (bool): If :data:`False`, connection errors during read
+            operations will be logged with a warning and treated as cache misses,
+            but will not raise an exception in the application, with connection
+            errors during reads being treated as cache misses. If :data:`True`, in
+            the event of connection errors, cache operations will be retried a
+            number of times before eventually raising the connection error to the
+            application layer, if it does not resolve after retrying. Setting this
+            to :data:`True` will cause NDB operations to take longer to complete if
+            there are transient errors in the cache layer. Default: :data:`False`.
         strict_write (bool): If :data:`False`, connection errors during write
-            operations will be logged with a warning, but will not raise an exception in
-            the application. If :data:`True`, connection errors during write will be
-            raised as exceptions in the application. Because write operations involve
-            cache invalidation, setting this to :data:`False` may allow other clients to
-            retrieve stale data from the cache.  If there is a connection error, an
-            internal flag will be set to clear the cache the next time any method is
-            called on this object, to try and minimize the opportunity for clients to
-            read stale data from the cache.  Default: :data:`True`.
+            operations will be logged with a warning, but will not raise an
+            exception in the application. If :data:`True`, connection errors during
+            write will be raised as exceptions in the application. Because write
+            operations involve cache invalidation, setting this to :data:`False` may
+            allow other clients to retrieve stale data from the cache.  If there is
+            a connection error, an internal flag will be set to clear the cache the
+            next time any method is called on this object, to try and minimize the
+            opportunity for clients to read stale data from the cache. If
+            :data:`True`, in the event of connection errors, cache operations will
+            be retried a number of times before eventually raising the connection
+            error to the application layer, if it does not resolve after retrying.
+            Setting this to :data:`True` will cause NDB operations to take longer to
+            complete if there are transient errors in the cache layer. Default:
+            :data:`True`.
     """
 
     transient_errors = (
@@ -458,9 +494,12 @@ def from_environment(cls, max_pool_size=4, strict_read=False, strict_write=True)
             strict_read (bool): If :data:`False`, connection errors during read
                 operations will be logged with a warning and treated as cache misses,
                 but will not raise an exception in the application, with connection
-                errors during reads being treated as cache misses.  If :data:`True`,
-                connection errors will be raised as exceptions in the application.
-                Default: :data:`False`.
+                errors during reads being treated as cache misses. If :data:`True`, in
+                the event of connection errors, cache operations will be retried a
+                number of times before eventually raising the connection error to the
+                application layer, if it does not resolve after retrying. Setting this
+                to :data:`True` will cause NDB operations to take longer to complete if
+                there are transient errors in the cache layer. Default: :data:`False`.
             strict_write (bool): If :data:`False`, connection errors during write
                 operations will be logged with a warning, but will not raise an
                 exception in the application. If :data:`True`, connection errors during
@@ -469,7 +508,12 @@ def from_environment(cls, max_pool_size=4, strict_read=False, strict_write=True)
                 allow other clients to retrieve stale data from the cache.  If there is
                 a connection error, an internal flag will be set to clear the cache the
                 next time any method is called on this object, to try and minimize the
-                opportunity for clients to read stale data from the cache.  Default:
+                opportunity for clients to read stale data from the cache. If
+                :data:`True`, in the event of connection errors, cache operations will
+                be retried a number of times before eventually raising the connection
+                error to the application layer, if it does not resolve after retrying.
+                Setting this to :data:`True` will cause NDB operations to take longer to
+                complete if there are transient errors in the cache layer. Default:
                 :data:`True`.
 
         Returns:
diff --git a/packages/google-cloud-ndb/tests/unit/test__cache.py b/packages/google-cloud-ndb/tests/unit/test__cache.py
