-
Notifications
You must be signed in to change notification settings - Fork 106
Don't raise exception when closing a stale Thrift session #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
src/databricks/sql/client.py
Outdated
) | ||
pass | ||
else: | ||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if we should swallow all exceptions for close session calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought. I've updated the code to log all exceptions but not raise them.
Signed-off-by: Jesse Whitehouse <[email protected]>
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Oddly, my integration test that I added is now failing. Researching before I merge this... |
session close RPCs. Updated logging for clarity and modified e2e test Signed-off-by: Jesse Whitehouse <[email protected]>
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Okay I researched why it failed. It's because the Databricks thrift server no longer raises an exception upon multiple attempts to emit |
…#159) Signed-off-by: Jesse Whitehouse <[email protected]>
Prior to this change, if a context manager or a user explicitly called
Connection.close()
twice on the same connection, the second call would raise an exception:This sometimes affects dbt, airflow and others if they use connections as a context manager.
This pull request explicitly catches this error, logs it cleanly, and then continues execution. The pull request is broken into a few commits that do this:
bytes
session_handle we receive from Thrift server into a human readable GUIDtest.env.example
to include the environment variables we need for running sqlalchemy e2e tests. This is just convenient. Has no impact on the connector as installed for users.