From 1518fd937bc4716b50280f7710939f04d0041283 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Fri, 31 Aug 2018 15:23:10 +0200 Subject: [PATCH 1/7] handle server errors with backoff --- commcare_export/commcare_hq_client.py | 25 ++++++++++++++++++++----- setup.py | 1 + 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/commcare_export/commcare_hq_client.py b/commcare_export/commcare_hq_client.py index 5b8234f3..dac42329 100644 --- a/commcare_export/commcare_hq_client.py +++ b/commcare_export/commcare_hq_client.py @@ -4,6 +4,7 @@ from collections import OrderedDict from copy import deepcopy +import backoff import requests from datetime import datetime from requests.auth import HTTPDigestAuth @@ -27,6 +28,18 @@ LATEST_KNOWN_VERSION='0.5' +def on_backoff(details): + logger.warn("Request failed after {tries} attempts. Waiting for retry.".format(**details)) + + +def on_giveup(details): + logger.warn("Request failed after {tries} attempts. Giving up.".format(**details)) + + +def is_client_error(ex): + return 400 <= ex.response.status_code < 500 + + class CommCareHqClient(object): """ A connection to CommCareHQ for a particular version, project, and user. @@ -67,6 +80,11 @@ def session(self, session): def api_url(self): return '%s/a/%s/api/v%s' % (self.url, self.project, self.version) + @backoff.on_exception( + backoff.expo, requests.exceptions.RequestException, + giveup=is_client_error, + on_backoff=on_backoff, on_giveup=on_giveup + ) def get(self, resource, params=None): """ Gets the named resource. @@ -78,11 +96,8 @@ def get(self, resource, params=None): logger.debug("Fetching batch: %s", params) resource_url = '%s/%s/' % (self.api_url, resource) response = self.session.get(resource_url, params=params, auth=self.__auth) - - if response.status_code != 200: - raise Exception('GET %s failed (%s): %s' % (resource_url, response.status_code, response.text)) - else: - return response.json() + response.raise_for_status() + return response.json() def iterate(self, resource, paginator, params=None): """ diff --git a/setup.py b/setup.py index 060c7d19..506fcafa 100644 --- a/setup.py +++ b/setup.py @@ -79,6 +79,7 @@ def run_tests(self): 'sqlalchemy', 'pytz', 'sqlalchemy-migrate', + 'backoff' ], tests_require = ['pytest', 'psycopg2', 'mock'], cmdclass = {'test': PyTest}, From 9068ad569488e4f2b5d46202120e5f12f7a1be0e Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Fri, 31 Aug 2018 15:23:25 +0200 Subject: [PATCH 2/7] nicer message for 401 --- commcare_export/cli.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/commcare_export/cli.py b/commcare_export/cli.py index 1215902e..add2f979 100644 --- a/commcare_export/cli.py +++ b/commcare_export/cli.py @@ -9,6 +9,7 @@ import sys import dateutil.parser +import requests from six.moves import input from commcare_export import excel_query @@ -257,11 +258,19 @@ def main_with_args(args): ) with env: - lazy_result = query.eval(env) - if lazy_result is not None: - # evaluate lazy results - for r in lazy_result: - list(r) if r else r + try: + lazy_result = query.eval(env) + if lazy_result is not None: + # evaluate lazy results + for r in lazy_result: + list(r) if r else r + except requests.exceptions.RequestException as e: + if e.response.status_code == 401: + print("\nAuthentication failed. Please check your credentials.") + return + else: + raise + if checkpoint_manager: checkpoint_manager.set_final_checkpoint() From 73fbc41ab9baa93a00266182d1e5d5b643dcd6fe Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Fri, 31 Aug 2018 15:23:35 +0200 Subject: [PATCH 3/7] disable backoff logging --- commcare_export/cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/commcare_export/cli.py b/commcare_export/cli.py index add2f979..283c34fc 100644 --- a/commcare_export/cli.py +++ b/commcare_export/cli.py @@ -103,6 +103,7 @@ def main(argv): format='%(asctime)s %(name)-12s %(levelname)-8s %(message)s') logging.getLogger('alembic').setLevel(logging.WARN) + logging.getLogger('backoff').setLevel(logging.FATAL) if args.version: print('commcare-export version {}'.format(__version__)) From b8b71893c901b1d99d06d117475e89d066e8edc3 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Fri, 31 Aug 2018 15:35:19 +0200 Subject: [PATCH 4/7] better logging --- commcare_export/commcare_hq_client.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/commcare_export/commcare_hq_client.py b/commcare_export/commcare_hq_client.py index dac42329..14c66961 100644 --- a/commcare_export/commcare_hq_client.py +++ b/commcare_export/commcare_hq_client.py @@ -2,13 +2,11 @@ import logging from collections import OrderedDict -from copy import deepcopy import backoff import requests -from datetime import datetime -from requests.auth import HTTPDigestAuth from requests.auth import AuthBase +from requests.auth import HTTPDigestAuth AUTH_MODE_DIGEST = 'digest' AUTH_MODE_APIKEY = 'apikey' @@ -29,15 +27,22 @@ def on_backoff(details): - logger.warn("Request failed after {tries} attempts. Waiting for retry.".format(**details)) + _log_backoff(details, 'Waiting for retry.') def on_giveup(details): - logger.warn("Request failed after {tries} attempts. Giving up.".format(**details)) + _log_backoff(details, 'Giving up.') + + +def _log_backoff(details, action_message): + details['__suffix'] = action_message + logger.warn("Request failed after {tries} attempts ({elapsed:.1f}s). {__suffix}".format(**details)) def is_client_error(ex): - return 400 <= ex.response.status_code < 500 + if hasattr(ex, 'response') and ex.response: + return 400 <= ex.response.status_code < 500 + return False class CommCareHqClient(object): From 80cbfbf17288450fe82752a0038283adaae2401a Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Fri, 31 Aug 2018 15:36:05 +0200 Subject: [PATCH 5/7] add timeout to requests --- commcare_export/commcare_hq_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commcare_export/commcare_hq_client.py b/commcare_export/commcare_hq_client.py index 14c66961..87c0c357 100644 --- a/commcare_export/commcare_hq_client.py +++ b/commcare_export/commcare_hq_client.py @@ -100,7 +100,7 @@ def get(self, resource, params=None): """ logger.debug("Fetching batch: %s", params) resource_url = '%s/%s/' % (self.api_url, resource) - response = self.session.get(resource_url, params=params, auth=self.__auth) + response = self.session.get(resource_url, params=params, auth=self.__auth, timeout=60) response.raise_for_status() return response.json() From 35a01d13e6db1431d4b76058a7e1fe663b2d6a6c Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Fri, 31 Aug 2018 15:47:45 +0200 Subject: [PATCH 6/7] fix tests --- tests/test_commcare_hq_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_commcare_hq_client.py b/tests/test_commcare_hq_client.py index 7e13354a..b367e500 100644 --- a/tests/test_commcare_hq_client.py +++ b/tests/test_commcare_hq_client.py @@ -13,7 +13,7 @@ class FakeSession(object): - def get(self, resource_url, params=None, auth=None): + def get(self, resource_url, params=None, auth=None, timeout=None): result = self._get_results(params) # Mutatey construction method required by requests.Response response = requests.Response() From 69fa228827408e3ac824aa3aeb1805b277be72f4 Mon Sep 17 00:00:00 2001 From: Simon Kelly Date: Fri, 31 Aug 2018 15:51:00 +0200 Subject: [PATCH 7/7] five up after 5 mins This equates to roughly 9 attempts with exponential backoff --- commcare_export/commcare_hq_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commcare_export/commcare_hq_client.py b/commcare_export/commcare_hq_client.py index 87c0c357..9c0c0fcf 100644 --- a/commcare_export/commcare_hq_client.py +++ b/commcare_export/commcare_hq_client.py @@ -87,7 +87,7 @@ def api_url(self): @backoff.on_exception( backoff.expo, requests.exceptions.RequestException, - giveup=is_client_error, + max_time=300, giveup=is_client_error, on_backoff=on_backoff, on_giveup=on_giveup ) def get(self, resource, params=None):