-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix statements in on run start hooks #693
Conversation
@cmcarthur nice! This should also fix the issue with connections being closed in on-run-end hooks, right? |
@drewbanin yep, that's right. if you have a test case for that, can you run this branch and see if it fixes it? |
@cmcarthur I don't have a test case handy -- let me see if I can whip one up |
dbt/node_runners.py
Outdated
auto_begin=False) | ||
adapter.release_connection(profile, conn_name) | ||
|
||
if len(sql) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do sql.strip()
here? Unsure if it will make any difference, but I know sometimes people put conditional statements into hooks, and the else
clause might contain some empty space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmcarthur this LGTM if you've tested on Redshift + Snowflake with and without hooks!
dbt/node_runners.py
Outdated
# configuring psycopg2 (and other adapters?) to ensure that a | ||
# transaction is only created if dbt initiates it. | ||
conn_name = adapter.clear_transaction(profile) | ||
adapter.clear_transaction(profile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmcarthur this will use the master
connection, right? I think this can still cause issues with the on-run-end
hook after a long dbt run. Do we need to clear this transaction here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not? good call
dbt/node_runners.py
Outdated
|
||
ordered_hooks = sorted(compiled_hooks, key=lambda h: h.get('index', 0)) | ||
|
||
for hook in ordered_hooks: | ||
model_name = compiled.get('name') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be hook
, not compiled
I think
…statements-in-on-run-start-hooks
This also fixes #620 |
statements don't really work in on run start hooks, because the connection name used doesn't match. there's some code that uses
master
and some that uses the node name. this makes everything use the node name, and releases the connection after compilation in case a statement is run.