You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "HyunWooZZ (via GitHub)" <gi...@apache.org> on 2026/01/17 09:46:08 UTC
[PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
HyunWooZZ opened a new pull request, #60689:
URL: https://github.com/apache/airflow/pull/60689
### Summary
This PR enhances the Snowflake SQL API hook by consistently applying the http_timeout_seconds parameter to both synchronous and asynchronous HTTP requests.
Also it updates retry logic to consider asyncio.TimeoutError retryable.
---
##### Was generative AI tooling used to co-author this PR?
- [X] No (please specify the tool below)
---
Details
1. Propagate http_timeout_seconds to requests.Session.request(...) in _make_api_call_with_retries.
2. Configure aiohttp.ClientSession with ClientTimeout(total=...) when http_timeout_seconds is provided.
3. Expand _should_retry_on_error to treat asyncio.TimeoutError as retryable.
4. Refactor request kwargs construction to reduce duplication and centralize timeout behavior.
---
Tests
- Verified timeout propagation for synchronous HTTP requests.
- Checked that async ClientSession is configured with the expected timeout.
- Ensured async requests are retried on timeout errors.
Sync retry behavior is already extensively covered by existing tests..
So, this PR adds coverage for timeout propagation and async timeout retries.
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700939344
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
@eladkal Thank you for the suggestion 🙂
I agree that passing request parameters as kwargs can make the interface more flexible, and this is a pattern we use in other HTTP-related hooks.
That said, this hook supports both sync (requests) and async (aiohttp) execution. Their request interfaces and timeout behavior are different, so using a single generic kwargs dict would either add extra complexity or work well for only one of the two paths.
Given the nature of this workload, I didn’t see strong value in splitting the timeout into multiple phases. The SQL API call is essentially a single request, so I chose to keep a single total timeout and keep the logic simple.
For this reason, I kept the change focused on a single, explicit timeout parameter and added a comment to clarify the scope:
`# For simplicity, we only configure the total timeout (no per-phase tuning).`
I wanted to keep this PR minimal.
If I misunderstood anything, please let me know. I’m happy to adjust.
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700939344
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
@eladkal Thank you for the suggestion 🙂
I agree that passing request parameters as kwargs can make the interface more flexible, and this is a pattern we use in other HTTP-related hooks.
That said, this hook supports both sync (requests) and async (aiohttp) execution. Their request interfaces and timeout behavior are different, so using a single generic kwargs dict would either add extra complexity or work well for only one of the two paths.
Given the nature of this workload, I didn’t see strong value in splitting the timeout into multiple phases. The SQL API call is essentially a single request, so I chose to keep a single total timeout and keep the logic simple.
For this reason, I kept the change focused on a single, explicit timeout parameter and added a comment to clarify the scope:
`# For simplicity, we only configure the total timeout (no per-phase tuning).`
I wanted to keep this PR minimal and avoid expanding the public API surface until we align on a broader design.
If I misunderstood anything, please let me know. I’m happy to adjust.
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700998206
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
Sorry, I misunderstood your comment.
Currently, the invocation flow for a query in the hook is:
execute_query -> _make_api_call_with_retries -> request
So we need to change execute_query signature also async method also.
e.g pseudo code like this.
```python
def execute_query(some_args, http_args):
_make_api_call_with_retries(some_args, http_args)
def _make_api_call_with_retries(origin_param, http_args=origin_default_args):
request(http_args)
```
In short, I understand this as a suggestion to shift the responsibility to the calling side.
Is that right?
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2701014921
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> think we should avoid it.
This is just 1 parameter out of so many that can be passed to request.
I rather have request_kwargs then each user can pass any parameter accepted by requests.
First i understood change :param http_timeout_seconds: -> :param request_kwargs:.
So i thought parsing request_kwargs could lead code complexity.
>I think that is how we do it in other http related operators?
I reference dbt hook :)
https://airflow.apache.org/docs/apache-airflow-providers-dbt-cloud/stable/_modules/airflow/providers/dbt/cloud/hooks/dbt.html
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700939344
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
@eladkal Thank you for the suggestion 🙂
I agree that passing request parameters as kwargs can make the interface more flexible, and this is a pattern we use in other HTTP-related hooks.
That said, this hook supports both sync (requests) and async (aiohttp) execution. Their request interfaces and timeout behavior are different, so using a single generic kwargs dict would either add extra complexity or work well for only one of the two paths.
Given the nature of this workload, I didn’t see strong value in splitting the timeout into multiple phases. The SQL API call is essentially a single request, so I chose to keep a single total timeout and keep the logic simple.
For this reason, I kept the change focused on a single, explicit timeout parameter and added a comment to clarify the scope:
`# For simplicity, we only configure the total timeout (no per-phase tuning).`
If I misunderstood anything, please let me know. I’m happy to adjust.
--
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
Re: [PR] (feat): add HTTP request customization parameters to `SnowflakeSqlApiHook` [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #60689:
URL: https://github.com/apache/airflow/pull/60689#issuecomment-3776813971
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional 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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2701014921
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> think we should avoid it.
This is just 1 parameter out of so many that can be passed to request.
I rather have request_kwargs then each user can pass any parameter accepted by requests.
First i understood change :param http_timeout_seconds: -> :param request_kwargs:.
I just wanted to satisfy both sync and async http request.
But if we pass kwargs, I thought we need to pass kwargs for both or kwargs like
`one_kwargs = {"sync": dict(some), "async_session": dict{some}, "async_request": dict{some}}`
So i thought that is not intuitive way.
>I think that is how we do it in other http related operators?
I reference dbt hook :)
https://airflow.apache.org/docs/apache-airflow-providers-dbt-cloud/stable/_modules/airflow/providers/dbt/cloud/hooks/dbt.html
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2701014921
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> think we should avoid it.
This is just 1 parameter out of so many that can be passed to request.
I rather have request_kwargs then each user can pass any parameter accepted by requests.
First i understood change :param http_timeout_seconds: -> :param request_kwargs:.
The main issue with a single request_kwargs is that we have two different underlying clients (requests for sync and aiohttp for async) with non-identical kwargs and timeout semantics.
For example, sync requests accepts timeout as a number or (connect, read) tuple, while aiohttp uses ClientTimeout(...) (and the keys/types could be differ).
Because above reason, I just wanted to hide complexity, and focuse on how to use hook not struggling with other options.
Also we need to guard against users overriding hook-controlled fields (like: method/url/headers/params), so we can’t blindly forward everything without validation.
That’s why I initially kept an explicit http_timeout_seconds to provide consistent behavior across sync and async both with minimal surface area.
If the project prefers a kwargs-based passing, I’m happy to refactor into separate kwargs (e.g., http_request_kwargs, aiohttp_session_kwargs, aiohttp_request_kwargs) or another pattern you recommend.
>I think that is how we do it in other http related operators?
I reference dbt hook :)
https://airflow.apache.org/docs/apache-airflow-providers-dbt-cloud/stable/_modules/airflow/providers/dbt/cloud/hooks/dbt.html
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700998206
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
Sorry, I misunderstood your comment.
Currently, the invocation flow for a query in the hook is:
execute_query -> _make_api_call_with_retries -> request
So we need to change execute_query signature also async method also.
e.g pseudo code like this.
```python
def execute_query(some_args, http_args=default_args):
_make_api_call_with_retries(some_args, http_args)
def _make_api_call_with_retries(origin_param, http_args):
request(http_args)
```
In short, I understand this as a suggestion to shift the responsibility to the calling side.
Is that right?
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2701014921
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> think we should avoid it.
This is just 1 parameter out of so many that can be passed to request.
I rather have request_kwargs then each user can pass any parameter accepted by requests.
First i understood change :param http_timeout_seconds: -> :param request_kwargs:.
I just wanted to satisfy both sync and async http request.
But if we pass kwargs, I thought we need to pass kwargs for both or kwargs like
`one_kwargs = dict{sync: dict(some), async_session: dict{some}, async_request: dict{some}}`
So i thought that is not intuitive way.
>I think that is how we do it in other http related operators?
I reference dbt hook :)
https://airflow.apache.org/docs/apache-airflow-providers-dbt-cloud/stable/_modules/airflow/providers/dbt/cloud/hooks/dbt.html
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2710365923
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
This looks good now. I like the pattern. Having it as unified interface would be possibly easier from the UX point of view but indeed this is more flexible and gives more control.
--
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
Re: [PR] (feat): add HTTP request customization parameters to `SnowflakeSqlApiHook` [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #60689:
URL: https://github.com/apache/airflow/pull/60689
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700975176
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -472,9 +477,10 @@ def _make_api_call_with_retries(
for attempt in Retrying(**self.retry_config): # type: ignore
with attempt:
if method.upper() in ("GET", "POST"):
- response = session.request(
- method=method.lower(), url=url, headers=headers, params=params, json=json
- )
+ request_kwargs = dict(method=method.lower(), url=url, headers=headers, params=params, json=json)
Review Comment:
I'll make this change with the below review is complete.
https://github.com/apache/airflow/pull/60689/files#r2700868724
```python
request_kwargs: dict[str, Any] = {
"method": method.lower(),
"url": url,
"headers": headers,
"params": params,
"json": json,
}
```
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700868724
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
I think we should avoid it.
This is just 1 parameter out of so many that can be passed to request.
I rather have `request_kwargs` then each user can pass any parameter accepted by requests.
I think that is how we do it in other http related operators?
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #60689:
URL: https://github.com/apache/airflow/pull/60689#issuecomment-3762959196
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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-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/airflow-core/docs/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/dev/breeze/doc/README.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-docs/05_pull_requests.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2701014921
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> think we should avoid it.
This is just 1 parameter out of so many that can be passed to request.
I rather have request_kwargs then each user can pass any parameter accepted by requests.
First i understood change :param http_timeout_seconds: -> :param request_kwargs:.
I just wanted to satisfy both sync and async http request.
But if we pass kwargs, I thought we need to pass kwargs for both or kwargs like
`dict {sync: dict(some), async_session: dict{some}, async_request: dict{some}}`
So i thought that is not intuitive way.
>I think that is how we do it in other http related operators?
I reference dbt hook :)
https://airflow.apache.org/docs/apache-airflow-providers-dbt-cloud/stable/_modules/airflow/providers/dbt/cloud/hooks/dbt.html
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700998206
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
Sorry, I misunderstood your comment.
Currently, the invocation flow for a query in the hook is:
execute_query -> _make_api_call_with_retries -> request
~~So we need to change execute_query signature also async method also.~~
~~e.g pseudo code like this.~~
```python
def execute_query(some_args, http_args=default_args):
_make_api_call_with_retries(some_args, http_args)
def _make_api_call_with_retries(origin_param, http_args):
request(http_args)
```
In short, I understand this as a suggestion to shift the responsibility to the calling side.
Is that right?
You can find this change in another commit.
Plz view in another commit. :)
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700980684
##########
providers/snowflake/tests/unit/snowflake/hooks/test_snowflake_sql_api.py:
##########
@@ -1471,3 +1472,64 @@ def test_cancel_queries(self, mock_cancel_execution):
assert mock_cancel_execution.call_count == 3
mock_cancel_execution.assert_has_calls([call("query-1"), call("query-2"), call("query-3")])
+
+ def test_make_api_call_passes_timeout_to_requests(self, mock_requests):
+ hook = SnowflakeSqlApiHook(snowflake_conn_id="test_conn", http_timeout_seconds=12)
+
+ resp = mock.MagicMock()
+ resp.status_code = 200
+ resp.raise_for_status.return_value = None
+ resp.json.return_value = {"ok": True}
+ mock_requests.request.return_value = resp
+
+ hook._make_api_call_with_retries("GET", API_URL, HEADERS)
+
+ mock_requests.request.assert_called_once_with(
+ method="get",
+ url=API_URL,
+ headers=HEADERS,
+ params=None,
+ json=None,
+ timeout=12.0,
+ )
+
+
+ @pytest.mark.asyncio
+ async def test_make_api_call_with_retries_async_passes_timeout_to_clientsession(self):
+ hook = SnowflakeSqlApiHook(snowflake_conn_id="test_conn", http_timeout_seconds=7)
+
+ with mock.patch(f"{MODULE_PATH}.aiohttp.ClientSession") as client_session_cls:
+ session_cm = mock.MagicMock()
+ client_session_cls.return_value.__aenter__ = AsyncMock(return_value=session_cm)
+
+ req_cm = mock.MagicMock()
+ session_cm.request.return_value = req_cm
+
+ resp = mock.MagicMock()
+ resp.status = 200
+ resp.raise_for_status.return_value = None
+ resp.json = AsyncMock(return_value=GET_RESPONSE)
+ req_cm.__aenter__ = AsyncMock(return_value=resp)
+
+ await hook._make_api_call_with_retries_async("GET", API_URL, HEADERS)
+
+ _, kwargs = client_session_cls.call_args
+ timeout_obj = kwargs["timeout"]
+ assert isinstance(timeout_obj, aiohttp.ClientTimeout)
+ assert timeout_obj.total == 7.0
+
+
+ @pytest.mark.asyncio
+ async def test_make_api_call_with_retries_async_retries_on_timeout_error(self, mock_async_request):
+ hook = SnowflakeSqlApiHook(snowflake_conn_id="test_conn", http_timeout_seconds=7)
+
+ mock_async_request.__aenter__.side_effect = [
+ asyncio.TimeoutError(),
+ create_async_request_client_response_success(json=GET_RESPONSE, status_code=200),
+ ]
+
+ status, data = await hook._make_api_call_with_retries_async("GET", API_URL, HEADERS)
+
+ assert status == 200
+ assert data == GET_RESPONSE
+ assert mock_async_request.__aenter__.call_count == 2
Review Comment:
I'll add a newline at the end of the file.
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700972737
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> so using a single generic kwargs dict would either add extra complexity or work well for only one of the two paths.
can you explain the complexity?
The way I see it you get a dic from user and you just pass it as parameter to the underlying library (whatever that may be). What am I missing?
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> so using a single generic kwargs dict would either add extra complexity or work well for only one of the two paths.
can you explain the complexity?
The way I see it you get a dict from user and you just pass it as parameter to the underlying library (whatever that may be). What am I missing?
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2700998206
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
Sorry, I misunderstood your comment.
Currently, the invocation flow for a query in the hook is:
execute_query -> _make_api_call_with_retries -> request
So we need to change execute_query signature also async method also.
e.g pseudo code like this.
```python
def execute_query(some_args, http_args):
_make_api_call_with_retries(some_args, http_args=origin_default_args)
def _make_api_call_with_retries(origin_param, http_args):
request(http_args)
```
In short, I understand this as a suggestion to shift the responsibility to the calling side.
Is that right?
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "HyunWooZZ (via GitHub)" <gi...@apache.org>.
HyunWooZZ commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2701014921
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
> think we should avoid it.
This is just 1 parameter out of so many that can be passed to request.
I rather have request_kwargs then each user can pass any parameter accepted by requests.
First i understood change :param http_timeout_seconds: -> :param request_kwargs:.
So i think parsing that could make complexity.
>I think that is how we do it in other http related operators?
I reference dbt hook :)
https://airflow.apache.org/docs/apache-airflow-providers-dbt-cloud/stable/_modules/airflow/providers/dbt/cloud/hooks/dbt.html
--
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
Re: [PR] Snowflake: At SQL API hook apply http_timeout_seconds to sync/async requests and treat asyncio.TimeoutError as retryable. [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #60689:
URL: https://github.com/apache/airflow/pull/60689#discussion_r2710366405
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake_sql_api.py:
##########
@@ -78,6 +79,7 @@ class SnowflakeSqlApiHook(SnowflakeHook):
:param token_renewal_delta: Renewal time of the JWT Token in timedelta
:param deferrable: Run operator in the deferrable mode.
:param api_retry_args: An optional dictionary with arguments passed to ``tenacity.Retrying`` & ``tenacity.AsyncRetrying`` classes.
+ :param http_timeout_seconds: Optional timeout for http requests.
Review Comment:
@eladkal ?
--
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