Feature/3117 dbt deps timeout #3275
Conversation
|
Hey folks 👋 just wanted to check to see if there was anything I needed to update on this to have it reviewed/approved? Thank you. |
There was a problem hiding this comment.
@TeddyCr This looks solid! Thanks so much for contributing, and sorry for the delay on my end. I left a few comments prompted by the failing Windows CI test.
Once that test is passing, could you add a changelog entry (v0.20.0 - "Under the hood") and add yourself to the list of contributors?
core/dbt/clients/registry.py
Outdated
| raise requests.exceptions.Timeout( | ||
| 'Request timeout, server has not issued a response for 60 seconds' | ||
| ) from exc_timeout |
There was a problem hiding this comment.
Should this raise its own Timeout-specific error message after hanging for 60 seconds, or simply kick back to retry in the line below?
except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as exc:I think either could be a good answer!
There was a problem hiding this comment.
My initial thought was to have 2 distinct error handling, but since, it appears that, the azure pipeline does not raise a Timeouterror, it might be safer to handle both in a generic error handling block as you wrote above.
| class testRegistryGetRequestTimeout(unittest.TestCase): | ||
| def test_registry_request_timeout(self): | ||
| # using non routable IP to test timeout logic in the _get function | ||
| self.assertRaises(Timeout, _get, '', 'http://10.255.255.1') |
There was a problem hiding this comment.
Related to above, it looks like the failing Windows test is skipping the Timeout error, retrying 5 times, and finally raising dbt.exceptions.RegistryException: Unable to connect to registry hub.
Let's do one of:
- Figure out why Windows is skipping the
Timeoutexception and jumping into retries - Amend the code above to raise a consistent retry-based exception
- Edit this test to check for either
TimeoutorRegistryException
There was a problem hiding this comment.
Based on the above update to the timeout error handling I went ahead and made a generic error catching test.
…ic to the timeout exception
… error catching + renamed test file accordingly
resolves #
Description
resolves #3117 (dbt deps hanging for hours)
In this PR we added a
timeoutparameter to therequests.get()method to prevent requests from hanging for hours.Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.