Fix: close connections after use #2650
Conversation
beckjake
force-pushed
the
fix/close-connections
branch
2 times, most recently
from
08802d8
to
3611f2d
Compare
- close() implies rollback, so do not call it - make sure to not open new connections for executors in single-threaded mode - logging cleanups - fix a test case that never acquired connections - to cancel other connections, one must first acquire a connection for the master thread - change a number of release() calls to rollback release vs rollback
beckjake
force-pushed
the
fix/close-connections
branch
2 times, most recently
from
3101ef6
to
23ed2f5
Compare
beckjake
force-pushed
the
fix/close-connections
branch
from
23ed2f5
to
44e3c7e
Compare
core/dbt/adapters/base/impl.py
Outdated
| @@ -1059,7 +1063,7 @@ def calculate_freshness( | |||
| table = self.execute_macro( | |||
| FRESHNESS_MACRO_NAME, | |||
| kwargs=kwargs, | |||
| release=True, | |||
| release=False, | |||
There was a problem hiding this comment.
is the release arg used anymore and should it stick around if not?
There was a problem hiding this comment.
It's not used in core anymore, but it is used in some plugins. Maybe anything using it needs to be changed anyway, I just hate to cause more plugin churn.
There was a problem hiding this comment.
I've played around with this some, and I think we have to remove the release arg.
@jtcohen6 I think this will break any dependent adapters. I am mostly fine with that happening in a patch release since it fixes a pretty crippling bug. Ours all are locked to minor versions so we'll be fine... let me know what you think!
edit: I've decided to deprecate it. I don't think it's a great fix, but I think it's an okay fix. Some adapters that are currently using the release argument may not have to make changes.
There was a problem hiding this comment.
Ok! I'm on board. This is a tiny breaking change to resolve a much more significant regression. We'll want to check that our plugins (spark + presto) can work with 0.17.2-rc1.
- fix conftest adainivalue_line calls - deprecate the release argument to execute_macro
| @@ -934,8 +934,10 @@ def execute_macro( | |||
| execution context. | |||
| :param kwargs: An optional dict of keyword args used to pass to the | |||
| macro. | |||
| :param release: If True, release the connection after executing. | |||
| :param release: Ignored. | |||
resolves #2645
Description
To make the snowflake thread exit, we need to actually close open connections. We should have been doing this before!
Fixing that caused some fallout with connection management, especially in single-threaded mode but also in a couple other places that were relying on open connections not being closed. I think this is all set now.
In order to test this, I had to modify the rpc integration tests to support snowflake. I think the way I did it is pretty good. Of note, I only enabled two snowflake tests (one snowflake-specific one) - the snowflake tests are just way too slow for us to reasonably run the full suite, IMO. Snowflake already takes an eternity.
I anticipate this will make it a bit easier to add RPC tests for other adapter-specific issues we find.
Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.