TDL-16314 added request timeout#96
Conversation
|
Disabled pylint for consider-using-f-string when used format. |
| # backoff for 1 minute when the Timeout error occurs as the request will again backoff | ||
| # when timeout occurs in `refresh_credentials()`, hence instead of setting max_tries as 5 | ||
| # setting the max_time of 60 seconds | ||
| @backoff.on_exception(backoff.expo, Timeout, max_time=60, factor=2) |
There was a problem hiding this comment.
Why we don't add ConnectTimeout here?
There was a problem hiding this comment.
Why we don't add ConnectTimeout here?
We didn't get ConnectTimeout Error here, so haven't included it
| import pytz | ||
| import backoff | ||
| import singer | ||
| from requests.exceptions import Timeout, ConnectTimeout |
There was a problem hiding this comment.
Is there any specific reason to import ConnectTimeout?
There was a problem hiding this comment.
Is there any specific reason to import ConnectTimeout?
It was not used anywhere as ConnectTimeout inherits Timeout, hence removed it
kspeer825
left a comment
kspeer825
left a comment
There was a problem hiding this comment.
Left a suggestion for the test implementation.
| mock_send.side_effect = Timeout | ||
| mock_post.side_effect = Timeout | ||
| before_time = datetime.datetime.now() | ||
| with self.assertRaises(Timeout): |
There was a problem hiding this comment.
I think these with assertRaises contexts should contain only the method that raises the exception, if the XeroClient init had a request in it and that timed out unexpectedly, this would never get to the check_platform_access step. But it would still pass. While I think it is performing functionally the way it's intended, it is difficult to see exactly what is being tested.
|
The build is failing because the token is expired on the circleci side |
…DL-16314-implement-request-timeout
6 participants
Description of change
Implemented request timeout with parameter provided from config
Manual QA steps
Risks
Rollback steps