korex-f commented on PR #66633: URL: https://github.com/apache/airflow/pull/66633#issuecomment-5043819996
> 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: > > * **[Fix CloudWatch remote logging for ephemeral lifecycle executor #68779](https://github.com/apache/airflow/pull/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 [Cloudwatch remote logging does not work for ECS Executor #66475](https://github.com/apache/airflow/issues/66475) symptom at the provider layer. > > * **[Upload task logs even when final state update fails #67935](https://github.com/apache/airflow/pull/67935)** (merged) moved `_upload_logs()` into a `finally` after `update_task_state_if_needed()`, so logs upload even when the state update raises. > > * **[Remote logging fix #68370](https://github.com/apache/airflow/pull/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. Thanks for the detailed audit @kaxil — really helpful context. You're right that #68779, #67935, and #68370 together cover the provider-layer symptom. Looking at what's left: **log.py reorder (Bug 1):** `getattr(remote, "processors")` was called before `dictConfig` ran, which meant `dictConfig`'s `_clearExistingHandlers` closed the just-built watchtower handler before any task log was emitted. With #68779's self-healing property this is no longer a live drop, the handler rebuilds on next access, but the reorder is still the correct root-cause fix and removes the need for the rebuild in the first place. It's a cleaner argument even if it's no longer strictly necessary for correctness. **supervisor.py `_close_remote_log_handler`:** You're right this now overlaps with `upload() → close()` from #67935/#68779. I'm happy to drop the supervisor.py changes entirely if you think log.py alone is worth keeping, or close the whole PR if you think it's fully superseded. I'd rather do the right thing for the codebase than land something redundant. What would you prefer? -- 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]
