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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
