Skip to content
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

Gracefully Close Database Connector #364

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

sophiadt
Copy link
Contributor

Wanted a mechanism to properly close the database connector and this PR is chaining the pipeline finish event from the Graph Database Writer all the way down to the database connector.

Copy link
Contributor

@zprobst zprobst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @sophiadt!

I think this approach will work for us. A couple requests:

  • Can we add finish to the ABCs for the executor and the ingest strategies? In the case of the executor I think a reasonable default is to simply pass?
  • Is it possible to add a test case or two to cover the added lines?

@zprobst
Copy link
Contributor

zprobst commented Sep 4, 2024

I've also approved the CI run. I suspect we may see a linter error or two crop up.

@sophiadt
Copy link
Contributor Author

sophiadt commented Sep 5, 2024

Hi @zprobst!
I've added the abstract methods and tests now 👍

Copy link
Contributor

@zprobst zprobst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now! Thanks for the contribution and collaboration!

@zprobst zprobst merged commit b7b9336 into nodestream-proj:main Sep 5, 2024
9 checks passed
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.95%. Comparing base (d31478c) to head (a002b4a).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #364   +/-   ##
=======================================
  Coverage   97.94%   97.95%           
=======================================
  Files         147      147           
  Lines        5307     5319   +12     
=======================================
+ Hits         5198     5210   +12     
  Misses        109      109           
Flag Coverage Δ
3.10-macos-latest 97.91% <100.00%> (-0.02%) ⬇️
3.10-ubuntu-latest 97.91% <100.00%> (-0.02%) ⬇️
3.10-windows-latest 97.87% <100.00%> (-0.02%) ⬇️
3.11-macos-latest 97.91% <100.00%> (-0.02%) ⬇️
3.11-ubuntu-latest 97.91% <100.00%> (-0.02%) ⬇️
3.11-windows-latest 97.87% <100.00%> (-0.02%) ⬇️
3.12-macos-latest 97.93% <100.00%> (+<0.01%) ⬆️
3.12-ubuntu-latest 97.91% <100.00%> (-0.02%) ⬇️
3.12-windows-latest 97.87% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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