Add cluster-wide config to override per-pool include_deferred filed - #71220
Add cluster-wide config to override per-pool include_deferred filed#71220msumit wants to merge 3 commits into
Conversation
Introduces a new configuration option. When set to or , it fixes the effective value for every pool (including pre-existing pools) and prevents users from setting a conflicting per-pool value via the UI, API, or CLI. The default remains unset, preserving the existing per-pool behavior. Updates the pool model, local/FastAPI API serializers, UI config endpoint and pool form, CLI pool commands, default pool creation, and adds corresponding tests.
boring-cyborg
Bot
added
area:API
|
cc @ramitkataria is working on a proposal to enchance the pool mechanisem. Ramit when will the AIP published? We need to make sure pool related features are alligned with the plan |
| Note that if tasks are not given a pool, they are assigned to a default pool ``default_pool``, which is | ||
| initialized with 128 slots and can be modified through the UI or CLI (but cannot be removed). | ||
|
|
||
| Whether deferred tasks occupy pool slots is normally decided per pool via its ``include_deferred`` flag. |
There was a problem hiding this comment.
[Discussion] Instead of this should the behavior be: Deployment manager is allowed to set the default value (True/False) for all the pools created under a team or globally and let individual pool owners override this ?
| return str(self.pool) | ||
|
|
||
| @staticmethod | ||
| def get_include_deferred_override() -> bool | None: |
There was a problem hiding this comment.
What happens if the deployment manager unsets this value ? Pool table will have include_deffered=True (which was populated due to global setting).
Now if admin makes it false, the original config set by the end user will not be honored
| return None | ||
|
|
||
| @property | ||
| def effective_include_deferred(self) -> bool: |
There was a problem hiding this comment.
Should we use this only for scheduling decisions ? Let the original value get stored in DB (whatever the end user provided) and take AND of both the values when making a task scheduling decision.
|
|
||
| include_deferred_override = Pool.get_include_deferred_override() | ||
| if include_deferred_override is not None: | ||
| include_deferred = include_deferred_override |
There was a problem hiding this comment.
This will permanently override the global config to pool configs right, making the rollback not possible ?
| name="include_deferred" | ||
| render={({ field }) => ( | ||
| <Field.Root mb={4} mt={4}> | ||
| <Field.Root disabled={includeDeferredOverride !== undefined} mb={4} mt={4}> |
There was a problem hiding this comment.
If the global config is set, should we just skip showing the option to users to configure whether to include deferred slots or not
3 participants
Value prop: As a centralised platform team, we don't want end users to give control of this field, especially when they are totally unaware of what
deferredreally means here. For them, pool slots are used to control the load on the downstream systems like Spark and Presto, and it doesn't matter whether that task is deferred or not. So with a cluster level config we'll take control of the behaviour.Changes: Introduced a new configuration option
core.pool_include_deferred. When set toTrueorFalse, it fixes the effective value for every pool (including pre-existing pools) and prevents users from setting a conflicting per-pool value via the UI, API, or CLI.The default remains unset (empty string
""), preserving the existing per-pool behavior.Updated the pool model, local/FastAPI API serializers, UI config endpoint and pool form, CLI pool commands, default pool creation, and added corresponding tests.
TrueFalseWas generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.