kaxil commented on code in PR #75404:
URL: https://github.com/apache/airflow/pull/75404#discussion_r4350452507
##########
airflow-core/src/airflow/jobs/scheduler_job_runner.py:
##########
@@ -4217,30 +4217,17 @@ def _update_asset_orphanage(self, *, session: Session =
NEW_SESSION) -> None:
:seealso: :meth:`AssetModelOperation.activate_assets_if_possible`.
"""
- # Group assets into orphaned=True and orphaned=False groups.
- orphaned = (
- (
- func.count(DagScheduleAssetReference.dag_id)
- + func.count(TaskOutletAssetReference.dag_id)
- + func.count(TaskInletAssetReference.dag_id)
- # An asset materialized from an ``AssetAlias`` outlet is
referenced only through
- # the alias association, not the schedule/outlet/inlet tables.
Count it so it is
- # activated (and shown in the UI) instead of being treated as
orphaned. See #58058.
- + func.count(alias_association_table.c.alias_id)
- )
- == 0
- ).label("orphaned")
- asset_reference_query = (
- select(AssetModel)
- .outerjoin(DagScheduleAssetReference)
- .outerjoin(TaskOutletAssetReference)
- .outerjoin(TaskInletAssetReference)
- .outerjoin(alias_association_table,
alias_association_table.c.asset_id == AssetModel.id)
- .group_by(AssetModel.id)
+ referenced = or_(
Review Comment:
Since the description points at the missing `non_fatal`: with it unset, a
`statement_timeout` here raises out of `timers.run()` and `_execute` re-raises
it, so the SchedulerJob exits rather than skipping a cycle. #69487 added
`non_fatal=True` to `_remove_unreferenced_triggers` and
`_reap_stale_connection_tests` for exactly this case but left the orphanage
timer out
([L2026-L2028](https://github.com/apache/airflow/blob/42b120159aafc75fae254c147ad6887c980cc0e9/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L2026-L2028)).
Worth adding it there too, here or in a follow-up? Each pass recomputes
everything from scratch, so skipping one cycle is harmless.
##########
airflow-core/src/airflow/jobs/scheduler_job_runner.py:
##########
@@ -4217,30 +4217,17 @@ def _update_asset_orphanage(self, *, session: Session =
NEW_SESSION) -> None:
:seealso: :meth:`AssetModelOperation.activate_assets_if_possible`.
"""
- # Group assets into orphaned=True and orphaned=False groups.
- orphaned = (
- (
- func.count(DagScheduleAssetReference.dag_id)
- + func.count(TaskOutletAssetReference.dag_id)
- + func.count(TaskInletAssetReference.dag_id)
- # An asset materialized from an ``AssetAlias`` outlet is
referenced only through
- # the alias association, not the schedule/outlet/inlet tables.
Count it so it is
- # activated (and shown in the UI) instead of being treated as
orphaned. See #58058.
- + func.count(alias_association_table.c.alias_id)
- )
- == 0
- ).label("orphaned")
- asset_reference_query = (
- select(AssetModel)
- .outerjoin(DagScheduleAssetReference)
- .outerjoin(TaskOutletAssetReference)
- .outerjoin(TaskInletAssetReference)
- .outerjoin(alias_association_table,
alias_association_table.c.asset_id == AssetModel.id)
- .group_by(AssetModel.id)
+ referenced = or_(
+ exists().where(DagScheduleAssetReference.asset_id ==
AssetModel.id),
+ exists().where(TaskOutletAssetReference.asset_id == AssetModel.id),
+ exists().where(TaskInletAssetReference.asset_id == AssetModel.id),
+ # An asset materialized from an ``AssetAlias`` outlet is
referenced only through
+ # the alias association, not the schedule/outlet/inlet tables.
Count it so it is
+ # activated (and shown in the UI) instead of being treated as
orphaned. See #58058.
+ exists().where(alias_association_table.c.asset_id ==
AssetModel.id),
)
-
- orphan_query = asset_reference_query.having(orphaned).cte()
- activate_query = asset_reference_query.having(~orphaned).cte()
+ orphan_query = select(AssetModel).where(~referenced).cte()
Review Comment:
#75127 changes this same line to `.subquery()` because Vitess/PlanetScale
reject `WITH` inside a `DELETE`. With the `GROUP BY` gone the CTE isn't buying
anything on the orphan side, so switching it to `.subquery()` here (plus the
`CTE` annotation on `_orphan_unreferenced_assets`) would let the two PRs
compose instead of conflicting. Either way one of them will need a rebase onto
the other.
##########
airflow-core/tests/unit/jobs/test_scheduler_job.py:
##########
@@ -10166,6 +10166,37 @@ def
test_asset_orphaning_keeps_alias_materialized_asset_active(self, session):
assert asset in active
assert asset not in orphaned
+ def test_asset_orphaning_asset_referenced_from_every_table(self,
dag_maker, session):
Review Comment:
The name says every table, but `shared` never gets an `asset_alias_asset`
row. I also don't think anything pins the inlet branch: the only inlet-only
asset in the suite is `asset6` in `test_asset_orphaning`, and the asserts after
its first `_update_asset_orphanage` call reuse the lists fetched before the
call (L10117-L10120). So dropping the `TaskInletAssetReference` EXISTS would,
as far as I can trace, leave every orphanage test green. Could this test add an
inlet-only asset and attach an alias to `shared`, so each of the four branches
is pinned on its own?
##########
airflow-core/src/airflow/jobs/scheduler_job_runner.py:
##########
@@ -4217,30 +4217,17 @@ def _update_asset_orphanage(self, *, session: Session =
NEW_SESSION) -> None:
:seealso: :meth:`AssetModelOperation.activate_assets_if_possible`.
"""
- # Group assets into orphaned=True and orphaned=False groups.
- orphaned = (
- (
- func.count(DagScheduleAssetReference.dag_id)
- + func.count(TaskOutletAssetReference.dag_id)
- + func.count(TaskInletAssetReference.dag_id)
- # An asset materialized from an ``AssetAlias`` outlet is
referenced only through
- # the alias association, not the schedule/outlet/inlet tables.
Count it so it is
- # activated (and shown in the UI) instead of being treated as
orphaned. See #58058.
- + func.count(alias_association_table.c.alias_id)
- )
- == 0
- ).label("orphaned")
- asset_reference_query = (
- select(AssetModel)
- .outerjoin(DagScheduleAssetReference)
- .outerjoin(TaskOutletAssetReference)
- .outerjoin(TaskInletAssetReference)
- .outerjoin(alias_association_table,
alias_association_table.c.asset_id == AssetModel.id)
- .group_by(AssetModel.id)
+ referenced = or_(
+ exists().where(DagScheduleAssetReference.asset_id ==
AssetModel.id),
+ exists().where(TaskOutletAssetReference.asset_id == AssetModel.id),
+ exists().where(TaskInletAssetReference.asset_id == AssetModel.id),
+ # An asset materialized from an ``AssetAlias`` outlet is
referenced only through
+ # the alias association, not the schedule/outlet/inlet tables.
Count it so it is
Review Comment:
Nothing is counted any more, so "Count it" reads oddly now. Maybe "Treat it
as referenced so it is activated (and shown in the UI) instead of being
orphaned."
--
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]
