Parametrized keda task concurrency in chart #13571
Conversation
FloChehab
force-pushed
the
support-paretrized-nb-tasks-per-worker-with-keda
branch
from
087b3d3 to
ba49005
Compare
|
(I've rebased / push -f to fix conflict in the chart Readme) |
FloChehab
force-pushed
the
support-paretrized-nb-tasks-per-worker-with-keda
branch
from
ba49005 to
434d576
Compare
|
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. |
github-actions
bot
added
the
okay to merge
There was a problem hiding this comment.
i wonder if a better solution would be to allow user to specify the entire query, instead of worker_concurrency. for one, this would resolve the problem highlighted in my other comment. additionally though it would open the door to support for mysql. mysql has a different query syntax but otherwise there is a keda trigger for it and we could add support for it by enabling custom query. custom query is also more flexible in general.
There was a problem hiding this comment.
Currently the chart does not support MySQL anyway. Agree in principle though
There was a problem hiding this comment.
the chart does not support mysql when you run the metastore on k8s along with airflow. however, wouldn't it work with mysql if you had an external metastore e.g. rds (which is what is recommended anyway)?
i know postgres is the better choice for airflow and much more widely embraced but some folks might use other dbs and off top of my head i think it would probably work since you just need to specify conn uri and disable postgres and pgbouncer...
kaxil
left a comment
kaxil
left a comment
There was a problem hiding this comment.
Can you rebase your PR on latest Master please and tag me again so we can merge it
FloChehab
force-pushed
the
support-paretrized-nb-tasks-per-worker-with-keda
branch
from
434d576 to
d2e348a
Compare
* Rely on the config.celery.worker_concurrency value to determine the number of task a keda worker can take (vs the previous 16 that was hardcoded in the query). * Updated documentation accordingly
FloChehab
force-pushed
the
support-paretrized-nb-tasks-per-worker-with-keda
branch
from
d2e348a to
dcf8ebe
Compare
Hello @kaxil, we should be good to merge for this PR. |
|
Awesome, thank you @FloChehab and @dstandish |
FloChehab
deleted the
support-paretrized-nb-tasks-per-worker-with-keda
branch
3 participants
Hello,
This PR adds a small feature to the chart:
to determine the number of task a keda worker can take
(vs the previous 16 that was hardcoded in the query).
The only impact is that with this PR the default concurrency of KEDA goes down from 16 to 8 (I thought that would be better than to set the concurrency by default from 8 to 16 for all CeleryExecutor deployments).
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.