Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry requests #83

Merged
merged 10 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions kbcstorage/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
.. _Storage API documentation:
http://docs.keboola.apiary.io/
"""
from . import retry_requests
import requests


Expand Down Expand Up @@ -66,7 +67,7 @@ def _get_raw(self, url, params=None, **kwargs):
headers = kwargs.pop('headers', {})
headers.update(self._auth_header)

r = requests.get(url, params, headers=headers, **kwargs)
r = retry_requests.get(url, params=params, headers=headers, **kwargs)
try:
r.raise_for_status()
except requests.HTTPError:
Expand Down Expand Up @@ -111,7 +112,7 @@ def _post(self, *args, **kwargs):
"""
headers = kwargs.pop('headers', {})
headers.update(self._auth_header)
r = requests.post(headers=headers, *args, **kwargs)
r = retry_requests.post(headers=headers, *args, **kwargs)
try:
r.raise_for_status()
except requests.HTTPError:
Expand All @@ -137,7 +138,7 @@ def _put(self, *args, **kwargs):
"""
headers = kwargs.pop('headers', {})
headers.update(self._auth_header)
r = requests.put(headers=headers, *args, **kwargs)
r = retry_requests.put(headers=headers, *args, **kwargs)
try:
r.raise_for_status()
except requests.HTTPError:
Expand All @@ -163,7 +164,7 @@ def _delete(self, *args, **kwargs):
"""
headers = kwargs.pop('headers', {})
headers.update(self._auth_header)
r = requests.delete(headers=headers, *args, **kwargs)
r = retry_requests.delete(headers=headers, *args, **kwargs)
try:
r.raise_for_status()
except requests.HTTPError:
Expand Down
29 changes: 29 additions & 0 deletions kbcstorage/retry_requests.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import time
import requests

MAX_RETRIES = 11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 10 retries? That means 17 minutes request if the service is down completely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have 10 retries in SAPI, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tak jsme to rozmotali, jak to je. Zásadní totiž je, že v php clientovi je to konfigurovatelné. Tím pádem tam, kde je to nějaký worker, který běží nekonečně je použitý default nebo i víc. A pak když je to používané někde v rámci requestu, tak je to zkrácené třeba na 3 pokusy. Tak prosím tady stejně. Tj. konfigurovatelné parametrem konstruktoru jako v té PHP verzi.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hotovson

BACKOFF_FACTOR = 1.0

def _get_backoff_time(retry_count):
tomasfejfar marked this conversation as resolved.
Show resolved Hide resolved
return BACKOFF_FACTOR * (2 ** retry_count)

def _retry_request(request_func, url, *args, **kwargs):
response = request_func(url, *args, **kwargs)
for retry_count in range(MAX_RETRIES - 1):
if response.status_code == 501 or response.status_code < 500:
return response
time.sleep(_get_backoff_time(retry_count))
response = request_func(url, **kwargs)
return response

def get(url, *args, **kwargs):
return _retry_request(requests.get, url, *args, **kwargs)

def post(url, *args, **kwargs):
return _retry_request(requests.post, url, *args, **kwargs)

def put(url, *args, **kwargs):
return _retry_request(requests.put, url, *args, **kwargs)

def delete(url, *args, **kwargs):
return _retry_request(requests.delete, url, *args, **kwargs)
88 changes: 88 additions & 0 deletions tests/mocks/test_retry.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
"""
Test basic functionality of the Tables endpoint
"""
import time
import unittest
from unittest.mock import patch
from urllib.error import HTTPError

import requests
import responses
tomasfejfar marked this conversation as resolved.
Show resolved Hide resolved

from kbcstorage.tables import Tables

from .table_responses import list_response


class TestRequestRetry(unittest.TestCase):
"""
Test that requests are retried.
"""
def setUp(self):
token = 'dummy_token'
base_url = 'https://connection.keboola.com/'
self.tables = Tables(base_url, token)

@responses.activate
@patch('time.sleep', return_value=None)
def test_ok(self, sleep_mock):
"""
Retry will try at least 5 times.
"""
for _ in range(4):
responses.add(
responses.Response(
method='GET',
url='https://connection.keboola.com/v2/storage/tables',
json=list_response,
status=502
)
)
responses.add(
responses.Response(
method='GET',
url='https://connection.keboola.com/v2/storage/tables',
json=list_response,
)
)
tables_list = self.tables.list()
assert isinstance(tables_list, list)

@responses.activate
@patch('time.sleep', return_value=None)
def test_raises_error_many_tries(self, sleep_mock):
"""
Retry will fail if it gets enough of error responses.
"""
for _ in range(20):
responses.add(
responses.Response(
method='GET',
url='https://connection.keboola.com/v2/storage/tables',
json=list_response,
status=502
)
)
responses.add(
responses.Response(
method='GET',
url='https://connection.keboola.com/v2/storage/tables',
json=list_response,
)
)
with self.assertRaises(requests.exceptions.HTTPError):
self.tables.list()

@responses.activate
@patch('time.sleep', return_value=None)
def test_raises_error_on_4xx(self, sleep_mock):
responses.add(
responses.Response(
method='GET',
url='https://connection.keboola.com/v2/storage/tables',
json=list_response,
status=401
)
)
with self.assertRaises(requests.exceptions.HTTPError):
self.tables.list()
Loading