Allow dbt to cancel connections #718
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR. CLA has not been signed by users: @holly-evans |
|
Signed the CLA just now. |
|
I locked Output before: Output after: Logs before: Logs after: |
|
Hi, is there anything I can add to help this PR move forward? Tagging @mikealfare, I see you on most of the PRs this year |
Thanks for your submission @holly-evans! This looks pretty good. I updated the branch and approved running integration tests. The one thing that jumps out is a missing changelog. Please refer to the steps here for adding a changelog entry. I'll review this sometime in the next week. Thanks again! |
mikealfare
requested review from
mikealfare and
a team
and removed request for
mikealfare
|
@mikealfare added changelog entry 👍 |
McKnight-42
reviewed
Apr 5, 2024
dbt/adapters/redshift/connections.py
Outdated
| def cancel(self, connection: Connection): | ||
| try: | ||
| pid = self._get_backend_pid() | ||
| pid = connection.handle.backend_pid |
There was a problem hiding this comment.
@holly-evans This is all looking very good but we do have some tests failing was looking at this a bit and here may be a good point doesn't look like we setup a handle object so we may need to ref the new ConnectionWrapper more directly.
If I can be of any more help please feel free to message me I will try to spend a little more time with this to give more advice as I can.
|
@McKnight-42 Thank you for that feedback! @holly-evans also wanted to apologize for the delay with this PR. We have pulled this into our current sprint and we are watching for your engagement and are aiming to get this reviewed and approved as soon as possible. Thank you for your patience. 🙏 @colin-rogers-dbt if this is ready for final approval while I'm out next week, can you please do the honors of getting it across the line? Thanks! |
|
Many of the failures are known flaky tests (already captured in another ticket). I'm rerunning CI to see if it changes what's passing. There were only maybe one or two failed tests that looked like they could be related to this. |
mikealfare
reviewed
Apr 12, 2024
There was a problem hiding this comment.
I left several comments, mostly around trying a different approach. As I mention below, I would have rather used your wrapper approach, but I could not figure out how to get it to work with some existing scenarios. While the approach I recommend appears to keep the existing tests passing, I'm eager to find out if you see a difference in the execution, and subsequently the log outputs that you previously shared. Please let me know how this works out, and if you see any issues with these recommendations. Thanks again for your contribution!
|
Made the suggested changes and the unit tests are passing. Hoping the functional tests pass! It works in my project too. I locked dbt_hevans.accounting_feature_value before the dbt command to simulate my issue then cancelled the run with CTRL + C. |
mikealfare
reviewed
Apr 12, 2024
mikealfare
reviewed
Apr 12, 2024
holly-evans
commented
Apr 12, 2024
holly-evans
commented
Apr 12, 2024
mikealfare
approved these changes
Apr 13, 2024
mikealfare
pushed a commit
that referenced
this pull request
Apr 15, 2024
mikealfare
added a commit
that referenced
this pull request
Apr 17, 2024
Co-authored-by: Holly Evans <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #705
docs dbt-labs/docs.getdbt.com/#
Problem
Interrupting dbt executions does not kill currently running queries. The dbt process does not stop until queries have completed. dbt issues a
pg_terminate_backendcommand for the wrong pid.Solution
This PR wraps
Connection.handleto capture the backend pid upon initiation and uses that connection-specific pid to cancel connections. Storing the pid before the model query begins ensures that we can runselect pg_terminate_backend(pid)upon cancel, even when a query is in progress.I tried simply switching
_get_backend_idto use the connection passed in tocancel, but it could not query for the pid until the running query finished.Context
Currently,
select pg_backend_pid()is called at the time of cancel, on themasterconnection. This pid is connection-specific, so whenselect pg_terminate_backend(pid)is called with that pid, it's not canceling the query of the connection passed tocancel, but a non-existent query onmaster.Interrupting a dbt execution worked up until dbt-redshift 1.5 because
psycopg2stored a backend pid on the handle that was used to cancel, butredshift_connectordoes not have this pid available.If a connection is actively querying, attempting to query
select pg_backend_pid()on that connection is delayed until the active query finishes (which may never happen in case of a blocking lock).dbt-databricks uses a connection wrapper to augment the
handle, which inspired this approach.Checklist