Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
eladkal commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3996274676 @ido177 can you fix static checks? -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
ido177 commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3996068212 Hi, just added `UP_FOR_RESCHEDULE` state into check and `_STATES_WITH_COMPLETED_ATTEMPT` frozenset -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
potiuk commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3995723967 @ido177 -> can you address the comments from @kaxil today ? I am planning to release 2.11.2 today and I would love to backport this one as well. -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
kaxil commented on code in PR #54547:
URL: https://github.com/apache/airflow/pull/54547#discussion_r2879817054
##
airflow-core/src/airflow/utils/log/file_task_handler.py:
##
@@ -644,7 +644,9 @@ def _read(
if ti.state in (TaskInstanceState.RUNNING, TaskInstanceState.DEFERRED)
and not has_k8s_exec_pod:
sources, served_logs = self._read_from_logs_server(ti,
worker_log_rel_path)
source_list.extend(sources)
-elif ti.state not in State.unfinished and not (local_logs or
remote_logs):
+elif (ti.state not in State.unfinished or ti.state ==
TaskInstanceState.UP_FOR_RETRY) and not (
+local_logs or remote_logs
Review Comment:
nit: This condition pattern (`not in set or == specific_member`) is a bit
fragile — if `State.unfinished` is modified in the future, this carve-out is
easy to miss. Consider defining a small frozenset at module level:
```python
_STATES_WITH_COMPLETED_ATTEMPT = frozenset({
TaskInstanceState.UP_FOR_RETRY,
TaskInstanceState.UP_FOR_RESCHEDULE,
})
```
Then:
```python
elif (ti.state not in State.unfinished or ti.state in
_STATES_WITH_COMPLETED_ATTEMPT) and not (
local_logs or remote_logs
):
```
This makes the intent self-documenting and easier to maintain.
##
airflow-core/src/airflow/utils/log/file_task_handler.py:
##
@@ -644,7 +644,9 @@ def _read(
if ti.state in (TaskInstanceState.RUNNING, TaskInstanceState.DEFERRED)
and not has_k8s_exec_pod:
sources, served_logs = self._read_from_logs_server(ti,
worker_log_rel_path)
source_list.extend(sources)
Review Comment:
`UP_FOR_RESCHEDULE` has the same bug. It is also in `State.unfinished` (see
`state.py` line 201), and a sensor task in `UP_FOR_RESCHEDULE` has completed
its current poke attempt — its logs should be viewable via the served-logs
fallback just like `UP_FOR_RETRY`.
```suggestion
elif (ti.state not in State.unfinished or ti.state in
(TaskInstanceState.UP_FOR_RETRY, TaskInstanceState.UP_FOR_RESCHEDULE)) and not (
local_logs or remote_logs
):
```
--
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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
potiuk commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3992155495 @kaxil -> are you OK with merging it ? -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
pedro-cf commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3935882676 I'm not sure if this is an issue in Airflow `3.x.x`, but there's also a related problem in `2.11.0` where the webserver always queries the last worker's hostname for all task attempts. When viewing logs from earlier attempts that ran on different workers, it causes 404s because `TaskInstance.hostname` is overwritten on each retry. @ido177 -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
pedro-cf commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3935879316 I'm not sure if this is an issue in Airflow `3.x.x`, but there's also a related problem in `2.11.0` where the webserver always queries the last worker's hostname for all task attempts. When viewing logs from earlier attempts that ran on different workers, it causes 404s because `TaskInstance.hostname` is overwritten on each retry. -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
potiuk commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3539420836 Can you please add unit test @ido177 -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
kaxil commented on PR #54547: URL: https://github.com/apache/airflow/pull/54547#issuecomment-3473410124 let's see what CI says -- 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]
Re: [PR] Fix missing logs in UI for tasks in UP_FOR_RETRY state (#54544) [airflow]
ido177 commented on code in PR #54547: URL: https://github.com/apache/airflow/pull/54547#discussion_r2356002003 ## airflow-core/src/airflow/utils/log/file_task_handler.py: ## @@ -647,7 +647,9 @@ def _read( if ti.state in (TaskInstanceState.RUNNING, TaskInstanceState.DEFERRED) and not has_k8s_exec_pod: sources, served_logs = self._read_from_logs_server(ti, worker_log_rel_path) source_list.extend(sources) -elif ti.state not in State.unfinished and not (local_logs or remote_logs): +elif (ti.state not in State.unfinished or ti.state == TaskInstanceState.UP_FOR_RETRY) and not ( Review Comment: There is an indentation error, which breaks the code structure and causes a syntax error -- 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]
