influxdata / influxdb-python

Python client for InfluxDB
MIT License
1.69k stars 520 forks source link

retry on HTTPError exceptions need raise_for_status()? #832

Open loopway opened 4 years ago

loopway commented 4 years ago

We occasionally receive a http status 500 code (time out) from our influxdb instance, probably because of temporary peak load. Looking at the retry code in InfluxDBClient class I can see that you try to catch HTTPErrors (line 345):

https://github.com/influxdata/influxdb-python/blob/fc0235e2c07abf09eff4de7f6396a17aca1080f1/influxdb/client.py#L326-L353

According to the requests library documentation a raise_for_status() is needed for the exception to be caught:

https://requests.readthedocs.io/en/master/user/quickstart/#errors-and-exceptions

Or am I missing something?

russorat commented 4 years ago

@loopway thanks for the issue. I'm not sure that's needed tbh. this code prints exception using python 3.7.7 on my mac:

# import requests module
import requests

try:
  # ping an incorrect url
  response = requests.get('https://not a real url/')

  print(response)
except (requests.exceptions.ConnectionError,
             requests.exceptions.HTTPError,
             requests.exceptions.Timeout):
  print("exception")
loopway commented 4 years ago

@russorat in your example it gets printed because it's a 'ConnectionError', and not a 'HTTPError'. Please try this code which will connect to httpstat.us to return a http error code 500:

# import requests module
import requests

try:
  # get a http status code 500
  response = requests.get('https://httpstat.us/500')

  print(response)

  # comment this and exception will not be raised
  response.raise_for_status()
except (requests.exceptions.ConnectionError,
             requests.exceptions.HTTPError,
             requests.exceptions.Timeout):
  # will only be printed if raise_for_status() is called
  print("exception")
russorat commented 4 years ago

@loopway thanks for clarifying! We'd be happy to review a PR if you're interested!