Skip to content

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