[ARVADOS] updated: 69594694568da1abc2bd8e46134da81ec1f2a7c8

git at public.curoverse.com git at public.curoverse.com
Tue Nov 11 15:19:22 EST 2014


Summary of changes:
 sdk/python/arvados/keep.py           | 36 +++++++++++-------
 sdk/python/tests/test_collections.py |  2 +-
 sdk/python/tests/test_keep_client.py | 73 ++++++++++++++++++++++--------------
 3 files changed, 68 insertions(+), 43 deletions(-)

       via  69594694568da1abc2bd8e46134da81ec1f2a7c8 (commit)
      from  1fa1006e5b21cfc30e694890db969c20864af8c6 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 69594694568da1abc2bd8e46134da81ec1f2a7c8
Author: Tim Pierce <twp at curoverse.com>
Date:   Tue Nov 11 11:04:19 2014 -0500

    3857: code review feedback
    
    * Set default timeouts to (20, 300) for proxies and (2, 300) for all
      else.
    * KeepService:
    ** Removed unused 'timeout' argument to __init__()
    ** Trimmed unnecessary exceptions in last_status()
    * Tests:
    ** test_collections.py: changed mock_api_call return value back to
       fake_httplib2_response
    ** Flattened test layers to eliminate KeepClientRetryGetTestMixin and
       KeepClientRetryPutTestMixin
    ** Moved proxy timeout tests to
       KeepClientServiceTestCase.test_proxy_get_request and
       .test_proxy_put_request
    * Removed excess blank lines in docstrings

diff --git a/sdk/python/arvados/keep.py b/sdk/python/arvados/keep.py
index d927580..1fd8fa5 100644
--- a/sdk/python/arvados/keep.py
+++ b/sdk/python/arvados/keep.py
@@ -210,12 +210,12 @@ class KeepBlockCache(object):
 
 class KeepClient(object):
 
-    # Default Keep server connection timeout:  3 seconds
-    # Default Keep server read timeout:       30 seconds
+    # Default Keep server connection timeout:  2 seconds
+    # Default Keep server read timeout:      300 seconds
     # Default Keep proxy connection timeout:  20 seconds
-    # Default Keep proxy read timeout:        60 seconds
-    DEFAULT_TIMEOUT = (3, 30)
-    DEFAULT_PROXY_TIMEOUT = (20, 60)
+    # Default Keep proxy read timeout:       300 seconds
+    DEFAULT_TIMEOUT = (2, 300)
+    DEFAULT_PROXY_TIMEOUT = (20, 300)
 
     class ThreadLimiter(object):
         """
@@ -279,7 +279,7 @@ class KeepClient(object):
         HTTP_ERRORS = (requests.exceptions.RequestException,
                        socket.error, ssl.SSLError)
 
-        def __init__(self, root, timeout=None, **headers):
+        def __init__(self, root, **headers):
             self.root = root
             self.last_result = None
             self.success_flag = None
@@ -296,7 +296,7 @@ class KeepClient(object):
         def last_status(self):
             try:
                 return self.last_result.status_code
-            except (AttributeError, IndexError, ValueError):
+            except AttributeError:
                 return None
 
         def get(self, locator, timeout=None):
@@ -419,10 +419,10 @@ class KeepClient(object):
           non-proxy servers.  A tuple of two floats is interpreted as
           (connection_timeout, read_timeout): see
           http://docs.python-requests.org/en/latest/user/advanced/#timeouts.
-          Default: (3, 30).
+          Default: (2, 300).
         * proxy_timeout: The timeout (in seconds) for HTTP requests to
           Keep proxies. A tuple of two floats is interpreted as
-          (connection_timeout, read_timeout). Default: (20, 60).
+          (connection_timeout, read_timeout). Default: (20, 300).
         * api_token: If you're not using an API client, but only talking
           directly to a Keep proxy, this parameter specifies an API token
           to authenticate Keep requests.  It is an error to specify both
@@ -437,7 +437,6 @@ class KeepClient(object):
         * num_retries: The default number of times to retry failed requests.
           This will be used as the default num_retries value when get() and
           put() are called.  Default 0.
-
         """
         self.lock = threading.Lock()
         if proxy is None:
@@ -454,6 +453,8 @@ class KeepClient(object):
             local_store = os.environ.get('KEEP_LOCAL_STORE')
 
         self.block_cache = block_cache if block_cache else KeepBlockCache()
+        self.timeout = timeout
+        self.proxy_timeout = proxy_timeout
 
         if local_store:
             self.local_store = local_store
@@ -467,7 +468,6 @@ class KeepClient(object):
                 self.api_token = api_token
                 self.service_roots = [proxy]
                 self.using_proxy = True
-                self.timeout = proxy_timeout
                 self.static_service_roots = True
             else:
                 # It's important to avoid instantiating an API client
@@ -478,9 +478,17 @@ class KeepClient(object):
                 self.api_token = api_client.api_token
                 self.service_roots = None
                 self.using_proxy = None
-                self.timeout = timeout
                 self.static_service_roots = False
 
+    def current_timeout(self):
+        """Return the appropriate timeout to use for this client: the proxy
+        timeout setting if the backend service is currently a proxy,
+        the regular timeout setting otherwise.
+        """
+        # TODO(twp): the timeout should be a property of a
+        # KeepService, not a KeepClient. See #4488.
+        return self.proxy_timeout if self.using_proxy else self.timeout
+
     def build_service_roots(self, force_rebuild=False):
         if (self.static_service_roots or
               (self.service_roots and not force_rebuild)):
@@ -642,7 +650,7 @@ class KeepClient(object):
                                for root in (local_roots + hint_roots)
                                if roots_map[root].usable()]
             for keep_service in services_to_try:
-                blob = keep_service.get(locator, timeout=self.timeout)
+                blob = keep_service.get(locator, timeout=self.current_timeout())
                 if blob is not None:
                     break
             loop.save_result((blob, len(services_to_try)))
@@ -715,7 +723,7 @@ class KeepClient(object):
                     data_hash=data_hash,
                     service_root=service_root,
                     thread_limiter=thread_limiter,
-                    timeout=self.timeout)
+                    timeout=self.current_timeout())
                 t.start()
                 threads.append(t)
             for t in threads:
diff --git a/sdk/python/tests/test_collections.py b/sdk/python/tests/test_collections.py
index a179c2b..fb29881 100644
--- a/sdk/python/tests/test_collections.py
+++ b/sdk/python/tests/test_collections.py
@@ -691,7 +691,7 @@ class CollectionTestMixin(object):
             mock_method.return_value = body
         else:
             mock_method.side_effect = arvados.errors.ApiError(
-                tutil.fake_requests_response(code, None), "{}")
+                tutil.fake_httplib2_response(code), "{}")
 
     def mock_keep_services(self, api_mock, code, body):
         self._mock_api_call(api_mock.keep_services().accessible, code, body)
diff --git a/sdk/python/tests/test_keep_client.py b/sdk/python/tests/test_keep_client.py
index feb0dd5..96522d4 100644
--- a/sdk/python/tests/test_keep_client.py
+++ b/sdk/python/tests/test_keep_client.py
@@ -293,6 +293,36 @@ class KeepClientServiceTestCase(unittest.TestCase):
                 arvados.KeepClient.DEFAULT_TIMEOUT,
                 mock_request.call_args[1]['timeout'])
 
+    def test_proxy_get_timeout(self):
+        # Force a timeout, verifying that the requests.get or
+        # requests.put method was called with the proxy_timeout
+        # setting rather than the default timeout.
+        api_client = self.mock_keep_services(('keep', 10, False, 'proxy'))
+        keep_client = arvados.KeepClient(api_client=api_client)
+        force_timeout = [socket.timeout("timed out")]
+        with mock.patch('requests.get', side_effect=force_timeout) as mock_request:
+            with self.assertRaises(arvados.errors.KeepReadError):
+                keep_client.get('ffffffffffffffffffffffffffffffff')
+            self.assertTrue(mock_request.called)
+            self.assertEqual(
+                arvados.KeepClient.DEFAULT_PROXY_TIMEOUT,
+                mock_request.call_args[1]['timeout'])
+
+    def test_proxy_put_timeout(self):
+        # Force a timeout, verifying that the requests.get or
+        # requests.put method was called with the proxy_timeout
+        # setting rather than the default timeout.
+        api_client = self.mock_keep_services(('keep', 10, False, 'proxy'))
+        keep_client = arvados.KeepClient(api_client=api_client)
+        force_timeout = [socket.timeout("timed out")]
+        with mock.patch('requests.put', side_effect=force_timeout) as mock_request:
+            with self.assertRaises(arvados.errors.KeepWriteError):
+                keep_client.put('foo')
+            self.assertTrue(mock_request.called)
+            self.assertEqual(
+                arvados.KeepClient.DEFAULT_PROXY_TIMEOUT,
+                mock_request.call_args[1]['timeout'])
+
 
 class KeepClientRetryTestMixin(object):
     # Testing with a local Keep store won't exercise the retry behavior.
@@ -305,6 +335,11 @@ class KeepClientRetryTestMixin(object):
     # supporting servers, and prevents side effects in case something hiccups.
     # To use this mixin, define DEFAULT_EXPECT, DEFAULT_EXCEPTION, and
     # run_method().
+    #
+    # Test classes must set TEST_PATCHER to a static method that mocks
+    # out appropriate methods in the client --
+    # e.g. tutil.mock_get_responses or tutil.mock_put_responses.
+
     PROXY_ADDR = 'http://[%s]:65535/' % (tutil.TEST_HOST,)
     TEST_DATA = 'testdata'
     TEST_LOCATOR = 'ef654c40ab4f1747fc699915d4f70902+8'
@@ -331,56 +366,37 @@ class KeepClientRetryTestMixin(object):
         self.assertRaises(error_class, self.run_method, *args, **kwargs)
 
     def test_immediate_success(self):
-        with tutil.mock_requestslib_responses(self.mock_method, self.DEFAULT_EXPECT, 200):
+        with self.TEST_PATCHER(self.DEFAULT_EXPECT, 200):
             self.check_success()
 
     def test_retry_then_success(self):
-        with tutil.mock_requestslib_responses(self.mock_method, self.DEFAULT_EXPECT, 500, 200):
+        with self.TEST_PATCHER(self.DEFAULT_EXPECT, 500, 200):
             self.check_success(num_retries=3)
 
     def test_no_default_retry(self):
-        with tutil.mock_requestslib_responses(self.mock_method, self.DEFAULT_EXPECT, 500, 200):
+        with self.TEST_PATCHER(self.DEFAULT_EXPECT, 500, 200):
             self.check_exception()
 
     def test_no_retry_after_permanent_error(self):
-        with tutil.mock_requestslib_responses(self.mock_method, self.DEFAULT_EXPECT, 403, 200):
+        with self.TEST_PATCHER(self.DEFAULT_EXPECT, 403, 200):
             self.check_exception(num_retries=3)
 
     def test_error_after_retries_exhausted(self):
-        with tutil.mock_requestslib_responses(self.mock_method, self.DEFAULT_EXPECT, 500, 500, 200):
+        with self.TEST_PATCHER(self.DEFAULT_EXPECT, 500, 500, 200):
             self.check_exception(num_retries=1)
 
     def test_num_retries_instance_fallback(self):
         self.client_kwargs['num_retries'] = 3
-        with tutil.mock_requestslib_responses(self.mock_method, self.DEFAULT_EXPECT, 500, 200):
+        with self.TEST_PATCHER(self.DEFAULT_EXPECT, 500, 200):
             self.check_success()
 
-    def test_proxy_timeout(self):
-        # Force a timeout, verifying that the requests.get or
-        # requests.put method was called with the proxy_timeout
-        # setting rather than the default timeout.
-        force_timeout = [socket.timeout("timed out")]
-        with mock.patch(self.mock_method, side_effect=force_timeout) as mock_request:
-            self.check_exception()
-            self.assertTrue(mock_request.called)
-            self.assertEqual(
-                arvados.KeepClient.DEFAULT_PROXY_TIMEOUT,
-                mock_request.call_args[1]['timeout'])
-
-
-class KeepClientRetryGetTestMixin(KeepClientRetryTestMixin):
-    mock_method = 'requests.get'
-
-
-class KeepClientRetryPutTestMixin(KeepClientRetryTestMixin):
-    mock_method = 'requests.put'
-
 
 @tutil.skip_sleep
-class KeepClientRetryGetTestCase(KeepClientRetryGetTestMixin, unittest.TestCase):
+class KeepClientRetryGetTestCase(KeepClientRetryTestMixin, unittest.TestCase):
     DEFAULT_EXPECT = KeepClientRetryTestMixin.TEST_DATA
     DEFAULT_EXCEPTION = arvados.errors.KeepReadError
     HINTED_LOCATOR = KeepClientRetryTestMixin.TEST_LOCATOR + '+K at xyzzy'
+    TEST_PATCHER = staticmethod(tutil.mock_get_responses)
 
     def run_method(self, locator=KeepClientRetryTestMixin.TEST_LOCATOR,
                    *args, **kwargs):
@@ -423,9 +439,10 @@ class KeepClientRetryGetTestCase(KeepClientRetryGetTestMixin, unittest.TestCase)
 
 
 @tutil.skip_sleep
-class KeepClientRetryPutTestCase(KeepClientRetryPutTestMixin, unittest.TestCase):
+class KeepClientRetryPutTestCase(KeepClientRetryTestMixin, unittest.TestCase):
     DEFAULT_EXPECT = KeepClientRetryTestMixin.TEST_LOCATOR
     DEFAULT_EXCEPTION = arvados.errors.KeepWriteError
+    TEST_PATCHER = staticmethod(tutil.mock_put_responses)
 
     def run_method(self, data=KeepClientRetryTestMixin.TEST_DATA,
                    copies=1, *args, **kwargs):

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list