kaxil commented on code in PR #65412:
URL: https://github.com/apache/airflow/pull/65412#discussion_r3100371715
##########
airflow-core/src/airflow/config_templates/airflow_local_settings.py:
##########
@@ -370,4 +370,4 @@ def _default_conn_name_from(mod_path, hook_name):
"section 'elasticsearch' if you are using Elasticsearch. In the
other case, "
"'remote_base_log_folder' option in the 'logging' section."
)
-
DEFAULT_LOGGING_CONFIG["handlers"]["task"].update(remote_task_handler_kwargs)
+ DEFAULT_LOGGING_CONFIG["handlers"]["task"].update(_handler_kwargs)
Review Comment:
The fix looks correct and restores the pre-#64832 behavior. Two small
follow-ups worth considering:
**1. Regression test.** The bug only triggers when `remote_logging=True`
with a backend that resets `_handler_kwargs` (S3/Wasb/GCS/OSS/HDFS/CloudWatch),
so it slipped through #64832. A parametrized test that reloads
`airflow_local_settings` under each backend (with `mock.patch` on the
`RemoteLogIO` import and `conf_vars` setting `remote_task_handler_kwargs` to a
dict with `remote_base`/custom keys), then asserts the local `task` handler
dict doesn't contain those keys, catches it in ~10s without needing real
credentials. I validated it locally: fails on all six params without the fix,
passes with it.
**2. Newsfragment.** No `65412.bugfix.rst` in this PR. Since this is a
user-visible startup failure when remote logging is configured, it's worth a
line in release notes.
--
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]
