per-thread connections #1336
Merged
per-thread connections #1336
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
beckjake
force-pushed
the
feature/one-connection-per-thread
branch
5 times, most recently
from
f165bf8
to
13c1a71
Compare
parsing now always opens a connection, instead of waiting to need it remove model_name/available_raw/etc
beckjake
force-pushed
the
feature/one-connection-per-thread
branch
from
13c1a71
to
e2af871
Compare
drewbanin
reviewed
Mar 7, 2019
core/dbt/task/run.py
Outdated
|
|
||
| adapter.release_connection(model_name) | ||
| adapter.release_connection() |
There was a problem hiding this comment.
do we want to close the connection here? or is that handled by the context manager?
There was a problem hiding this comment.
We don't close the actual connection until the connection enters an error state or at the end of execution entirely (the finally block of execute_with_hooks).
core/dbt/task/run.py
Outdated
| self.safe_run_hooks(adapter, RunHookType.Start, {}) | ||
| with adapter.connection_named('master'): |
drewbanin
approved these changes
Mar 8, 2019
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.
Fixes #1312
Bind connections to threads, rather than to names. Names are attached to connections, but only show up in logging. Threads get a connection assigned to them and re-use/rename it as necessary. I ended up enforcing that with a locked dictionary instead of thread-local storage because the main thread needs access to the connection object for query cancellation.
This removes
model_name/connection_namefrom adapter methods, and the oldavailable_rawdecorator is now just namedavailable, and the DatabaseWrapper no longer injects model_name.One user-facing change: Parsing now always opens a connection instead of waiting to need it, which simplifies the connection logic a lot but does open a socket that isn't always necessary. I think it's ok, but if we have to I can do clever things to try to defer the actual opening until it's needed.
I spent a lot of time trying to break this and failed, but I still kind of expect to find significant edge-case bugs.