[AIRFLOW-1945] Autoscale celery workers for airflow added by phanindhra876 · Pull Request #3989 · apache/airflow

phanindhra876 · 2018-10-03T01:09:23Z

Dear Airflow Maintainers,

This will add a provision to autoscale celery workers unlike same numbers of workers irrespective of number of running tasks.

Please accept this PR that addresses the following issues:
https://issues.apache.org/jira/browse/AIRFLOW-1945

Testing Done:

Manually tested by passing arguments in cli

codecov-io · 2018-10-03T07:51:15Z

Codecov Report

Merging #3989 into master will decrease coverage by 3.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3989      +/-   ##
==========================================
- Coverage   75.92%   72.87%   -3.05%     
==========================================
  Files         199      199              
  Lines       15954    17003    +1049     
==========================================
+ Hits        12113    12391     +278     
- Misses       3841     4612     +771

Impacted Files	Coverage Δ
airflow/bin/cli.py	`58.76% <0%> (-6.3%)`	⬇️
airflow/hooks/druid_hook.py	`67.36% <0%> (-20.64%)`	⬇️
airflow/task/task_runner/base_task_runner.py	`60.97% <0%> (-18.34%)`	⬇️
airflow/models.py	`75.55% <0%> (-16.4%)`	⬇️
airflow/example_dags/example_python_operator.py	`78.94% <0%> (-15.79%)`	⬇️
airflow/utils/configuration.py	`85.71% <0%> (-14.29%)`	⬇️
airflow/configuration.py	`85.07% <0%> (-3.78%)`	⬇️
airflow/operators/s3_file_transform_operator.py	`93.87% <0%> (-2.35%)`	⬇️
airflow/www_rbac/views.py	`72.18% <0%> (-0.36%)`	⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a581cba...959ca5d. Read the comment docs.

ashb

Please follow the contribution guidelines.

ashb · 2018-10-03T08:46:40Z

We probably shouldn't set a default value for this and worker_concurrency. Please comment this one out.

A link to the section of the celery docs about this in the comment would help too.

I agree on not using default values but we don't have a provision to use shell functions here so that during execution we can get number of cores and all. What can be done in these scenarios.

@ashb this value has been commented out. Please review

msumit · 2018-10-08T12:29:31Z

@phani8996 whats harm in letting my machine run with the full capacity of workers all the time? Cause if I am allowing to grow it to a max, then it means that my machine has the capacity to handle that many workers anyway.

phanindhra876 · 2018-10-08T17:55:54Z

@phani8996 whats harm in letting my machine run with the full capacity of workers all the time? Cause if I am allowing to grow it to a max, then it means that my machine has the capacity to handle that many workers anyway.

We can run it at full capacity, but what advantage are we going to get with a bunch of idle workers? Instead this feature spawns workers as per demand. In a way you get what is required. No more under utilisation of workers.

Fokko · 2018-10-12T07:43:42Z

Can you add the link to: http://docs.celeryproject.org/en/latest/reference/celery.bin.worker.html#cmdoption-celery-worker-autoscale

@Fokko Link has been added. Please check.

@phani8996 a space after # would be better. i.e # worker_autoscale = 12,16

Fokko

LGTM

msumit · 2018-10-15T13:20:47Z

@phani8996 plz rebase your commits into a single commit. Also, the commit message could be like this [AIRFLOW-1945] Add Autoscale config for Celery workers

phanindhra876 · 2018-10-15T14:03:09Z

@phani8996 plz rebase your commits into a single commit. Also, the commit message could be like this [AIRFLOW-1945] Add Autoscale config for Celery workers

@msumit commits have been rebased and commit message updated with proper message. Please check.

phanindhra876 · 2018-10-15T14:09:02Z

Mistakely closed PR

ddelange · 2019-09-27T06:09:10Z

fyi, seems this functionality has been removed in celery v4.x :(
celery/celery#4003

potiuk · 2019-09-27T06:58:24Z

Taking into account the comments in the celary bug quoted by @ddelange - should we really enable this @Fokko @ashb ? Seems that autoscale option does not work in celery 4.x at all. And this the version we are using.

ashb · 2019-09-27T09:01:01Z

D'oh. Well that's annoyingly frustrating. I guess we should remove that in our next point release with a note in the updating to the lines of "We've removed this option as celery didn't respect it."

Having an extra config directive in our files won't cause any problems/errors on our side so it can be in 1.10.16. @ddelange Fancy creating such a PR? (Against master please, and I will deal with back-porting it to the release)

ddelange · 2019-09-27T10:20:26Z

I actually really like the potential of using celery autoscale (e.g. put one massive worker on an autoscaling AWS EC2 instance to solve issue of scaling airflow temporarily for a sudden heavy load).
I saw they added autoscaling back for celery milestones 4.5/4.6/4.7 or else for sure v5, so I've left the autoscaling option in our airflow config file, linking to the celery issue in our comments below the comments from default_config.cfg, and hope they fix it in future celery version, while assuming that at some point airflow will bump celery along in a future version and it'll start working again :)

right now it's not breaking anything having this option, celery only just ignores this option internally, and permanently puts it on the minimum concurrency specified. so from my side all good for now. just wanted to let you know (and anyone else who may stumble upon this PR via git blame) ^^

ashb · 2019-09-27T10:24:37Z

Sounds good. I'm okay leaving it so long as Celery still document it (even if it doesn't "work" right now) then

msumit · 2019-09-27T10:34:28Z

I see that the autoscale code is still present in Celery master https://github.com/celery/celery/blob/master/celery/worker/autoscale.py

…

On Fri, Sep 27, 2019 at 3:54 PM Ash Berlin-Taylor ***@***.***> wrote: Sounds good. I'm okay leaving it so long as Celery still document it (even if it doesn't "work" right now) then — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#3989?email_source=notifications&email_token=AAPMYZ3SF6LOXADBWCBRF3DQLXNO7A5CNFSM4FYUP5JKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7YPAXA#issuecomment-535883868>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPMYZZTNMPFMAN5QTFUOVDQLXNO7ANCNFSM4FYUP5JA> .

ashb requested changes Oct 3, 2018

View reviewed changes

Fokko reviewed Oct 12, 2018

View reviewed changes

Fokko approved these changes Oct 12, 2018

View reviewed changes

phanindhra876 closed this Oct 15, 2018

[AIRFLOW-1945] Added Autoscaling for Celery workers

959ca5d

phanindhra876 reopened this Oct 15, 2018

msumit merged commit 6097f82 into apache:master Oct 15, 2018

aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019

[AIRFLOW-1945] Add Autoscale config for Celery workers (apache#3989)

f22a65e

ashb pushed a commit to ashb/airflow that referenced this pull request Mar 5, 2019

[AIRFLOW-1945] Add Autoscale config for Celery workers (apache#3989)

dede256

wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-1945] Add Autoscale config for Celery workers (apache#3989)

706d1ca

This was referenced May 3, 2020

Fix autoscale when prefetch_multiplier is 1 celery/celery#6069

Merged

Make celery worker_prefetch_multiplier configurable #8695

Merged

Uh oh!

Conversation

phanindhra876 commented Oct 3, 2018

Uh oh!

codecov-io commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

ashb Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

ashb Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

phanindhra876 Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

phanindhra876 Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

msumit commented Oct 8, 2018

Uh oh!

phanindhra876 commented Oct 8, 2018

Uh oh!

Fokko Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

phanindhra876 Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

msumit Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

msumit commented Oct 15, 2018

Uh oh!

phanindhra876 commented Oct 15, 2018

Uh oh!

phanindhra876 commented Oct 15, 2018

Uh oh!

ddelange commented Sep 27, 2019

Uh oh!

potiuk commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashb commented Sep 27, 2019

Uh oh!

ddelange commented Sep 27, 2019

Uh oh!

ashb commented Sep 27, 2019

Uh oh!

msumit commented Sep 27, 2019 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Oct 3, 2018 •

edited

Loading

potiuk commented Sep 27, 2019 •

edited

Loading