SameerMesiah97 commented on PR #60254: URL: https://github.com/apache/airflow/pull/60254#issuecomment-3730370361
Overall solid PR. The wrapper class is a great approach. But there is one potential interaction I wanted to flag based on reading through the code path.. If you look at `def watch_pod_events` (should be line 1005-1054 in your branch), you will see that there is a parameter `timeout_seconds` which has a default argument of 30s (let's call this the 'Watch timeout') . As your new wrapper class `_TimeoutK8sApiClient` (which sets the 'API timeout') is injected when `def get_conn` is called by `def watch_pod_events`. In idle or stalled watch scenarios (i.e. no events received for a period), this effectively means the API timeout can act as a hard upper bound on how long the watch remains open if it is lower than `timeout_seconds`. In those cases, the watch timeout becomes a best-case limit rather than a guarantee. Normally, this would not be too concerning but as `def watch_pod_events` is a public API, and this 'API timeout' is an internal default setting, it would not hurt to document this behavior. -- 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]
