Skip to content

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

Merged
merged 10 commits into from
Jun 26, 2023
Merged

Conversation

susodapop
Copy link
Contributor

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:

databricks.sql.exc.DatabaseError: Invalid SessionHandle: SessionHandle [<GUID>]

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:

  1. Make it easy to see log messages when running pytest
  2. Implement a method that converts the bytes session_handle we receive from Thrift server into a human readable GUID
  3. Explicitly catch the InvalidSession handle error (this is the most important part of this PR)
  4. Log when this happens so we can observe it (this shouldn't happen in the normal course of usage)
  5. Update the changelog
  6. Unrelated: update test.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.

@github-actions
Copy link

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 (git rebase -i main).

)
pass
else:
raise e

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.

Copy link
Contributor Author

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.

@github-actions
Copy link

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 (git rebase -i main).

@susodapop
Copy link
Contributor Author

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]>
@github-actions
Copy link

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 (git rebase -i main).

@susodapop
Copy link
Contributor Author

Okay I researched why it failed. It's because the Databricks thrift server no longer raises an exception upon multiple attempts to emit closeSession for the same handler. I've revised my test to account for this and updated the logging.

@susodapop susodapop merged commit 8d70f6c into main Jun 26, 2023
@susodapop susodapop deleted the issue-157 branch June 26, 2023 20:05
susodapop pushed a commit to unj1m/databricks-sql-python that referenced this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants