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/06/10 09:32:22 UTC
[GitHub] [airflow] NBardelot opened a new issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
NBardelot opened a new issue #16364:
URL: https://github.com/apache/airflow/issues/16364
In SSHHook the timeout argument of the constructor is used to set a connection timeout. This is fine.
But in SSHOperator the timeout argument of the constructor is used for *both* the timeout of the SSHHook *and* the timeout of the command itself. This ambiguous use of the same parameter is very dirty.
I see two ways to clean the behaviour:
1. Let the SSHHook constructor be the only way to handle the connection timeout (thus, if one wants a specific timeout they should explicitely build a hook to be passed to the operator using the operator's constructor).
2. Split the timeout argument in SSHOperator into two arguments conn_timeout and cmd_timeout for example.
The choice between 1 and 2 depends on how frequently people are supposed to want to change the connection timeout. If it is something very frequent. then go for 2. if not go for 1.
BR and thanks for the code!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-859613446
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-882211456
Yes; you can search for `DeprecationWarning` to get some inspiratrions how to format the message.
--
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 edited a comment on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-859613446
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-859634650
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] fatmumuhomer commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
fatmumuhomer commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-882105445
@potiuk or @ashb - Can someone please assign to me if no one else is already working on it? Thanks!
--
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] fatmumuhomer edited a comment on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
fatmumuhomer edited a comment on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-882208078
Just to be clear on the preferred usage going forward:
- SSHOperator uses two different timeouts:
- A connection timeout passed into SSHHook (see below)
- A command timeout passed into paramiko.SSHClient.exec_command() which sets the command’s channel timeout
- SSHHook uses a single timeout:
- A connection timeout passed into paramiko.SSHClient.connect() which is a timeout for the TCP connect.
It doesn't seem to make sense to accept a cmd_timeout in the hook, so I am planning to do the following:
- Accept both conn_timeout and cmd_timeout in SSHOperator.
- Only accept conn_timeout in SSHHook.
- Add a deprecation warning to SSHHook for the use of timeout in extra_options; favor conn_timeout if provided.
--
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] fatmumuhomer commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
fatmumuhomer commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-882208078
Just to be clear on the preferred usage going forward:
- SSHOperator uses two different timeouts:
- A connection timeout passed into SSHHook (see below)
- A command timeout passed into paramiko.SSHClient.exec_command() which sets the command’s channel timeout
- SSHHook uses a single timeout:
- A connection timeout passed into paramiko.SSHClient.connect() which is a timeout for the TCP connect.
It doesn't seem to make sense to accept a cmd_timeout in the hook, so I am planning to do the following:
- Accept both conn_timeout and cmd_timeout in SSHOperator.
- Accept conn_timeout only in SSHHook.
- Add a deprecation warning to SSHHook for the use of timeout in extra_options; favor conn_timeout if provided.
--
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 commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-882105958
Please!
--
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 issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-859560606
I feel Option 2 is clearer regardless, since it is still quite confusing if `SSHHook(timeout=...)` and `SSHOperator(timeout=...)` mean two different things. It’s probably better to be explicit about what the timeout is for:
1. Create `conn_timeout` and pass it to `SSHCHook(timeout)`.
2. Create `cmd_timeout` for the command line timeout.
3. Keep `SSHOperator(timeout)`; if it’s passed, set `conn_timeout` and `cmd_timeout` to `timeout` if they are `None`. `TypeError` if `timeout` is all three are passed.
4. (Optional) Deprecate `SSHOperator(timeout)` in favour of `conn_timeout` and `cmd_timeout`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
potiuk closed issue #16364:
URL: https://github.com/apache/airflow/issues/16364
--
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] fatmumuhomer commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
fatmumuhomer commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-882209613
Thanks, @uranusjr. For the deprecation warning, is it fair to warn that it will be removed in a future version without specifying the exact one?
--
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] fatmumuhomer commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
fatmumuhomer commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-886904138
@uranusjr, @potiuk I created a pull request for this change - https://github.com/apache/airflow/pull/17236
Can someone take a look when you have time? Also, it says I need a maintainer to approve running workflows since I am a first time contributor.
Thank 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] uranusjr commented on issue #16364: Timeout is ambiguous in SSHHook and SSHOperator
Posted by GitBox <gi...@apache.org>.
uranusjr commented on issue #16364:
URL: https://github.com/apache/airflow/issues/16364#issuecomment-882209178
Sounds reasonable to me. You’ll also need to keep the `timeout` arguments on both the operator and hook for backward compatibility (also with a deprecation warning).
--
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