Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

closing connection once data diff is executed #785

Merged
merged 52 commits into from
Jan 22, 2024
Merged

Conversation

sar009
Copy link
Contributor

@sar009 sar009 commented Nov 28, 2023

This PR revamps how we close database connections, adds new unit tests to the main entrypoint logic, and refactors tests for better readability.

Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
@sar009
Copy link
Contributor Author

sar009 commented Nov 29, 2023

fixes issue #784

Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
@dlawin dlawin requested review from nolar and dlawin November 29, 2023 17:54
@dlawin dlawin linked an issue Nov 29, 2023 that may be closed by this pull request
@sungchun12 sungchun12 self-requested a review January 3, 2024 19:23
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
@sar009
Copy link
Contributor Author

sar009 commented Jan 10, 2024

I noticed you only added the close() method to postgres. You'll need to define that for the other databases. Should be straightforward for other files. Example: databricks.py, redshift.py, etc.

All other classes are handled from the close method in ThreadedDatabase or from Database since postgres.py was overriding close and has some special way of handling connection

@sar009
Copy link
Contributor Author

sar009 commented Jan 10, 2024

Let's also add a test to test_database.py and assert that the close method is working as expected. It'll dynamically test all the databases are closing as expected.

I am not sure how this will dynamically test for all of the databases. For now, I have written for Postgres (https://github.com/datafold/data-diff/pull/785/files#diff-82fbc96aa3f082d4667c09a2a35aec050c73120de9174e1f37bef26ef9cd3115R173)

Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
@sar009
Copy link
Contributor Author

sar009 commented Jan 10, 2024

@sungchun12 I had to remove connection sharing since it was causing test failure in parallel unit test run
https://github.com/datafold/data-diff/pull/785/files#diff-53eb316ef7b18ba47acdd955cdaa7ca5467cd5ba6f5cf8ed92c68a421902d120L90

sungchun12 and others added 11 commits January 13, 2024 10:58
Co-authored-by: Sung Won Chung <sungwonchung3@gmail.com>
Co-authored-by: Sung Won Chung <sungwonchung3@gmail.com>
Co-authored-by: Sung Won Chung <sungwonchung3@gmail.com>
Co-authored-by: Sung Won Chung <sungwonchung3@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
@sungchun12
Copy link
Contributor

@sar009 any updates?

Signed-off-by: Sarad Mohanan <sarad.mohanan@gmail.com>
@sungchun12 sungchun12 merged commit d2161cc into datafold:master Jan 22, 2024
@sar009 sar009 deleted the 784 branch January 23, 2024 02:30
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DB Connection is not closed automatically and close function fails
2 participants