You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/09/22 12:12:52 UTC
[GitHub] [airflow] kazanzhy opened a new pull request #18431: Add hook_params in SqlSensor.
kazanzhy opened a new pull request #18431:
URL: https://github.com/apache/airflow/pull/18431
This PR is based on: #17592
closes: #13750
SqlSensor relies on underlying hooks that are automatically injected (DI) depending on the connection type provided to the SqlSensor. However, from time to time (but mainly in BigqueryHook) it is needed to pre-configure the "to-be-injected" hook because some default params do not fit the current config.
For example, if using SqlSensor with Bigquery it by default configures the SQL dialect to "legacy SQL", and there is no way to change this through configuration. Instead, the SqlSensor has to be extended and some functions are overwritten.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-968395987
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18431: Add hook_kwargs to SqlSensor.
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750848153
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_kwargs=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_kwargs = hook_kwargs
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
- 'jdbc',
- 'mssql',
- 'mysql',
- 'odbc',
- 'oracle',
- 'postgres',
- 'presto',
- 'snowflake',
- 'sqlite',
- 'trino',
- 'vertica',
- }
- if conn.conn_type not in allowed_conn_type:
+ hook = conn.get_hook(hook_kwargs=self.hook_kwargs)
+ if not isinstance(hook, DbApiHook):
Review comment:
Why are you making this change in a PR that purports only to add `hook_kwargs` to `SqlSensor`?
The description should be updated to explain what the reason is for this change. Probably it makes sense to be in a separate PR.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add `hook_params` and fix `allowed_conn_types` for SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r751057294
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_params=None,
Review comment:
I definitely agree with you.
But this inconsistency was provided by this PR (https://github.com/apache/airflow/pull/18718), where BaseSqlOperator takes `hook_params` and Connection takes `hook_kwargs`.
If this PR will be merged I'll create another one that resolves it.
Should I create an issue for that?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18431: Add `hook_params` and fix `allowed_conn_types` for SqlSensor.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750962756
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_kwargs=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_kwargs = hook_kwargs
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
- 'jdbc',
- 'mssql',
- 'mysql',
- 'odbc',
- 'oracle',
- 'postgres',
- 'presto',
- 'snowflake',
- 'sqlite',
- 'trino',
- 'vertica',
- }
- if conn.conn_type not in allowed_conn_type:
+ hook = conn.get_hook(hook_kwargs=self.hook_kwargs)
+ if not isinstance(hook, DbApiHook):
Review comment:
Same feeling, this should be a separate PR.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add `hook_params` and fix `allowed_conn_types` for SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r751078170
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_params=None,
Review comment:
Done
https://github.com/apache/airflow/issues/19638
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-924874491
Hi, @SamWheating.
Could you help me with this PR?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18431: Add hook_kwargs to SqlSensor.
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750848153
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_kwargs=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_kwargs = hook_kwargs
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
- 'jdbc',
- 'mssql',
- 'mysql',
- 'odbc',
- 'oracle',
- 'postgres',
- 'presto',
- 'snowflake',
- 'sqlite',
- 'trino',
- 'vertica',
- }
- if conn.conn_type not in allowed_conn_type:
+ hook = conn.get_hook(hook_kwargs=self.hook_kwargs)
+ if not isinstance(hook, DbApiHook):
Review comment:
Why are you making this change in a PR that purports only to add `hook_kwargs` to `SqlSensor`?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add `hook_params` and fix `allowed_conn_types` for SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r751057294
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_params=None,
Review comment:
I definitely agree with you.
But this inconsistency was provided by this PR (https://github.com/apache/airflow/pull/18718), where BaseSqlOperator takes `hook_params` and Connection takes `hook_kwargs`.
If this PR will be merged I'll create another one that resolves it.
Should I create an issue for that?
Resolve conflicting parameter SqlSensor(hook_params), BaseSqlOperator(hook_params), Connection(_**hook_kwargs**_)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18431: Add `hook_params` and fix `allowed_conn_types` for SqlSensor.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750961965
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_params=None,
Review comment:
I wonder if we should use `hook_kwargs` for consistency with `Connection.get_hook()`.
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_params=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = {} if hook_params is None else hook_params
Review comment:
The `if-else` is not needed since `Connection.get_hook()` handled None automatically.
```suggestion
self.hook_params = hook_params
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on pull request #18431: Add `hook_params` and fix `allowed_conn_types` for SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-971311918
I updated the description. Also changed name of the parameter to `hook_params` which is similar to BaseSqlOperator.
I think the parameter of the method `Connection.get_hook()` also might be changed to `hook_params`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750561700
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
We actually check for subclass of `DbApiHook` in:
https://github.com/apache/airflow/blob/dd410fd3c9de14e94034dcb4ccae52bbf5216199/airflow/operators/sql.py#L68-L73
So we should do here the same.
I'm not sure why you mention druid and elasticsearch as exceptions?
https://github.com/apache/airflow/blob/dd410fd3c9de14e94034dcb4ccae52bbf5216199/airflow/providers/apache/druid/hooks/druid.py#L131
https://github.com/apache/airflow/blob/dd410fd3c9de14e94034dcb4ccae52bbf5216199/airflow/providers/elasticsearch/hooks/elasticsearch.py#L27
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714285583
##########
File path: airflow/sensors/sql.py
##########
@@ -90,7 +94,7 @@ def _get_hook(self):
f"Connection type ({conn.conn_type}) is not supported by SqlSensor. "
+ f"Supported connection types: {list(allowed_conn_type)}"
)
- return conn.get_hook()
+ return conn.get_hook(self.hook_params)
Review comment:
```suggestion
return conn.get_hook(hook_params=self.hook_params)
```
Using a keyword argument might make this more readable and less likely to break if someone later adds another argument to the `get_hook` function.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #18431: Add `hook_params` to SqlSensor.
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-980044455
Awesome work, congrats on your first merged pull request!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750468458
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,34 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_params={},
Review comment:
same as above
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, hook_params: Dict = {}):
Review comment:
I understand your point @kazanzhy but I agree we should just stick with the practice of using `None` as the default for `Optional[Dict]` that is, as far as I know, uniformly adhered to in this repo.
If you leave it, people will always give it a second look when they see it. And pycharm complains about it. And using `None` for an optional arg is also a common pattern that I think is good aside from the mutability issue, just sends the message, you don't need to pass this, and we don't forward anything in this case.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18431: Add `hook_params` to SqlSensor.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-971709610
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714229811
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
I found the PR which introduced this change:
https://github.com/apache/airflow/pull/4723/files
But this was because at the time, the `get_hook` function looked very different:
https://github.com/apache/airflow/blob/b1acc5508a4b9aaa7c98104ba31ab0623c95f4aa/airflow/models/connection.py#L184-L190
And the `google_cloud_platform` conn type returned a BigQueryHook.
So I think you're right that this is incorrect, since this would allow someone to create a SqlSensor with a `ComputeEngineSSHHook`, which certainly wouldn't work - nice catch!
cc @XD-DENG I think a refactor might be required here - thoughts on either:
- updating the list of conn_types to include some new connections
- changing the check here to assert that a connection is a subclass of a dbApiHook (and thus has a get_records method).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dinigo commented on pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
dinigo commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-931271368
Hello there. Been on holiday for some time, thats why #17592 was stalled. I see you get good progress here, including adding the missing DbHooks. Tell me If I can contribute somehow, just don't want to duplicate efforts
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy edited a comment on pull request #18431: Add `hook_params` to SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy edited a comment on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-970740972
Hi @eladkal @dstandish.
Please take a look at the latest changes.
I've used changes from PR #18718 and doesn't make changes in `airflow/models/connection.py`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add `hook_params` to SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r751091283
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_kwargs=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_kwargs = hook_kwargs
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
- 'jdbc',
- 'mssql',
- 'mysql',
- 'odbc',
- 'oracle',
- 'postgres',
- 'presto',
- 'snowflake',
- 'sqlite',
- 'trino',
- 'vertica',
- }
- if conn.conn_type not in allowed_conn_type:
+ hook = conn.get_hook(hook_kwargs=self.hook_kwargs)
+ if not isinstance(hook, DbApiHook):
Review comment:
Done.
https://github.com/apache/airflow/pull/19639
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-970259960
Hi @eladkal.
As you see this PR was before #18718 and I didn't see that it's referred to this PR.
I'll make rebase to the current main branch and update this PR.
Thank you for your comment
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r713932904
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
As I understand, to use some connection in this sensor it hook should be an instance of DbApiHook.
But GoogleBaseHook which returns for the google_cloud_platform isn't that one.
Am I correct?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r719374081
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, hook_params: Dict = {}):
Review comment:
Yeah, I've also had to deal with the problem described in SO.
But in this case, Dict is just a container and hasn't been updated in the method.
Could you please write an example in the context of SqlSensor when we can receive the behavior similar to SO.
Because now, in m opinion, it looks like code sophistication.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18431: Add `hook_params` and fix `allowed_conn_types` for SqlSensor.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r751060607
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_params=None,
Review comment:
Yes let’s open a new issue and fix this separately. Thankfully we caught this rather quickly so we don’t need to worry about backwards compatibility as long as we can amend it before 2.3 is released!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714229811
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
I found the PR which introduced this change:
https://github.com/apache/airflow/pull/4723/files
And it looks like at the time at the time, the `get_hook` function looked very different and returned a BigQueryHook for the `google_cloud_platform` conn type.:
https://github.com/apache/airflow/blob/b1acc5508a4b9aaa7c98104ba31ab0623c95f4aa/airflow/models/connection.py#L184-L190
So I think you're right that this is incorrect, since this would allow someone to create a SqlSensor with a `ComputeEngineSSHHook`, which certainly wouldn't work - nice catch!
cc @XD-DENG I think a refactor might be required here - thoughts on either:
- updating the list of conn_types to include some new connections
- changing the check here to assert that a connection is a subclass of a dbApiHook (and thus has a get_records method).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714218813
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
Yeah, this certainly isn't a list of all DbApiHooks, since its missing `druid` and `eleasticsearch` amongst others.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714309619
##########
File path: airflow/sensors/sql.py
##########
@@ -90,7 +94,7 @@ def _get_hook(self):
f"Connection type ({conn.conn_type}) is not supported by SqlSensor. "
+ f"Supported connection types: {list(allowed_conn_type)}"
)
- return conn.get_hook()
+ return conn.get_hook(self.hook_params)
Review comment:
Definitely agree with you
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714217459
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
I think that if this were the case then it might be easier to instantiate the hook and then check if its a subclass of `dbApiHook` using something like
```python
conn = BaseHook.get_connection(self.conn_id)
hook = conn.get_hook(self.hook_params)
if issubclass(hook, DbApiHook):
....
```
So maybe this is isn't supported for all DbApiHooks? Let me see if the original PR which added this offered any explanation.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r719374081
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, hook_params: Dict = {}):
Review comment:
Yeah, I've also had to deal with the problem described in SO.
But in this case, Dict is just a container and hasn't been updated in the method.
Could you please write an example in the context of SqlSensor when we can receive the behavior similar to SO?
Because now, in my opinion, it looks like code sophistication.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18431: Add hook_kwargs to SqlSensor.
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750848153
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_kwargs=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_kwargs = hook_kwargs
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
- 'jdbc',
- 'mssql',
- 'mysql',
- 'odbc',
- 'oracle',
- 'postgres',
- 'presto',
- 'snowflake',
- 'sqlite',
- 'trino',
- 'vertica',
- }
- if conn.conn_type not in allowed_conn_type:
+ hook = conn.get_hook(hook_kwargs=self.hook_kwargs)
+ if not isinstance(hook, DbApiHook):
Review comment:
It's not clear from the PR description why you're making the change with `allowed_conn_types`. It doesn't seem like it's necessary as part of adding `hook_kwargs`, but seems completely unrelated.
Probably it makes sense to be in a separate PR. But at least the description should be updated to explain what the reason is for this change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #18431: Add `hook_params` to SqlSensor.
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #18431:
URL: https://github.com/apache/airflow/pull/18431
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr closed pull request #18431: Add `hook_params` to SqlSensor.
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #18431:
URL: https://github.com/apache/airflow/pull/18431
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714285583
##########
File path: airflow/sensors/sql.py
##########
@@ -90,7 +94,7 @@ def _get_hook(self):
f"Connection type ({conn.conn_type}) is not supported by SqlSensor. "
+ f"Supported connection types: {list(allowed_conn_type)}"
)
- return conn.get_hook()
+ return conn.get_hook(self.hook_params)
Review comment:
```suggestion
return conn.get_hook(hook_params=self.hook_params)
```
Using a keyword argument might make this more readable and less likely to break if someone later adds another argument to the `get_hook` function.
However this might be more personal opinion, thoughts?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dinigo commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
dinigo commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r719346069
##########
File path: airflow/models/connection.py
##########
@@ -289,8 +289,11 @@ def rotate_fernet_key(self):
if self._extra and self.is_extra_encrypted:
self._extra = fernet.rotate(self._extra.encode('utf-8')).decode()
- def get_hook(self):
- """Return hook based on conn_type."""
+ def get_hook(self, hook_params: Dict = {}):
Review comment:
It is discouraged to pass a mutable object as a default. See this SO https://stackoverflow.com/q/1132941/621058 . Instead you can null it by default and if it's null assign it later
```python
def get_hook(self, hook_params: Dict = None):
if hook_params is None:
hook_params = {}
# ...
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r714229811
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
I found the PR which introduced this change:
https://github.com/apache/airflow/pull/4723/files
But this was because at the time, the `get_hook` function looked very different:
https://github.com/apache/airflow/blob/b1acc5508a4b9aaa7c98104ba31ab0623c95f4aa/airflow/models/connection.py#L184-L190
And the `google_cloud_platform` conn type returned a BigQueryHook.
So I think you're right that this is incorrect, since this would allow someone to create a SqlSensor with a `ComputeEngineSSHHook`, which certainly wouldn't work.
cc @XD-DENG I think a refactor might be required here - thoughts on either:
- updating the list of conn_types to include some new connections
- changing the check here to assert that a connection is a subclass of a dbApiHook (and thus has a get_records method).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750713343
##########
File path: airflow/sensors/sql.py
##########
@@ -58,21 +61,22 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, hook_params={}, **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_params = hook_params
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
allowed_conn_type = {
- 'google_cloud_platform',
+ 'gcpbigquery',
Review comment:
Done
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-970740972
Hi @eladkal @dstandish.
Please take a look at the latest changes.
I've used changes from PR #18718 and doesn't make changes in `airflow/models/connection.py`.
Also, I'm checking the hook if it is an instance of DbApiHook.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #18431: Add hook_kwargs to SqlSensor.
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r750848153
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_kwargs=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_kwargs = hook_kwargs
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
- 'jdbc',
- 'mssql',
- 'mysql',
- 'odbc',
- 'oracle',
- 'postgres',
- 'presto',
- 'snowflake',
- 'sqlite',
- 'trino',
- 'vertica',
- }
- if conn.conn_type not in allowed_conn_type:
+ hook = conn.get_hook(hook_kwargs=self.hook_kwargs)
+ if not isinstance(hook, DbApiHook):
Review comment:
Why are you making this change in a PR that purports only to add `hook_kwargs` to `SqlSensor`? E.g. it doesn't seem like it's necessary as part of adding `hook_kwargs` -- it seems completely unrelated.
Probably it makes sense to be in a separate PR. But at least the description should be updated to explain what the reason is for this change.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kazanzhy commented on a change in pull request #18431: Add `hook_params` to SqlSensor.
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#discussion_r751091283
##########
File path: airflow/sensors/sql.py
##########
@@ -58,39 +62,35 @@ class SqlSensor(BaseSensorOperator):
ui_color = '#7c7287'
def __init__(
- self, *, conn_id, sql, parameters=None, success=None, failure=None, fail_on_empty=False, **kwargs
+ self,
+ *,
+ conn_id,
+ sql,
+ parameters=None,
+ success=None,
+ failure=None,
+ fail_on_empty=False,
+ hook_kwargs=None,
+ **kwargs
):
self.conn_id = conn_id
self.sql = sql
self.parameters = parameters
self.success = success
self.failure = failure
self.fail_on_empty = fail_on_empty
+ self.hook_kwargs = hook_kwargs
super().__init__(**kwargs)
def _get_hook(self):
conn = BaseHook.get_connection(self.conn_id)
-
- allowed_conn_type = {
- 'google_cloud_platform',
- 'jdbc',
- 'mssql',
- 'mysql',
- 'odbc',
- 'oracle',
- 'postgres',
- 'presto',
- 'snowflake',
- 'sqlite',
- 'trino',
- 'vertica',
- }
- if conn.conn_type not in allowed_conn_type:
+ hook = conn.get_hook(hook_kwargs=self.hook_kwargs)
+ if not isinstance(hook, DbApiHook):
Review comment:
Done
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #18431: Add hook_params in SqlSensor.
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #18431:
URL: https://github.com/apache/airflow/pull/18431#issuecomment-970244470
hi @kazanzhy recently https://github.com/apache/airflow/pull/18718 was merged which is similar.
I see that you are depended on https://github.com/apache/airflow/pull/17592 but since it's stale maybe you want to take over and integrate the needed changes to this PR?
This should be a relatively small change probably there is no real need to split the change into two PRs.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org