-
Notifications
You must be signed in to change notification settings - Fork 17.7k
Add cluster-wide config to override per-pool include_deferred filed #71220
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,26 @@ class Pool(Base): | |
| def __repr__(self): | ||
| return str(self.pool) | ||
|
|
||
| @staticmethod | ||
| def get_include_deferred_override() -> bool | None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the deployment manager unsets this value ? Pool table will have include_deffered=True (which was populated due to global setting). |
||
| """ | ||
| Get the cluster-wide ``include_deferred`` value fixed via config, if any. | ||
|
|
||
| When ``[core] pool_include_deferred`` is set, its value applies to every pool and takes | ||
| precedence over the per-pool ``include_deferred`` column. Returns None when unset. | ||
| """ | ||
| from airflow.configuration import conf | ||
|
|
||
| if conf.get("core", "pool_include_deferred", fallback=""): | ||
| return conf.getboolean("core", "pool_include_deferred") | ||
| return None | ||
|
|
||
| @property | ||
| def effective_include_deferred(self) -> bool: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| """The ``include_deferred`` value in effect: the cluster-wide config value when fixed, else the pool's own.""" | ||
| override = Pool.get_include_deferred_override() | ||
| return self.include_deferred if override is None else override | ||
|
|
||
| @staticmethod | ||
| @provide_session | ||
| def get_pools(*, session: Session = NEW_SESSION) -> Sequence[Pool]: | ||
|
|
@@ -143,6 +163,10 @@ def create_or_update_pool( | |
| "team_name cannot be set when multi_team mode is disabled. Please contact your administrator." | ||
| ) | ||
|
|
||
| include_deferred_override = Pool.get_include_deferred_override() | ||
| if include_deferred_override is not None: | ||
| include_deferred = include_deferred_override | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will permanently override the global config to pool configs right, making the rollback not possible ? |
||
|
|
||
| pool = session.scalar(select(Pool).filter_by(pool=name)) | ||
| if pool is None: | ||
| pool = Pool( | ||
|
|
@@ -197,6 +221,7 @@ def slots_stats( | |
|
|
||
| pools: dict[str, PoolStats] = {} | ||
| pool_includes_deferred: dict[str, bool] = {} | ||
| include_deferred_override = Pool.get_include_deferred_override() | ||
|
|
||
| # The below type annotation is acceptable on SQLA2.1, but not on 2.0 | ||
| query: Select[str, int, bool] = select(Pool.pool, Pool.slots, Pool.include_deferred) # type: ignore[type-arg] | ||
|
|
@@ -210,7 +235,9 @@ def slots_stats( | |
| pools[pool_name] = PoolStats( | ||
| total=total_slots, running=0, queued=0, open=0, deferred=0, scheduled=0 | ||
| ) | ||
| pool_includes_deferred[pool_name] = include_deferred | ||
| pool_includes_deferred[pool_name] = ( | ||
| include_deferred if include_deferred_override is None else include_deferred_override | ||
| ) | ||
|
|
||
| allowed_execution_states = EXECUTION_STATES | { | ||
| TaskInstanceState.DEFERRED, | ||
|
|
@@ -287,7 +314,7 @@ def occupied_slots(self, *, session: Session = NEW_SESSION) -> int: | |
| ) | ||
|
|
||
| def get_occupied_states(self): | ||
| if self.include_deferred: | ||
| if self.effective_include_deferred: | ||
| return EXECUTION_STATES | { | ||
| TaskInstanceState.DEFERRED, | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,9 +56,15 @@ const PoolForm = ({ error, initialPool, isPending, manageMutate, setError }: Poo | |
| mode: "onChange", | ||
| }); | ||
| const multiTeamEnabled = Boolean(useConfig("multi_team")); | ||
| const includeDeferredConfig = useConfig("pool_include_deferred"); | ||
| // A boolean means include_deferred is fixed cluster-wide and cannot be chosen per pool | ||
| const includeDeferredOverride = | ||
| typeof includeDeferredConfig === "boolean" ? includeDeferredConfig : undefined; | ||
|
|
||
| const onSubmit = (data: PoolBody) => { | ||
| manageMutate(data); | ||
| manageMutate( | ||
| includeDeferredOverride === undefined ? data : { ...data, include_deferred: includeDeferredOverride }, | ||
| ); | ||
| }; | ||
|
|
||
| const handleReset = () => { | ||
|
|
@@ -141,11 +147,22 @@ const PoolForm = ({ error, initialPool, isPending, manageMutate, setError }: Poo | |
| control={control} | ||
| name="include_deferred" | ||
| render={({ field }) => ( | ||
| <Field.Root mb={4} mt={4}> | ||
| <Field.Root disabled={includeDeferredOverride !== undefined} mb={4} mt={4}> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the global config is set, should we just skip showing the option to users to configure whether to include deferred slots or not |
||
| <Field.Label fontSize="md">{translate("pools.form.includeDeferred")}</Field.Label> | ||
| <Checkbox checked={field.value} onChange={field.onChange}> | ||
| <Checkbox | ||
| checked={includeDeferredOverride ?? field.value} | ||
| disabled={includeDeferredOverride !== undefined} | ||
| onChange={field.onChange} | ||
| > | ||
| {translate("pools.form.checkbox")} | ||
| </Checkbox> | ||
| {includeDeferredOverride === undefined ? undefined : ( | ||
| <Field.HelperText> | ||
| {translate("pools.form.includeDeferredFixedHelperText", { | ||
| value: includeDeferredOverride ? "True" : "False", | ||
| })} | ||
| </Field.HelperText> | ||
| )} | ||
| </Field.Root> | ||
| )} | ||
| /> | ||
|
|
||
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.
[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 ?