apache / apache/airflow

De-duplicate callback log reading with the task-instance log reading path

Open
#68,822 0 comments 0 reactions 0 assignees View on GitHub
area:logging
Dominant language
Python
Stars
46.9k
Forks
17.8k
Avg merge
2d 9h
Merged PRs (30d)
472

Description

Depends on #66610 getting merged first.

### Summary

PR #66610 added `airflow-core/src/airflow/utils/log/callback_log_reader.py` to read
deadline-callback execution logs for the UI. Its remote/local read helpers
re-implement logic that already exists in `FileTaskHandler`
(`airflow-core/src/airflow/utils/log/file_task_handler.py`) and is fronted by
`TaskLogReader` (`airflow-core/src/airflow/utils/log/log_reader.py`). This issue
tracks consolidating the two paths onto shared helpers as a follow-up.

This is **tech debt, not a bug** — the duplication is intentional in #66610 to keep
that PR focused.

### Duplicated logic

| `callback_log_reader.py` | Existing equivalent in `file_task_handler.py` |
| --- | --- |
| `_read_callback_remote_logs` | `FileTaskHandler._read_remote_logs` (`:967`) |
| `_read_callback_local_logs` (glob-by-prefix + `os.path.commonpath` containment) | `FileTaskHandler._read_from_local` (`:862`, `:880`) |
| remote-first-then-local fallback + `_interleave_logs` + "Log message source details" group header | `FileTaskHandler.read` (`:737`) |
| `tuple[list[str], list[RawLogStream]]` return annotation | existing `StreamingLogResponse` alias |

### Why a shared abstraction needs design (why it was deferred)

`FileTaskHandler`'s read path is TaskInstance-centric — it takes `(ti, try_number,
metadata)` and renders paths from the TI. Callback logs have **no TaskInstance**: the
path is a fixed prefix (`executor_callbacks/{dag_id}/{run_id}/{callback_id}` or
`triggerer_callbacks/...`). Extracting the common core means separating the
path-resolution step from the storage-read step so both callers can supply their own
relative path(s).

### Follow-up work

- [ ] Extract the storage-read core (remote-then-local fallback, `_interleave_logs`,
source-header emission, `os.path.commonpath` containment) into a shared helper that
takes already-resolved relative path(s) rather than a TI.
- [ ] Have both `FileTaskHandler.read` and `callback_log_reader.read_callback_log` call it.
- [ ] Annotate the callback reader helpers with the existing `StreamingLogResponse` alias.

### Acceptance criteria

- `callback_log_reader.py` no longer carries its own copies of the remote/local read and
containment logic.
- TaskInstance log reading behavior is unchanged (existing tests pass).
- Callback log reading (#66610) continues to work for both executor and triggerer paths.

### References

- PR: #66610
- Review note: https://github.com/apache/airflow/pull/66610#discussion_r3449916381

Contributor guide

Open the contributing guide

Research direction

After #66610 merges, read callback_log_reader.py alongside FileTaskHandler in file_task_handler.py and TaskLogReader in log_reader.py, focusing on the named remote, local, and read paths. Trace how each caller resolves its relative paths before designing the shared storage-read helper. Done means both readers use the shared logic, callback helpers use StreamingLogResponse, and existing task-instance and callback behavior remains intact.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.