Fix run operation return codes (#1377) #1406
Conversation
Make dbt run-operation actually function at all with the RPC changes On exceptions that occur outside of actual macro execution, catch them and return failure appropriately
There was a problem hiding this comment.
looks good. @drewbanin should we document run-operation in wilt chamberlain?
core/dbt/task/run_operation.py
Outdated
| @@ -22,19 +21,44 @@ def _get_macro_parts(self): | |||
| def _get_kwargs(self): | |||
| return dbt.utils.parse_cli_vars(self.args.args) | |||
|
|
|||
| def run(self): | |||
| def run_unsafe(self): | |||
There was a problem hiding this comment.
can you make this _run_unsafe
|
@cmcarthur yeah, I think Wilt is the right time to release this for real. @beckjake I think it's actually pretty important that we don't add a commit at the end of operations. Some queries (like vacuum / analyze on Redshift) are not allowed to run inside of transactions. I think that if people want to run transactional code through operations, they should add the If we do this, then we need to be really sure that we don't begin a transaction implicitly, otherwise the query won't be committed. Do we have a good way of ensuring that connections never have open connections when the operations SQL runs? |
core/dbt/task/run_operation.py
Outdated
| result = self._run_unsafe() | ||
| except dbt.exceptions.Exception as exc: | ||
| logger.error( | ||
| 'Encountered a dbt exception while running a macro: {}' |
There was a problem hiding this comment.
This is an exception that dbt is aware of, but it's not a dbt exception per se. I just don't want folks to think that dbt did something wrong if their operations fail. Can we make this say:
Encountered an error while running an operation:
Ok, I can do that instead then.
I think that's already true. If your macro is implemented via
I assume you mean "... connections never have open transactions ...". Yeah. |
There was a problem hiding this comment.
This looks great - good call on adding tests.
One thing: I think we'll want to clear the transaction before calling the operation macro. This is helpful for queries (like vacuum) that must be run outside of a transaction:
{% macro vacuum() %}
{% call statement(auto_begin=false) %}
vacuum public.events
{% endcall %}
{% endmacro %}
If you invoke this operation, you'll see:
VACUUM cannot run inside a transaction block
I don't actually see an explicit begin; in the logs, so I think this is that psycopg2 thing where new connections are created with an open transaction. I assumed that setting auto_begin=false would prevent this from happening, but maybe we can be heavy-handed and just add
adapter.clear_transaction()
right before the call to adapter.execute_macro()
This seems to work ok, and I believe it no-ops on databases that don't have transactions.
| .format(exc) | ||
| ) | ||
| logger.debug('', exc_info=True) | ||
| return False, None |
There was a problem hiding this comment.
Could we conceivably return a dict that encodes this error message? That would be useful for an eventual API that wants to do something with the results when an operation fails.
I think we can leave this as-is for now, but want to keep it in the back of our minds when we revisit the RunResults node contract.
There was a problem hiding this comment.
Sure, though in the RPC case I'd probably reach for overriding the run method entirely (the message would end up in the response logs anyway)
Fixes #1377
When we run an operation, set the return code on error.
Also, since wilt-chamberlain totally broke the run-operation command in multiple ways, I fixed all that. And added tests so I don't do that again.
I also added a commit call that happens after successfully executing the macro. Maybe that would be better as a flag? I just wanted it for testing, but I can't really see not wanting to commit.