[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