dabla commented on code in PR #74279:
URL: https://github.com/apache/airflow/pull/74279#discussion_r4556595326
##########
providers/common/sql/src/airflow/providers/common/sql/hooks/sql.py:
##########
@@ -197,6 +203,11 @@ def __init__(self, *args, schema: str | None = None,
log_sql: bool = True, **kwa
self.__schema = schema
self.log_sql = log_sql
self.descriptions: list[Sequence[Sequence] | None] = []
+ # The cursor whose statement is running right now (set by
``_track_running_cursor``), used by
+ # ``cancel_query``
+ self._running_cursor: Any = None
+ # Whether ``_track_running_cursor`` cancels a statement interrupted by
a timeout or keyboard interrupt
+ self.cancel_on_interrupt = True
Review Comment:
**[warning]** With `True` as the hook default, this changes every caller of
`DbApiHook.run()`, not only `SQLExecuteQueryOperator`.
`_track_running_cursor` sits in `run()` and `_run_command()`, and
`get_records()` and `get_first()` go through `run()`. So with this default, a
statement interrupted by `execution_timeout` is now cancelled for everything
that uses a `DbApiHook`:
- the SQL check operators, `BranchSQLOperator` and `SQLInsertRowsOperator`
(pre and post operator),
- `SqlSensor` and `GenericTransfer`,
- any `@task` or `PythonOperator` that calls `hook.run()` itself.
None of these has a `cancel_on_kill` parameter, so their users can only turn
it off by setting the attribute on the hook by hand. They also get half of the
feature: the statement is cancelled when the timeout elapses, but not when the
task is marked failed, because only `SQLExecuteQueryOperator` has the `on_kill`.
The PR title, the body and `operators.rst` describe this as a change to
`SQLExecuteQueryOperator`. Cancelling is probably what most people want, but a
statement that used to keep running on the server after the task timed out (an
autocommit load, a stored procedure) now stops, and nobody reading the
changelog entry would expect that from a check operator or a sensor.
`DbApiHook` defaults have been kept stable for this reason before
([#38715](https://github.com/apache/airflow/pull/38715#discussion_r1555556847)).
Two ways out:
1. Default `cancel_on_interrupt = False` on the hook.
`SQLExecuteQueryOperator.execute()` already sets it from `cancel_on_kill`
(default `True`), so the operator behaves exactly as it does now and other
callers opt in.
2. Keep `True`, and say so: a title that covers the hook, and a sentence in
the docs that every `DbApiHook.run()` caller is affected, with how to turn it
off.
I would go for 1 in this PR and switch the other operators on in follow-ups,
each with its own `on_kill`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
