kaxil commented on PR #66633: URL: https://github.com/apache/airflow/pull/66633#issuecomment-5038085928
Picking up on @o-nikolas's merge question -- I think this needs a second look at scope before it goes in, because a fair bit of it landed via other PRs while this was open: - **#68779** (merged) made the CloudWatch `RemoteLogIO` self-healing: the `handler` property rebuilds if `dictConfig` closed it mid-task, and `upload()` now calls `close()` to flush pending events. That covers the #66475 symptom at the provider layer. - **#67935** (merged) moved `_upload_logs()` into a `finally` after `update_task_state_if_needed()`, so logs upload even when the state update raises. - **#68370** (merged) fixed the cached-`None`-connection case so the worker can upload at all. Given that, the supervisor.py side here (explicit `_close_remote_log_handler` on teardown) now overlaps with `_upload_logs()` → `upload()` → `close()`, which already runs on the state-update error path after #67935. The log.py reorder (build the handler after `dictConfig`) is the one piece not on main, but with the self-healing handler it's no longer a live drop -- more of a cleaner-root-cause argument. @korex-f -- is there a failure mode left that the three merged PRs don't cover? If the log.py reorder is the remaining value, it may be worth narrowing this to just that (with a test); otherwise it looks mostly superseded now. Not trying to discount the work here, just want to make sure we're not re-landing something. -- 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]
