[ARVADOS] updated: 952bfa87465a27f83dca7feca7d369fda4200eb5
git at public.curoverse.com
git at public.curoverse.com
Thu Jan 15 12:01:56 EST 2015
Summary of changes:
sdk/python/arvados/errors.py | 47 +++++++++++++++++++++++--
sdk/python/arvados/keep.py | 40 +++++++++++++++------
sdk/python/tests/test_errors.py | 68 ++++++++++++++++++++++++++++++++++++
sdk/python/tests/test_keep_client.py | 65 ++++++++++++++++++++++++++++++++++
4 files changed, 207 insertions(+), 13 deletions(-)
create mode 100644 sdk/python/tests/test_errors.py
via 952bfa87465a27f83dca7feca7d369fda4200eb5 (commit)
via 1acdfd97cb761eed9357b8c0e59ab80dc56c2652 (commit)
via 079a14ebb221f2f0b4aa9eca266cc3efb8eb0150 (commit)
from 212e68e9df1ee2bf2b9642d87452e4520532f84d (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 952bfa87465a27f83dca7feca7d369fda4200eb5
Merge: 212e68e 1acdfd9
Author: Brett Smith <brett at curoverse.com>
Date: Thu Jan 15 12:00:13 2015 -0500
Merge branch '3835-pysdk-keep-exceptions-wip'
Closes #3835, #4972.
commit 1acdfd97cb761eed9357b8c0e59ab80dc56c2652
Author: Brett Smith <brett at curoverse.com>
Date: Wed Jan 14 15:22:24 2015 -0500
3835: PySDK raises NotFoundError when all Keep services report such.
Previously, we raised this error when >= 75% of services reported
such, as the most reasonable available cutoff to make the
distinction. Now that Keep exceptions include detailed information
about the error from each service, it seems useful to make this
threshold stricter, and only raise NotFoundError when we're sure
that's the problem. See further discussion from
<https://arvados.org/issues/3835#note-11>.
diff --git a/sdk/python/arvados/keep.py b/sdk/python/arvados/keep.py
index 471544c..7c53339 100644
--- a/sdk/python/arvados/keep.py
+++ b/sdk/python/arvados/keep.py
@@ -643,25 +643,22 @@ class KeepClient(object):
if loop.success():
return blob
- # No servers fulfilled the request. Count how many responded
- # "not found;" if the ratio is high enough (currently 75%), report
- # Not Found; otherwise a generic error.
- # Q: Including 403 is necessary for the Keep tests to continue
- # passing, but maybe they should expect KeepReadError instead?
- not_founds = sum(1 for ks in roots_map.values()
- if ks.last_status() in set([403, 404, 410]))
try:
all_roots = local_roots + hint_roots
except NameError:
# We never successfully fetched local_roots.
all_roots = hint_roots
+ # Q: Including 403 is necessary for the Keep tests to continue
+ # passing, but maybe they should expect KeepReadError instead?
+ not_founds = sum(1 for key in all_roots
+ if roots_map[key].last_status() in {403, 404, 410})
service_errors = ((key, roots_map[key].last_result)
for key in all_roots)
if not roots_map:
raise arvados.errors.KeepReadError(
"failed to read {}: no Keep services available ({})".format(
loc_s, loop.last_result()))
- elif ((float(not_founds) / len(roots_map)) >= .75):
+ elif not_founds == len(all_roots):
raise arvados.errors.NotFoundError(
"{} not found".format(loc_s), service_errors)
else:
commit 079a14ebb221f2f0b4aa9eca266cc3efb8eb0150
Author: Brett Smith <brett at curoverse.com>
Date: Fri Jan 9 13:15:07 2015 -0500
3835: Improve error reporting in PySDK Keep client.
* Create a KeepRequestError base exception to store information about
errors encountered when talking to Keep services, and include those
in the default formatting.
* Include Keep service error information in exceptions raised by the
Keep client.
diff --git a/sdk/python/arvados/errors.py b/sdk/python/arvados/errors.py
index 4740a2d..f70fa17 100644
--- a/sdk/python/arvados/errors.py
+++ b/sdk/python/arvados/errors.py
@@ -1,7 +1,10 @@
# errors.py - Arvados-specific exceptions.
import json
+import requests
+
from apiclient import errors as apiclient_errors
+from collections import OrderedDict
class ApiError(apiclient_errors.HttpError):
def _get_reason(self):
@@ -11,6 +14,46 @@ class ApiError(apiclient_errors.HttpError):
return super(ApiError, self)._get_reason()
+class KeepRequestError(Exception):
+ """Base class for errors accessing Keep services."""
+ def __init__(self, message='', service_errors=()):
+ """KeepRequestError(message='', service_errors=())
+
+ Arguments:
+ * message: A human-readable message describing what Keep operation
+ failed.
+ * service_errors: An iterable that yields 2-tuples of Keep
+ service URLs to the error encountered when talking to
+ it--either an exception, or an HTTP response object. These
+ will be packed into an OrderedDict, available through the
+ service_errors() method.
+ """
+ self._service_errors = OrderedDict(service_errors)
+ if self._service_errors:
+ exc_reports = [self._format_error(*err_pair)
+ for err_pair in self._service_errors.iteritems()]
+ base_msg = "{}: {}".format(message, "; ".join(exc_reports))
+ else:
+ base_msg = message
+ super(KeepRequestError, self).__init__(base_msg)
+ self.message = message
+
+ def _format_error(self, service_root, error):
+ if isinstance(error, requests.Response):
+ err_fmt = "{} responded with {e.status_code} {e.reason}"
+ else:
+ err_fmt = "{} raised {e.__class__.__name__} ({e})"
+ return err_fmt.format(service_root, e=error)
+
+ def service_errors(self):
+ """service_errors() -> OrderedDict
+
+ The keys of the dictionary are Keep service URLs.
+ The corresponding value is the exception raised when sending the
+ request to it."""
+ return self._service_errors
+
+
class ArgumentError(Exception):
pass
class SyntaxError(Exception):
@@ -19,9 +62,9 @@ class AssertionError(Exception):
pass
class CommandFailedError(Exception):
pass
-class KeepReadError(Exception):
+class KeepReadError(KeepRequestError):
pass
-class KeepWriteError(Exception):
+class KeepWriteError(KeepRequestError):
pass
class NotFoundError(KeepReadError):
pass
diff --git a/sdk/python/arvados/keep.py b/sdk/python/arvados/keep.py
index f4c8596..471544c 100644
--- a/sdk/python/arvados/keep.py
+++ b/sdk/python/arvados/keep.py
@@ -650,10 +650,23 @@ class KeepClient(object):
# passing, but maybe they should expect KeepReadError instead?
not_founds = sum(1 for ks in roots_map.values()
if ks.last_status() in set([403, 404, 410]))
- if roots_map and ((float(not_founds) / len(roots_map)) >= .75):
- raise arvados.errors.NotFoundError(loc_s)
+ try:
+ all_roots = local_roots + hint_roots
+ except NameError:
+ # We never successfully fetched local_roots.
+ all_roots = hint_roots
+ service_errors = ((key, roots_map[key].last_result)
+ for key in all_roots)
+ if not roots_map:
+ raise arvados.errors.KeepReadError(
+ "failed to read {}: no Keep services available ({})".format(
+ loc_s, loop.last_result()))
+ elif ((float(not_founds) / len(roots_map)) >= .75):
+ raise arvados.errors.NotFoundError(
+ "{} not found".format(loc_s), service_errors)
else:
- raise arvados.errors.KeepReadError(loc_s)
+ raise arvados.errors.KeepReadError(
+ "failed to read {}".format(loc_s), service_errors)
@retry.retry_method
def put(self, data, copies=2, num_retries=None):
@@ -714,9 +727,17 @@ class KeepClient(object):
if loop.success():
return thread_limiter.response()
- raise arvados.errors.KeepWriteError(
- "Write fail for %s: wanted %d but wrote %d" %
- (data_hash, copies, thread_limiter.done()))
+ if not roots_map:
+ raise arvados.errors.KeepWriteError(
+ "failed to write {}: no Keep services available ({})".format(
+ data_hash, loop.last_result()))
+ else:
+ service_errors = ((key, roots_map[key].last_result)
+ for key in local_roots
+ if not roots_map[key].success_flag)
+ raise arvados.errors.KeepWriteError(
+ "failed to write {} (wanted {} copies but wrote {})".format(
+ data_hash, copies, thread_limiter.done()), service_errors)
# Local storage methods need no-op num_retries arguments to keep
# integration tests happy. With better isolation they could
diff --git a/sdk/python/tests/test_errors.py b/sdk/python/tests/test_errors.py
new file mode 100644
index 0000000..cf06c84
--- /dev/null
+++ b/sdk/python/tests/test_errors.py
@@ -0,0 +1,68 @@
+#!/usr/bin/env python
+
+import traceback
+import unittest
+
+import arvados.errors as arv_error
+import arvados_testutil as tutil
+
+class KeepRequestErrorTestCase(unittest.TestCase):
+ SERVICE_ERRORS = [
+ ('http://keep1.zzzzz.example.org/', IOError("test IOError")),
+ ('http://keep3.zzzzz.example.org/', MemoryError("test MemoryError")),
+ ('http://keep5.zzzzz.example.org/', tutil.fake_requests_response(
+ 500, "test 500")),
+ ('http://keep7.zzzzz.example.org/', IOError("second test IOError")),
+ ]
+
+ def check_get_message(self, *args):
+ test_exc = arv_error.KeepRequestError("test message", *args)
+ self.assertEqual("test message", test_exc.message)
+
+ def test_get_message_with_service_errors(self):
+ self.check_get_message(self.SERVICE_ERRORS[:])
+
+ def test_get_message_without_service_errors(self):
+ self.check_get_message()
+
+ def check_get_service_errors(self, *args):
+ expected = dict(args[0]) if args else {}
+ test_exc = arv_error.KeepRequestError("test service exceptions", *args)
+ self.assertEqual(expected, test_exc.service_errors())
+
+ def test_get_service_errors(self):
+ self.check_get_service_errors(self.SERVICE_ERRORS[:])
+
+ def test_get_service_errors_none(self):
+ self.check_get_service_errors({})
+
+ def test_empty_exception(self):
+ test_exc = arv_error.KeepRequestError()
+ self.assertFalse(test_exc.message)
+ self.assertEqual({}, test_exc.service_errors())
+
+ def traceback_str(self, exc):
+ return traceback.format_exception_only(type(exc), exc)[-1]
+
+ def test_traceback_str_without_service_errors(self):
+ message = "test plain traceback string"
+ test_exc = arv_error.KeepRequestError(message)
+ exc_report = self.traceback_str(test_exc)
+ self.assertTrue(exc_report.startswith("KeepRequestError: "))
+ self.assertIn(message, exc_report)
+
+ def test_traceback_str_with_service_errors(self):
+ message = "test traceback shows Keep services"
+ test_exc = arv_error.KeepRequestError(message, self.SERVICE_ERRORS[:])
+ exc_report = self.traceback_str(test_exc)
+ self.assertTrue(exc_report.startswith("KeepRequestError: "))
+ for expect_substr in [message, "raised IOError", "raised MemoryError",
+ "test MemoryError", "second test IOError",
+ "responded with 500 Internal Server Error"]:
+ self.assertIn(expect_substr, exc_report)
+ # Assert the report maintains order of listed services.
+ last_index = -1
+ for service_key, _ in self.SERVICE_ERRORS:
+ service_index = exc_report.find(service_key)
+ self.assertGreater(service_index, last_index)
+ last_index = service_index
diff --git a/sdk/python/tests/test_keep_client.py b/sdk/python/tests/test_keep_client.py
index 982e4b4..37f274a 100644
--- a/sdk/python/tests/test_keep_client.py
+++ b/sdk/python/tests/test_keep_client.py
@@ -394,6 +394,71 @@ class KeepClientServiceTestCase(unittest.TestCase):
min_penalty,
max_penalty))
+ def check_64_zeros_error_order(self, verb, exc_class):
+ data = '0' * 64
+ if verb == 'get':
+ data = hashlib.md5(data).hexdigest() + '+1234'
+ api_client = self.mock_n_keep_disks(16)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ with mock.patch('requests.' + verb,
+ side_effect=socket.timeout) as req_mock, \
+ self.assertRaises(exc_class) as err_check:
+ getattr(keep_client, verb)(data)
+ urls = [urlparse.urlparse(url)
+ for url in err_check.exception.service_errors()]
+ self.assertEqual([('keep0x' + c, 80) for c in '3eab2d5fc9681074'],
+ [(url.hostname, url.port) for url in urls])
+
+ def test_get_error_shows_probe_order(self):
+ self.check_64_zeros_error_order('get', arvados.errors.KeepReadError)
+
+ def test_put_error_shows_probe_order(self):
+ self.check_64_zeros_error_order('put', arvados.errors.KeepWriteError)
+
+ def check_no_services_error(self, verb, exc_class):
+ api_client = mock.MagicMock(name='api_client')
+ api_client.keep_services().accessible().execute.side_effect = (
+ arvados.errors.ApiError)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ with self.assertRaises(exc_class) as err_check:
+ getattr(keep_client, verb)('d41d8cd98f00b204e9800998ecf8427e+0')
+ self.assertEqual(0, len(err_check.exception.service_errors()))
+
+ def test_get_error_with_no_services(self):
+ self.check_no_services_error('get', arvados.errors.KeepReadError)
+
+ def test_put_error_with_no_services(self):
+ self.check_no_services_error('put', arvados.errors.KeepWriteError)
+
+ def check_errors_from_last_retry(self, verb, exc_class):
+ api_client = self.mock_n_keep_disks(2)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ req_mock = getattr(tutil, 'mock_{}_responses'.format(verb))(
+ "retry error reporting test", 500, 500, 403, 403)
+ with req_mock, tutil.skip_sleep, \
+ self.assertRaises(exc_class) as err_check:
+ getattr(keep_client, verb)('d41d8cd98f00b204e9800998ecf8427e+0',
+ num_retries=3)
+ self.assertEqual([403, 403], [
+ getattr(error, 'status_code', None)
+ for error in err_check.exception.service_errors().itervalues()])
+
+ def test_get_error_reflects_last_retry(self):
+ self.check_errors_from_last_retry('get', arvados.errors.KeepReadError)
+
+ def test_put_error_reflects_last_retry(self):
+ self.check_errors_from_last_retry('put', arvados.errors.KeepWriteError)
+
+ def test_put_error_does_not_include_successful_puts(self):
+ data = 'partial failure test'
+ data_loc = '{}+{}'.format(hashlib.md5(data).hexdigest(), len(data))
+ api_client = self.mock_n_keep_disks(3)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ with tutil.mock_put_responses(data_loc, 200, 500, 500) as req_mock, \
+ self.assertRaises(arvados.errors.KeepWriteError) as exc_check:
+ keep_client.put(data)
+ self.assertEqual(2, len(exc_check.exception.service_errors()))
+
class KeepClientRetryTestMixin(object):
# Testing with a local Keep store won't exercise the retry behavior.
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list