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/20 22:48:27 UTC
[GitHub] [airflow] kazanzhy opened a new pull request #18394: Add hook_params in SqlSensor
kazanzhy opened a new pull request #18394:
URL: https://github.com/apache/airflow/pull/18394
Duplicate of https://github.com/apache/airflow/pull/17592
closes: #13750
related: #17315
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 (as mentioned in the #13750).
--
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 closed pull request #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy closed pull request #18394:
URL: https://github.com/apache/airflow/pull/18394
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#issuecomment-923410305
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712574648
##########
File path: airflow/models/connection.py
##########
@@ -308,7 +311,7 @@ def get_hook(self):
"Could not import %s when discovering %s %s", hook_class_name, hook_name, package_name
)
raise
- return hook_class(**{conn_id_param: self.conn_id})
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_params)
Review comment:
I just tried to make fewer changes, sir. In my opinion your changes are more elegant :)
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712573272
##########
File path: airflow/models/connection.py
##########
@@ -308,7 +311,7 @@ def get_hook(self):
"Could not import %s when discovering %s %s", hook_class_name, hook_name, package_name
)
raise
- return hook_class(**{conn_id_param: self.conn_id})
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_params)
Review comment:
Nevermind, I just realized that `conn_id_param` refers to a variable, not a constant. Neat!
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712572582
##########
File path: airflow/models/connection.py
##########
@@ -308,7 +311,7 @@ def get_hook(self):
"Could not import %s when discovering %s %s", hook_class_name, hook_name, package_name
)
raise
- return hook_class(**{conn_id_param: self.conn_id})
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_params)
Review comment:
```suggestion
return hook_class(conn_id_param=self.conn_id, **hook_params)
```
Maybe I'm missing something but I don't understand the usage of dictionary unpacking for a predetermined dictionary here π€
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712575171
##########
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 = {}):
+ """
+ Return hook based on conn_type
+ :param hook_params: kwargs dictionary that puts to hook constructor
Review comment:
```suggestion
:param hook_params: dictionary of keyword arguments to be passed to the hook constructor
```
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712581445
##########
File path: airflow/models/connection.py
##########
@@ -308,7 +311,7 @@ def get_hook(self):
"Could not import %s when discovering %s %s", hook_class_name, hook_name, package_name
)
raise
- return hook_class(**{conn_id_param: self.conn_id})
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_params)
Review comment:
I think my suggested changes are actually incorrect - `conn_id_param` is a variable, not a `"conn_id_param"` string.
The dict unpacking allows the parameter passed to `hook_class()` to change, since the Hooks all take different parameters to define the connection they should be using (`gcp_conn_id`, `aws_conn_id`, `airbyte_conn_id`, etc).
Sorry for the confusion, but you should ignore my initial suggestion and return this code to the way you originally had written it π
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
kazanzhy commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712824846
##########
File path: airflow/models/connection.py
##########
@@ -308,7 +311,7 @@ def get_hook(self):
"Could not import %s when discovering %s %s", hook_class_name, hook_name, package_name
)
raise
- return hook_class(**{conn_id_param: self.conn_id})
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_params)
Review comment:
Yeap, I get it also tonight. Sorry about that. Now looks 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] SamWheating commented on a change in pull request #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712581445
##########
File path: airflow/models/connection.py
##########
@@ -308,7 +311,7 @@ def get_hook(self):
"Could not import %s when discovering %s %s", hook_class_name, hook_name, package_name
)
raise
- return hook_class(**{conn_id_param: self.conn_id})
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_params)
Review comment:
I think my suggested changes are actually incorrect - `conn_id_param` is a variable, not a `"conn_id_param"` string.
The dict unpacking allows the parameter passed to `hook_class()` to change, since the Hooks all take different parameters to define the connection they should be using (`gcp_conn_id`, `aws_conn_id`, `airbyte_conn_id`, etc).
Sorry for the confusion, but you should ignore my initial suggestion and return this code to its original state π
--
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 #18394: Add hook_params in SqlSensor
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18394:
URL: https://github.com/apache/airflow/pull/18394#discussion_r712581445
##########
File path: airflow/models/connection.py
##########
@@ -308,7 +311,7 @@ def get_hook(self):
"Could not import %s when discovering %s %s", hook_class_name, hook_name, package_name
)
raise
- return hook_class(**{conn_id_param: self.conn_id})
+ return hook_class(**{conn_id_param: self.conn_id}, **hook_params)
Review comment:
I think my suggested changes are actually incorrect - `conn_id_param` is a variable, not a `"conn_id_param"` string.
The dict unpacking allows the key of the parameter passed to `hook_class()` to change, since the Hooks all take different parameters to define the connection they should be using (`gcp_conn_id`, `aws_conn_id`, `airbyte_conn_id`, etc).
Sorry for the confusion, but you should ignore my initial suggestion and return this code to the way you originally had written it π
--
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