kaxil commented on code in PR #68239:
URL: https://github.com/apache/airflow/pull/68239#discussion_r3376566954
##########
task-sdk/src/airflow/sdk/log.py:
##########
@@ -232,11 +232,14 @@ def upload_to_remote(logger: FilteringBoundLogger, ti:
RuntimeTI):
handler = load_remote_log_handler()
if not handler:
- upload_log.warning(
- "remote_log_handler_unavailable",
- ti_id=str(ti.id),
- note="Remote log handler could not be loaded; logs will be
available locally only.",
- )
+ from airflow.sdk.configuration import conf
+
+ if conf.getboolean("logging", "remote_logging", fallback=False):
Review Comment:
The guard keys off `remote_logging`, but that's narrower than what actually
decides whether a handler exists. In `resolve_remote_task_log()` a user-defined
`logging_config_class` exporting `REMOTE_TASK_LOG`, and the final unconditional
`discover_remote_log_handler()` fallback, both produce a handler without
`remote_logging` being set. So a deployment running a custom remote handler
with `remote_logging=False` that fails to load now gets no warning here (it's
still logged at info inside the factory, so not fully silent). Would it be
cleaner to have `resolve_remote_task_log()`/`load_remote_log_handler()` report
whether a handler was actually expected, and gate the warning on that, instead
of re-deriving intent from `remote_logging` at this call site?
--
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]
