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

Refactor commands to unify daemon context handling #34945

Merged
merged 25 commits into from
Oct 24, 2023

Conversation

Bisk1
Copy link
Contributor

@Bisk1 Bisk1 commented Oct 14, 2023

This refactoring is a follow-up of #34931

  • unified approach to daemon process creation and moved all daemon-related code to shared util daemon_utils.py.
  • moved async thread creation for scheduler so that it happens after entering daemon context (will prevent possible bugs like Airflow 2.7.1 can not start Scheduler & trigger #34816 for scheduler)
  • reused some utils like removing PID file of a dead process for all other commands in daemon mode

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

@Bisk1 Bisk1 marked this pull request as draft October 15, 2023 18:53
@Bisk1
Copy link
Contributor Author

Bisk1 commented Oct 15, 2023

converting to draft becuase I still need to fix some unit tests

@Bisk1 Bisk1 marked this pull request as ready for review October 20, 2023 09:56
@Bisk1 Bisk1 requested a review from Taragolis October 21, 2023 12:47
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few - mostly NITs

Bisk1 and others added 4 commits October 23, 2023 18:51
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@potiuk
Copy link
Member

potiuk commented Oct 23, 2023

@Taragolis ?

@potiuk potiuk merged commit 68adc0e into apache:main Oct 24, 2023
44 checks passed
@potiuk
Copy link
Member

potiuk commented Oct 24, 2023

Nice one @Bisk1 !

@Bisk1 Bisk1 deleted the refactor-deamon-commands branch October 24, 2023 12:07
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Oct 27, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Oct 27, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 9, 2024
Celery provider has an ambedded Airflow CLI command as of 3.6.1. When
the apache#36794 was merged, we thought mistakenly that it will only be used
in airflow 2.9.0+, so we used a feature introduced in Airflow 2.8.0 in
the apache#34945 - but in fact the CLI command is configured by the Celery
Executor which is also part of the Celery provider, so it was also
used for airflow < 2.8.0 and failed due to missing import.

This PR checks if Airflow version is < 2.8.0 and if so, it falls back
to built-in airflow CLI command.
potiuk added a commit that referenced this pull request Apr 9, 2024
Celery provider has an ambedded Airflow CLI command as of 3.6.1. When
the #36794 was merged, we thought mistakenly that it will only be used
in airflow 2.9.0+, so we used a feature introduced in Airflow 2.8.0 in
the #34945 - but in fact the CLI command is configured by the Celery
Executor which is also part of the Celery provider, so it was also
used for airflow < 2.8.0 and failed due to missing import.

This PR checks if Airflow version is < 2.8.0 and if so, it falls back
to built-in airflow CLI command.
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
…e#38879)

Celery provider has an ambedded Airflow CLI command as of 3.6.1. When
the apache#36794 was merged, we thought mistakenly that it will only be used
in airflow 2.9.0+, so we used a feature introduced in Airflow 2.8.0 in
the apache#34945 - but in fact the CLI command is configured by the Celery
Executor which is also part of the Celery provider, so it was also
used for airflow < 2.8.0 and failed due to missing import.

This PR checks if Airflow version is < 2.8.0 and if so, it falls back
to built-in airflow CLI command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI area:Triggerer area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants