henry3260 commented on code in PR #74921:
URL: https://github.com/apache/airflow/pull/74921#discussion_r4325082930
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/kube_client.py:
##########
@@ -60,21 +64,26 @@ class _TimeoutK8sApiClient(client.ApiClient):
"""
Wrapper around kubernetes sync ApiClient to set default timeout.
- When *disable_verify_ssl* is True the TLS certificate check is turned
off
- on the *client_configuration* that is passed (or on a fresh default
copy)
- so that callers do not need to repeat this logic at every call-site.
+ When *disable_verify_ssl* is True the TLS certificate check is turned
off, and when
+ *should_enable_tcp_keepalive* is True TCP keepalive is enabled, on the
*configuration*
+ that is passed (or on a fresh default copy) so that callers do not
need to repeat
+ this logic at every call-site.
"""
def __init__(
self,
configuration: client.Configuration | None = None,
*,
disable_verify_ssl: bool = False,
+ should_enable_tcp_keepalive: bool = False,
Review Comment:
One concept travels under three names here: the connection field is
`disable_tcp_keepalive`, the local is `use_tcp_keepalive`, and the kwarg is
`should_enable_tcp_keepalive` while `disable_verify_ssl` keeps a single name
along the same path. Could the kwarg just be `enable_tcp_keepalive` (and the
local match it)?
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/test_client.py:
##########
@@ -64,20 +73,118 @@ def test_load_config_ssl_ca_cert(self, conf, config):
assert client.api_client.configuration.ssl_ca_cert == "/path/to/ca.crt"
@pytest.mark.platform("linux")
- def test_enable_tcp_keepalive(self):
- socket_options = [
+ @pytest.mark.parametrize(
+ ("config_overrides", "expected_idle", "expected_intvl",
"expected_cnt"),
+ [
+ pytest.param({}, 120, 30, 6, id="defaults"),
+ pytest.param(
+ {
+ ("kubernetes_executor", "tcp_keep_idle"): "60",
+ ("kubernetes_executor", "tcp_keep_intvl"): "10",
+ ("kubernetes_executor", "tcp_keep_cnt"): "3",
+ },
+ 60,
+ 10,
+ 3,
+ id="custom",
+ ),
+ ],
+ )
+ def test_enable_tcp_keepalive(self, config_overrides, expected_idle,
expected_intvl, expected_cnt):
+ configuration = Configuration()
+ configuration.socket_options = None
+
+ with conf_vars(config_overrides):
+ enable_tcp_keepalive(configuration)
+
+ assert configuration.socket_options == [
+ (socket.IPPROTO_TCP, socket.TCP_NODELAY, 1),
Review Comment:
nit: The exact-equality assertion bakes urllib3's own default,
`(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)`, into the expected list, so this
test also asserts third-party behaviour and will break if urllib3 changes its
`HTTPConnection.default_socket_options`.
--
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]
