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.

Sign up for GitHub

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

rest request retry upon failure #63

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

rest request retry upon failure #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e161f99
rest request retry upon failure
barrysteyn Dec 4, 2017
32a671c
adding timeout to request
barrysteyn Dec 4, 2017
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
7 changes: 6 additions & 1 deletion numismatic/feeds/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,12 @@ def __requester_validator(self, attribute, value):
raise ValueError(f'{attribute.name}: {value}')

def _make_request(self, api_url, params=None, headers=None, raw=False):
response = self.requester.get(api_url, params=params, headers=headers)
try:
response = self.requester.get(api_url, params=params, headers=headers)
except Exception as ex:
logger.error(ex)
logger.error(packet)
raise ex
if not raw:
data = response.json()
else:
Expand Down
6 changes: 6 additions & 0 deletions numismatic/numismatic.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ assets = BTC
currencies = USD
channels = trades

[REQUESTER]
retries = 3
backoff_factor = 0.3
status_forcelist=500,502,503,504
timeout=5

[BitfinexFeed]

[BraveNewCoinFeed]
Expand Down
30 changes: 29 additions & 1 deletion numismatic/requesters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import logging
import requests
from requests.adapters import HTTPAdapter
from requests.packages.urllib3.util.retry import Retry
import time
from pathlib import Path
from functools import partial
Expand All @@ -9,6 +11,7 @@

import attr
from appdirs import user_cache_dir
from .config import config_item_getter


log = logging.getLogger(__name__)
Expand All @@ -21,6 +24,20 @@
class Requester:
"Basic Requester using requests and blocking calls"

retries = attr.ib(default=attr.Factory(
config_item_getter('REQUESTER', 'retries')),
Copy link
Owner

Choose a reason for hiding this comment

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

Nice! For the other classes I tried to have the ini file section have the same title as the class name (i.e. the same case). Not sure if that runs foul of INI file conventions of being all upper case. The aim was that with the ConfigMixin it would automatically find the correct section and be open for extension by subclasses. It was a bit tricky and I see that for the api keys in Luno and BraveNewCoin we didn't follow that. However it does seem to work for assets and currencies (e.g. for Luno) but I think you have to define a custom validator.

Copy link
Contributor Author

@barrysteyn barrysteyn Dec 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Donald Knuth's famous quote (and what I tell my team often) may apply here: Premature optimization is the root of all evil. My take is that we may be overthinking things a bit here. Lets just try make things simple for now. In that regard, I propose making things all capitals (like the ConfigSetter examples online) and have the spelling identical to the class name.

Also, those custom validators look like code bloat to me...

convert=int)
timeout = attr.ib(default=attr.Factory(
config_item_getter('REQUESTER', 'timeout')),
convert=int)
backoff_factor = attr.ib(default=attr.Factory(
config_item_getter('REQUESTER', 'backoff_factor')),
convert=float)
status_forcelist = attr.ib(default=attr.Factory(
config_item_getter('REQUESTER', 'status_forcelist')),
convert=lambda val: tuple(map(int, val.split(','))),
validator=attr.validators.instance_of(tuple))

@classmethod
def factory(cls, requester, **kwargs):
requester = '' if requester is None else requester.lower()
Expand All @@ -36,7 +53,18 @@ def factory(cls, requester, **kwargs):
return subcls(**kwds)

def get(self, url, params=None, headers=None):
response = requests.get(url, params=params, headers=headers)
session = requests.Session()
retry = Retry(
total=self.retries,
read=self.retries,
connect=self.retries,
backoff_factor=self.backoff_factor,
status_forcelist=self.status_forcelist,
)
adapter = HTTPAdapter(max_retries=retry)
session.mount('http://', adapter)
session.mount('https://', adapter)
response = session.get(url, params=params, headers=headers, timeout=self.timeout)
return response


Expand Down