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

Fix proper joining of the path for logs retrieved from celery workers #26493

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Sep 19, 2022

The change #26337 "fixed" the way how logs were retrieved from Celery, but it - unfortunately broke the retrieval eventually.

This PR should fix it.

Fixes: #26492


^ 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.

The change apache#26377 "fixed" the way how logs were retrieved from
Celery, but it - unfortunately broke the retrieval eventually.

This PR should fix it.

Fixes: apache#26492
@potiuk potiuk force-pushed the fix-log-retrieval-for-celery-workers branch from 824ca74 to cbdf03b Compare September 19, 2022 15:17
@potiuk potiuk mentioned this pull request Sep 19, 2022
2 tasks
@potiuk potiuk added this to the Airflow 2.4.1 milestone Sep 19, 2022
@jedcunningham jedcunningham merged commit 52560b8 into apache:main Sep 19, 2022
jedcunningham pushed a commit to astronomer/airflow that referenced this pull request Sep 19, 2022
…apache#26493)

The change apache#26377 "fixed" the way how logs were retrieved from
Celery, but it - unfortunately broke the retrieval eventually.

This PR should fix it.

Fixes: apache#26492
(cherry picked from commit 52560b8)
@kaxil
Copy link
Member

kaxil commented Sep 20, 2022

Should we add a unit test to prevent regression?

@potiuk potiuk deleted the fix-log-retrieval-for-celery-workers branch September 20, 2022 18:55
@potiuk
Copy link
Member Author

potiuk commented Sep 20, 2022

Should we add a unit test to prevent regression?

Yeah. I thought of that. I will add it.

@ashb ashb added type:bug-fix Changelog: Bug Fixes priority:high High priority bug that should be patched quickly but does not require immediate new release labels Sep 21, 2022
potiuk added a commit to potiuk/airflow that referenced this pull request Sep 22, 2022
@potiuk
Copy link
Member Author

potiuk commented Sep 22, 2022

Added Unit test in #26603 not "super" valuable, but better than nothing (and this - essentially where it failed)

potiuk added a commit that referenced this pull request Sep 22, 2022
* Add unit test for log retrieval url

Added unit test to #26493
jedcunningham pushed a commit that referenced this pull request Sep 23, 2022
…#26493)

The change #26377 "fixed" the way how logs were retrieved from
Celery, but it - unfortunately broke the retrieval eventually.

This PR should fix it.

Fixes: #26492
(cherry picked from commit 52560b8)
jedcunningham pushed a commit that referenced this pull request Sep 23, 2022
* Add unit test for log retrieval url

Added unit test to #26493

(cherry picked from commit 061caff)
@kenho811
Copy link

Thanks @potiuk

I am experiencing the same problem with my CeleryExecutor setup and it is resolved by applying the trailing slash as in this PR. :)

@potiuk
Copy link
Member Author

potiuk commented Sep 25, 2022

Cool. 2.4.1 will fix it permanently :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging priority:high High priority bug that should be patched quickly but does not require immediate new release type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot fetch log from Celery worker
6 participants