-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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 airflow celery stop
to accept the pid file.
#17278
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the linter will complain, and we probably need to make sure:
- The custom PID path actually works as we expect.
- The
--pid
-less command form still continues to work.
In other words, you should not modify the current test_same_pid_file_is_used_in_start_and_stop
test, and instead add a new test that runs the worker in a non-standard PID file location to check the --pid
option actually has an effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of defensive coding improvements, the logic lgtm!
Awesome work, congrats on your first merged pull request! |
This bot makes me happy every time. |
Fix `airflow celery stop` to accept the pid file. (apache#17278) Fix isort (apache#17330) Fix failing celery test (apache#17337) This change fixes failing test due to mocking
Background
Airflow celery cli provides a nice interface to launch workers and stop them. This cli setup works very well when there is one worker running on node without a custom PID specified. But if a worker is launched with custom
--pid
file thenairflow celery stop
command is not capable of stopping the worker. In our setup, we run multiple workers on same node subscribed to different queues. The only way to stop worker is to provideSIGTERM
, which isn't always the best strategy as it is not always graceful.Changes made
Add an additional argument to the
airflow celery stop
command,--pid
. If this argument is present then the cli will kill worker associated with pid present in specified pidfile.