kaxil commented on code in PR #76528:
URL: https://github.com/apache/airflow/pull/76528#discussion_r4516976565
##########
providers/ssh/docs/connections/ssh.rst:
##########
@@ -51,6 +51,8 @@ Extra (optional)
* ``timeout`` - Deprecated - use conn_timeout instead.
* ``cmd_timeout`` - Timeout (in seconds) for executing the command. The
default is 10 seconds. `null` value means no timeout.
* ``compress`` - ``true`` to ask the remote client/server to compress
traffic; ``false`` to refuse compression. Default is ``true``.
+ The async hook ignores ``Compression`` from ``~/.ssh/config``, and each
compressed connection holds about 0.3 MB of zlib state;
Review Comment:
This reads as if `SSHHook` honours `Compression` from `~/.ssh/config`, but
it only takes `ProxyCommand` and `IdentityFile` from that file. Should this say
neither hook reads it?
##########
providers/ssh/src/airflow/providers/ssh/hooks/ssh.py:
##########
@@ -722,6 +732,8 @@ def _get_value(self_val, conn_val, default=None):
"port": _get_value(self.port, conn.port, SSH_PORT),
"username": _get_value(self.username, conn.login),
"password": _get_value(self.password, conn.password),
+ # asyncssh prefers no compression; offer zlib first as paramiko
does for SSHHook.
Review Comment:
This only holds from asyncssh 2.18, which moved `none` ahead of
`[email protected]`. On 2.12 to 2.17 (the floor is `asyncssh>=2.12.0`) the async
hook already offered zlib first, so for those installs the only change is that
`compress: false` now takes effect. Could the comment say the explicit list
keeps the order the same on every asyncssh version?
##########
providers/ssh/docs/connections/ssh.rst:
##########
@@ -51,6 +51,8 @@ Extra (optional)
* ``timeout`` - Deprecated - use conn_timeout instead.
* ``cmd_timeout`` - Timeout (in seconds) for executing the command. The
default is 10 seconds. `null` value means no timeout.
* ``compress`` - ``true`` to ask the remote client/server to compress
traffic; ``false`` to refuse compression. Default is ``true``.
+ The async hook ignores ``Compression`` from ``~/.ssh/config``, and each
compressed connection holds about 0.3 MB of zlib state;
+ set ``false`` on triggerers running many SSH remote job triggers.
Review Comment:
`compress` is a connection extra, so it can't be set per triggerer, and
`false` also turns compression off for `SSHHook` tasks sharing that connection.
Should this point at the connection `SSHRemoteJobOperator` uses instead?
--
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]
