-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
[AIRFLOW-4591] Make default_pool a real pool #5349
Conversation
d8608e1
to
a0b6cc6
Compare
Codecov Report
@@ Coverage Diff @@
## master #5349 +/- ##
==========================================
- Coverage 78.94% 78.93% -0.01%
==========================================
Files 480 480
Lines 30153 30160 +7
==========================================
+ Hits 23803 23807 +4
- Misses 6350 6353 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #5349 +/- ##
==========================================
- Coverage 79.12% 79.05% -0.07%
==========================================
Files 488 485 -3
Lines 30550 30356 -194
==========================================
- Hits 24172 23999 -173
+ Misses 6378 6357 -21
Continue to review full report at Codecov.
|
88cdd97
to
8b816de
Compare
@ashb thanks for the review. This PR is ready for another look. |
8b816de
to
76bbb52
Compare
Sorry - taking another look now. Was rather swamped after vacation and forgot to come back to this. |
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.
Happy with the code now, but would like others to take a look and see if they agree with the feature.
Is it worth adding a migration to the TaskInstance table to make Pool a non-nullable column too?
7b56d98
to
de5cf1b
Compare
I think it doesn't hurt to restrict the field to nonnull |
d212280
to
78da1ad
Compare
airflow/migrations/versions/6e96a59344a4_make_taskinstance_pool_not_nullable.py
Show resolved
Hide resolved
I will take a look at the pr next Monday if the pr is still open. |
9330537
to
f6a31c7
Compare
airflow/migrations/versions/6e96a59344a4_make_taskinstance_pool_not_nullable.py
Outdated
Show resolved
Hide resolved
4c63ed2
to
d05bf8b
Compare
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 this is good now. Will give others a day or two to chime in.
We could possibly pull this in to 1.10.4 too - it doesn't change any behaviour in a serious way I think?
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.
LGTM. Good work @milton0825
@ashb This PR only changes how user configure the default pool. |
@milton0825 could you rebase with master to resolve the conflict? Will merge it once the CI is clear. |
`non_pooled_task_slot_count` and `non_pooled_backfill_task_slot_count` are removed in favor of a real pool, e.g. `default_pool`. By default tasks are running in `default_pool`. `default_pool` is initialized with 128 slots and user can change the number of slots through UI/CLI. `default_pool` cannot be removed.
d05bf8b
to
4f4f24b
Compare
@feng-tao I have resolved the conflicts and now CI is green. |
ship |
A fair few conflicts - so this might not make it in to 1.10.4 (I haven't looked how difficult they are to resolve, just that there ~10 files and I don't want to deal with that last thing on a Friday afternoon) Resolved them - they weren't too bad once I looked at it. This will go in to 1.10.4 (as will the pool stats PR) |
`non_pooled_task_slot_count` and `non_pooled_backfill_task_slot_count` are removed in favor of a real pool, e.g. `default_pool`. By default tasks are running in `default_pool`. `default_pool` is initialized with 128 slots and user can change the number of slots through UI/CLI. `default_pool` cannot be removed. (cherry picked from commit 2c99ec6)
`non_pooled_task_slot_count` and `non_pooled_backfill_task_slot_count` are removed in favor of a real pool, e.g. `default_pool`. By default tasks are running in `default_pool`. `default_pool` is initialized with 128 slots and user can change the number of slots through UI/CLI. `default_pool` cannot be removed. (cherry picked from commit 2c99ec6)
`non_pooled_task_slot_count` and `non_pooled_backfill_task_slot_count` are removed in favor of a real pool, e.g. `default_pool`. By default tasks are running in `default_pool`. `default_pool` is initialized with 128 slots and user can change the number of slots through UI/CLI. `default_pool` cannot be removed.
`non_pooled_task_slot_count` and `non_pooled_backfill_task_slot_count` are removed in favor of a real pool, e.g. `default_pool`. By default tasks are running in `default_pool`. `default_pool` is initialized with 128 slots and user can change the number of slots through UI/CLI. `default_pool` cannot be removed.
Thanks @milton0825 for working on this. 6 days back I discovered the need and now I see this has already been addressed 👍 |
`non_pooled_task_slot_count` and `non_pooled_backfill_task_slot_count` are removed in favor of a real pool, e.g. `default_pool`. By default tasks are running in `default_pool`. `default_pool` is initialized with 128 slots and user can change the number of slots through UI/CLI. `default_pool` cannot be removed. (cherry picked from commit 2c99ec6)
Make sure you have checked all steps below.
Jira
Description
tl;dl
non_pooled_task_slot_count
andnon_pooled_backfill_task_slot_count
are removed in favor of a real pool, e.g.
default_pool
.Currently the number of running tasks that do not have a pool specified will be limited by
non_pooled_task_slot_count
ornon_pooled_backfill_task_slot_count
depends on the whether the task is triggered by scheduler or backfill. Conceptually it is a pool with slots specified through configuration but with a bit different implementation. This PR propose to make this pool a real pool, e.g. using the common implementation of pool so that the implementation can be simplified.By default tasks are running in
default_pool
.default_pool
is initialized with 128 slots and user can change thenumber of slots through UI/CLI.
default_pool
cannot be removed.Tests
Commits
Documentation
Code Quality
flake8