Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip served logs for non-running task try #32561

Merged
merged 4 commits into from
Aug 5, 2023
Merged

Skip served logs for non-running task try #32561

merged 4 commits into from
Aug 5, 2023

Conversation

Khrol
Copy link
Contributor

@Khrol Khrol commented Jul 12, 2023

If the task is running but is retried, the previous attempt logs opening tries to go to served logs. But the logs from there can be already moved to remote storage.
Screenshot 2023-07-12 at 18 39 51

We should skip served logs for non-running task try.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Khrol Khrol marked this pull request as ready for review July 12, 2023 17:57
@potiuk
Copy link
Member

potiuk commented Jul 15, 2023

cc: @dstandish ?

@potiuk potiuk requested a review from dstandish July 15, 2023 08:52
@eladkal eladkal added this to the Airflow 2.6.4 milestone Jul 26, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Aug 1, 2023
@eladkal eladkal modified the milestones: Airflow 2.6.4, Airflow 2.7.0 Aug 1, 2023
@pankajkoti
Copy link
Member

Looks correct to me

@eladkal eladkal merged commit 29d5e95 into apache:main Aug 5, 2023
42 checks passed
ephraimbuddy pushed a commit that referenced this pull request Aug 8, 2023
Co-authored-by: eladkal <[email protected]>
(cherry picked from commit 29d5e95)
@kahlstrm
Copy link
Contributor

This has caused a regression in our use case, as we currently store our logs on the worker with a Persistent Volume with a 2 week retention period. Now when looking at a DAG in our configuration with 2 fails and one retry running, logs of tries number 1 and 2 aren't visible. This happens because the current flow doesn't fetch logs from worker for a running task instance when the try number is not the most recent one.

Sure, we could refactor to use remotes but I think there is a way of resolving this that works for both use cases.

From what I understand the original problem @Khrol had was that they have their logs in remote and then the server sends an "unnecessary" request trying to fetch logs from the worker. This can be also achieved with just checking if not remote_logs prior to trying to fetch from worker instead of never trying to fetch from the worker.

@potiuk
Copy link
Member

potiuk commented Apr 22, 2024

From what I understand the original problem @Khrol had was that they have their logs in remote and then the server sends an "unnecessary" request trying to fetch logs from the worker. This can be also achieved with just checking if not remote_logs prior to trying to fetch from worker instead of never trying to fetch from the worker.

Surely, If you would like to propose a PR for that where it could be discussed, and take a lead on that - that's the best way to move it forward.

dstandish added a commit to astronomer/airflow that referenced this pull request Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants