Server error tolerance, more verbose exceptions #8
Conversation
| - 3.7-dev | ||
| - nightly | ||
| #- 3.7-dev | ||
| #- nightly |
There was a problem hiding this comment.
yaml library is not compiling with python3.7 yet :(
constructor_io/constructor_io.py
Outdated
| timeout = RETRY_TIMEOUT_IN_CASE_OF_SERVER_ERROR * ( | ||
| self._server_error_retries - retries_left + 1) | ||
| logger.warning( | ||
| str(error.message) + " Retrying in " + str(timeout) + |
There was a problem hiding this comment.
What about something like:
logger.warning(
'%s Retrying in %d seconds. Retries left: %d',
error.message, timeout, retries.left,
)
?
| ) | ||
| assert resp is True | ||
|
|
||
| def test_exceptions(self): |
There was a problem hiding this comment.
Given that pytest is in the requirements, I'd recommend to reple all the
self.assert....
with:
assert ...
There was a problem hiding this comment.
Why is it better?
There was a problem hiding this comment.
pytest rewrites the code on the fly and replaces the assert statements with their own code.
This way, you just need to use assert and the comparison operator that you like instead of having to remember which is the best self.assertSomething function for a given comparison.
Aside from that, when the assertion fails, it will typically show you more information about the values that were passed rather than just the result of the expression. You can see more information about that here.
constructor_io/constructor_io.py
Outdated
| try: | ||
| # Wrap server error codes as exceptions | ||
| response = request_method(*args, **kwargs) | ||
| if round(response.status_code / 100) == 5: |
There was a problem hiding this comment.
I believe that integer division would be more readable here. That is instead of:
round(response.status_code / 100) == 5
this:
(response.status_code // 100) == 5
There was a problem hiding this comment.
Also, note that round(451 / 100) is 5, not 4.
There was a problem hiding this comment.
Agree about integer division here. By the way:
$ python
Python 2.7.6 (default, Jun 22 2015, 17:58:13)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> round(451 / 100)
4.0
>>> round(459 / 100)
4.0
>>> round(499 / 100)
4.0
>>>
There was a problem hiding this comment.
My bad, I checked with python 3. Anyway, from what I see, the code is expected to work with both python 2 and 3, so it's better not to use round just in case.
constructor_io/constructor_io.py
Outdated
| timeout = RETRY_TIMEOUT_IN_CASE_OF_SERVER_ERROR * ( | ||
| self._server_error_retries - retries_left + 1) | ||
| logger.warning( | ||
| '%s Retrying in %d seconds. Retries left: %d' % |
There was a problem hiding this comment.
In this case it's preferable to let the logging module do the string interpolation for you, that is, replace:
logger.warning(<message> % <values)
with:
logger.warning(<message>, <values>)
The reason for this is that if log level is set to something above warning the interpolation won't happen. This might seem it doesn't make a difference, but in a big code base performance results might result of this, so in my opinion is a better option to do it always this way.
constructor_io/constructor_io.py
Outdated
| # Retry in case of server error | ||
| if retries_left <= 0: | ||
| raise error | ||
| timeout = RETRY_TIMEOUT_IN_CASE_OF_SERVER_ERROR * ( |
There was a problem hiding this comment.
This seems to increment the timemout linearly, while usually timeouts increment exponentially,
so I'd recommend something like:
timeout = <default>
try:
...
except <exception>:
time.sleep(timeout)
timeout **= 2
There was a problem hiding this comment.
If you still prefer to increment the timeout linearly, a modified version of the code above is more readable in my opinion:
timeout = <default>
try:
...
except <exception>:
time.sleep(timeout)
timeout += <default>
In case of server error, library will try to repeat the same request 10 more times.
10is by default. May be changed by user.