jason810496 commented on code in PR #66610:
URL: https://github.com/apache/airflow/pull/66610#discussion_r3448560264
##########
airflow-core/src/airflow/jobs/triggerer_job_runner.py:
##########
@@ -428,8 +428,35 @@ def upload_to_remote(self):
# Never actually called, nothing to do
return
+ if self.ti is None:
+ # Callback triggers have no task instance — upload using the path
directly.
+ self._upload_callback_log_to_remote()
+ return
+
upload_to_remote(self.bound_logger, self.ti)
+ def _upload_callback_log_to_remote(self):
+ """Upload callback trigger logs to remote storage without a task
instance."""
+ from airflow.sdk.log import load_remote_log_handler,
relative_path_from_logger
Review Comment:
The `_upload_callback_log_to_remote` looks almost exactly same as the
existing `upload_to_remote`.
The context that I double check with Claude:
---
`sdk/log.upload_to_remote(logger, ti=None)` already defaults and accepts
`ti=None`, and `RemoteLogIO.upload(self, path, ti=None)`
(`_shared/logging/remote.py:59`) accepts it too.
`_upload_callback_log_to_remote`'s body is a near-verbatim copy of
`upload_to_remote` minus a `try/except` warning — and the caller at
`triggerer_job_runner.py:555-559` *already* wraps `factory.upload_to_remote()`
in a try/except that logs. So both the `if self.ti is None:` dispatch and the
whole helper can go; just call `upload_to_remote(self.bound_logger, self.ti)`
unconditionally. That removes ~20 lines and the `# type: ignore[arg-type]`.
--
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]
