Skip to content

Conversation

@drago-f5a
Copy link
Contributor

#13469)

A key consequence of this fix is that webserver will properly
exit when gunicorn master dies and stops responding to signals.

…pache#13469)

A key consequence of this fix is that webserver will properly
exit when gunicorn master dies and stops responding to signals.
@boring-cyborg boring-cyborg bot added area:CLI area:webserver Webserver related Issues labels Jan 6, 2021
@kaxil kaxil requested a review from mik-laj January 7, 2021 00:40
@ashb ashb changed the title Correct the logic for webserver choosing number of workers to spawn (… Fix webserver exiting when gunicorn master crashes Jan 7, 2021
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jan 13, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

self.num_workers_expected - num_workers_running, self.worker_refresh_batch_size
)
self.log.debug(
self.log.info(
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for changing the log level? @drago-f5a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaxil
This action (spawning new workers) is taken specifically to correct an issue that was detected and logged at the error level few lines of code above. Given the prior error level log, it seemed reasonable to log an attempt at corrective action at the info level.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, can you create a new PR please to change the log level back to info. I revert that changed before merging the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaxil Sure, I'll do it and I'll tag you on it.

@kaxil kaxil merged commit 8a4bd3c into apache:master Jan 19, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Jan 19, 2021

Awesome work, congrats on your first merged pull request!

@kaxil kaxil added this to the Airflow 2.0.1 milestone Jan 19, 2021
kaxil pushed a commit that referenced this pull request Jan 21, 2021
* Correct the logic for webserver choosing number of workers to spawn (#13469)

A key consequence of this fix is that webserver will properly
exit when gunicorn master dies and stops responding to signals.

(cherry picked from commit 8a4bd3c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants