Wrap all requests.exceptions.RequestException as TrinoConnectionError - #393
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| try: | ||
| response = self._request.post(self._query, additional_http_headers) | ||
| except requests.exceptions.RequestException as e: | ||
| raise trino.exceptions.TrinoConnectionError("failed to execute: {}".format(e)) |
There was a problem hiding this comment.
I think a OperationalError is more at its place here.
This also applies to the other places where you wrap this exception, otherwise it would not be captured by other frameworks like sqlalchemy.
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
Please apply changes from #393 (comment)
LGTM otherwise.
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
LGTM.
Please squash the fixups @Shaheer-rossoneri14.
Before this change if a user needed to catch connection errors (e.g. connection refused) they needed to add a dependency on requests and import the relevant exception from requests.exceptions module. After this change all requests exception get rethrown as a TrinoConnectionError instead.
771163f to
e0cf1f2
Compare
|
@hashhar I have squashed the fixups. |
4 participants
Description
Wrap all requests.exceptions.RequestException as TrinoConnectionError.
Fixes #364.
Non-technical explanation
Before this change if a user needed to catch connection errors (e.g. connection refused) they needed to add a dependency on requests and import the relevant exception from requests.exceptions module.
After this change all requests exception get rethrown as a TrinoConnectionError instead.
Release notes
( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: