Vamsi-klu commented on PR #63594:
URL: https://github.com/apache/airflow/pull/63594#issuecomment-4061359556
Hey @mango766, nice catch on this one — the double problem (TypeError from
RemoteLogIO getting unexpected kwargs + handler params being silently discarded
via the `= {}` reset) is a real gotcha. The fix looks solid overall. A couple
of things I wanted to flag:
**The tests don't exercise the actual code path.** Right now they
re-implement the dictionary splitting logic in the test itself and assert it
works — which is really just testing that Python dict comprehensions work. They
don't import or execute anything from `airflow_local_settings.py`, so they
won't catch wiring issues. Even a simple test that constructs, say,
`S3RemoteLogIO` with `max_bytes` as a kwarg and asserts a `TypeError` would
demonstrate the bug exists and prove the fix prevents it.
**The `_HANDLER_LEVEL_KWARGS` set is coupled to
`FileTaskHandler.__init__`.** If that class ever gains new parameters, this set
needs manual updating. Not a blocker since the signature has been stable for
years, but maybe a short comment noting the coupling would help future readers.
**Missing newsfragment** — since this is a user-visible bugfix (configuring
`remote_task_handler_kwargs` with handler-level params was broken), it should
have a newsfragment per project conventions.
**One edge case to consider:** if someone happens to have a custom
RemoteLogIO that uses a parameter named `max_bytes`, `backup_count`, or
`delay`, those would now get silently routed to the handler config instead.
Unlikely in practice, but worth being aware of.
The removal of the `remote_task_handler_kwargs = {}` resets across all
branches is a nice cleanup — that pattern was masking the real issue. Good work
tracking this down.
--
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]
