Asquator commented on code in PR #53492:
URL: https://github.com/apache/airflow/pull/53492#discussion_r2250146754
##########
airflow-core/src/airflow/models/taskinstance.py:
##########
@@ -554,6 +559,14 @@ class TaskInstance(Base, LoggingMixin):
innerjoin=True,
viewonly=True,
)
+ pool_model: Pool = relationship(
+ "Pool",
+ primaryjoin="TaskInstance.pool == Pool.pool",
+ foreign_keys=pool,
+ uselist=False,
+ innerjoin=True,
Review Comment:
> At the moment there is no constraint on the relation between pool and TI.
Would it mean the scheduler will not "see" the TIs when a pool is referenced
that an admin has deleted? Do we need to add a constraint to pool table as well?
The only reason I added the relationship is easier join syntax in the query.
I'm not sure about the correct alchemy construct for this case, we should
probably remove the `foreign_keys` argument?
Regarding "ghost" pools - it's actually a deeper concern and I'm glad you
raised it. If a task references a pool that does not exist, you usually get a
corresponding message, but only when the task is "accidentally" pulled by the
scheduler. Let's discuss the possible solutions:
1. Don't show any message if a task references a pool that does not exist by
not pulling them at all, means non-indicative task banning (bad, current PR
state)
2. . Do an outer join, and for tasks with no corresponding pool emit the
message - makes the scheduler optimistic as to inconsistency handling, with
many inconsistent tasks we get more slots "flooded" with pool-ghosted tasks
(bad, current main state)
3. Show the message on dag parsing (why would you want to create a DAG with
such tasks?) (better, can it go wrong?)
4. Handle pools in a smarter way than it's done now - don't allow deleting
pools when there are tasks associated with it, but allow to turn it off (mainly
for disaster handling). It's difficult to implement because there is no table
for tasks.
We have different options and we have to choose one to merge this change
ASAP, avoid breaking the current behavior and leave space for introducing new
changes in subsequent PRs. I'll leave this open
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
