Skip to content

Separate Session related functionality from Connection class #567

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

varun-edachali-dbx
Copy link
Collaborator

What type of PR is this?

  • Refactor

Description

Separate the Session related functionality from the Connection class, in order to:

  • Better align the object structure with that of the JDBC driver
  • Further abstract the backend implementation details from the connection, to eventually give way to the introduction of SEA.

How is this tested?

  • Unit tests
    • some of the existing unit tests were slightly altered to account for the introduction of the Session class. No unit tests were removed or introduced.
  • E2E Tests
  • Manually
  • N/A

Related Tickets & Documents

https://docs.google.com/document/d/1Y-eXLhNqqhrMVGnOlG8sdFrCxBTN1GdQvuKG4IfHmo0/edit?usp=sharing

@varun-edachali-dbx varun-edachali-dbx force-pushed the decouple-session branch 2 times, most recently from 22974cd to 398465b Compare May 23, 2025 04:50
@varun-edachali-dbx varun-edachali-dbx marked this pull request as draft May 23, 2025 05:58
@varun-edachali-dbx varun-edachali-dbx marked this pull request as ready for review May 23, 2025 06:02
@varun-edachali-dbx varun-edachali-dbx marked this pull request as draft May 23, 2025 06:04
@varun-edachali-dbx varun-edachali-dbx marked this pull request as ready for review May 23, 2025 06:13
@varun-edachali-dbx varun-edachali-dbx force-pushed the decouple-session branch 2 times, most recently from b3aef77 to 106fdb1 Compare May 23, 2025 13:24
ensure maintenance of current APIs of Connection while delegating
responsibility

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
… through Connection

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
as in CONTRIBUTING.md

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
in case the openSession takes long, the initialisation of the session
will not complete immediately. This could make the session attribute
inaccessible. If the Connection is deleted in this time, the open()
check will throw because the session attribute does not exist. Thus, we
default to the Connection being closed in this case. This was not an
issue before because open was a direct attribute of the Connection
class. Caught in the integration tests.

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
earlier, one of the integration tests was failing because 'session was
not an attribute of Connection'. This is likely tied to a local
configuration issue related to unittest that was causing an error in the
test suite itself. The tests are now passing without checking for the
session attribute.
databricks@c676f9b

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
varun-edachali-dbx and others added 12 commits May 24, 2025 02:25
This reverts commit d6b1b19.

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
new codeowners

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
…t to prevent server side resource leaks (databricks#554)

* Enhance Cursor close handling and context manager exception management

* tests

* fmt

* Fix Cursor.close() to properly handle CursorAlreadyClosedError

* Remove specific test message from Cursor.close() error handling

* Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors.

* add

* add

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
* PECOBLR-86 Improve logging for debug level

Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>

* PECOBLR-86 Improve logging for debug level

Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>

* fixed format

Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>

* used lazy logging

Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>

* changed debug to error logs

Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>

* used lazy logging

Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>

---------

Signed-off-by: Sai Shree Pradhan <saishree.pradhan@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
…couple-session"

This reverts commit dbb2ec5, reversing
changes made to 7192f11.

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
…ecouple-session"

This reverts commit bdb8381.

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
ensures correctness of self.session.open call in Connection

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
@varun-edachali-dbx
Copy link
Collaborator Author

superseded by #571

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.

4 participants