dlamblin commented on a change in pull request #4566: [AIRFLOW-3566] - Adding
timeout and login_timeout to MsSqlHook
URL: https://github.com/apache/airflow/pull/4566#discussion_r251271683
##########
File path: airflow/hooks/mssql_hook.py
##########
@@ -45,7 +47,9 @@ def get_conn(self):
user=conn.login,
password=conn.password,
database=self.schema or conn.schema,
- port=conn.port)
+ port=conn.port,
+ timeout=self.timeout,
+ login_timeout=self.login_timeout or conn.login_timeout)
Review comment:
I think it should be in extras but also default to not being assigned if not
in the extras nor hook init, so that the pymssql connect default can be used.
That would imply the doc additions like:
https://airflow.readthedocs.io/en/latest/code.html#airflow.hooks.mysql_hook.MySqlHook
I also like how the mysql hook's extras are covered in:
https://github.com/apache/airflow/blob/1ab659f4cb4f2af49205fca57758a72f6341a125/docs/howto/manage-connections.rst
and think it would be useful if mssql hook's were too.
Unfortunately with this syntax if login_timeout were None, False or 0 they
all mean to use the extras; and I'm not sure if 0 is a valid choice… but if it
is, it wouldn't work. OTOH if 0 is not valid then this is fine.
The hook needs a docstring with descriptions of the init parameter and types
similar to http://pymssql.org/en/stable/ref/pymssql.html#pymssql.connect.
Stylistically I'd break out the closing parent to it's own line and add a
trailing comma to line 52, but… maybe that's not common in the project.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
