-
Notifications
You must be signed in to change notification settings - Fork 17.7k
Reduce scheduler memory for asset-triggered Dag runs #69849
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 |
|---|---|---|
|
|
@@ -43,7 +43,9 @@ | |
| delete, | ||
| exists, | ||
| func, | ||
| insert, | ||
| inspect, | ||
| literal, | ||
| or_, | ||
| select, | ||
| text, | ||
|
|
@@ -138,6 +140,7 @@ | |
| from sqlalchemy.engine import CursorResult | ||
| from sqlalchemy.orm import Session | ||
| from sqlalchemy.orm.interfaces import LoaderOption | ||
| from sqlalchemy.sql import Select | ||
| from sqlalchemy.sql.elements import ColumnElement | ||
| from sqlalchemy.sql.selectable import Subquery | ||
|
|
||
|
|
@@ -163,6 +166,23 @@ | |
| MAX_PARTITION_DAG_RUNS_PER_LOOP = 500 | ||
|
|
||
|
|
||
| def _associate_asset_events_with_dag_run( | ||
| *, | ||
| dag_run: DagRun, | ||
| asset_event_ids_select: Select[tuple[int]], | ||
| session: Session, | ||
| ) -> None: | ||
| """Associate selected asset events without materializing ORM objects.""" | ||
| selected_event_ids = asset_event_ids_select.subquery() | ||
| session.execute( | ||
| insert(association_table).from_select( | ||
| ["dag_run_id", "event_id"], | ||
| select(literal(dag_run.id), selected_event_ids.c.event_id), | ||
| ) | ||
| ) | ||
| session.expire(dag_run, ["consumed_asset_events"]) | ||
|
|
||
|
|
||
| def _eager_load_dag_run_for_validation() -> tuple[LoaderOption, LoaderOption]: | ||
| """ | ||
| Eager-load DagRun relations required for execution API datamodel validation. | ||
|
|
@@ -2425,13 +2445,15 @@ def _create_dagruns_for_partitioned_asset_dags(self, session: Session) -> set[st | |
| creating_job_id=self.job.id, | ||
| session=session, | ||
| ) | ||
| asset_events = session.scalars( | ||
| select(AssetEvent).where( | ||
| PartitionedAssetKeyLog.asset_partition_dag_run_id == apdr.id, | ||
| PartitionedAssetKeyLog.asset_event_id == AssetEvent.id, | ||
| ) | ||
| asset_event_ids_select = select(AssetEvent.id.label("event_id")).where( | ||
| PartitionedAssetKeyLog.asset_partition_dag_run_id == apdr.id, | ||
| PartitionedAssetKeyLog.asset_event_id == AssetEvent.id, | ||
| ) | ||
| _associate_asset_events_with_dag_run( | ||
| dag_run=dag_run, | ||
| asset_event_ids_select=asset_event_ids_select, | ||
| session=session, | ||
| ) | ||
| dag_run.consumed_asset_events.extend(asset_events) | ||
| session.flush() | ||
| apdr.created_dag_run_id = dag_run.id | ||
| session.flush() | ||
|
|
@@ -2688,26 +2710,33 @@ def _create_dag_runs_asset_triggered( | |
| ) | ||
| ), | ||
| ) | ||
| asset_events = list( | ||
| session.scalars( | ||
| select(AssetEvent) | ||
| .where( | ||
| event_predicate, | ||
| ~( | ||
| select(association_table.c.event_id) | ||
| .join(DagRun, DagRun.id == association_table.c.dag_run_id) | ||
| .where( | ||
| DagRun.dag_id == dag.dag_id, | ||
| association_table.c.event_id == AssetEvent.id, | ||
| ) | ||
| .exists() | ||
| ), | ||
| ) | ||
| .order_by(AssetEvent.timestamp.asc(), AssetEvent.id.asc()) | ||
| selected_asset_events = ( | ||
| select( | ||
| AssetEvent.id.label("event_id"), | ||
| AssetEvent.timestamp.label("timestamp"), | ||
| ) | ||
| .where( | ||
| event_predicate, | ||
| ~( | ||
| select(association_table.c.event_id) | ||
| .join(DagRun, DagRun.id == association_table.c.dag_run_id) | ||
| .where( | ||
| DagRun.dag_id == dag.dag_id, | ||
| association_table.c.event_id == AssetEvent.id, | ||
| ) | ||
| .exists() | ||
| ), | ||
| ) | ||
| .cte() | ||
| ) | ||
| if asset_events: | ||
| triggered_date = timezone.coerce_datetime(max(event.timestamp for event in asset_events)) | ||
| triggered_date, max_selected_event_id = session.execute( | ||
| select( | ||
| func.max(selected_asset_events.c.timestamp), | ||
| func.max(selected_asset_events.c.event_id), | ||
| ) | ||
| ).one() | ||
| if triggered_date is not None and max_selected_event_id is not None: | ||
| triggered_date = timezone.coerce_datetime(triggered_date) | ||
| self.log.debug( | ||
| "Creating asset-triggered DagRun for '%s': %d queued assets, triggered_date=%s", | ||
| dag.dag_id, | ||
|
|
@@ -2733,12 +2762,32 @@ def _create_dag_runs_asset_triggered( | |
| else None | ||
| ) | ||
| stats.incr("asset.triggered_dagruns", tags=prune_dict({"team_name": team_name})) | ||
| dag_run.consumed_asset_events.extend(asset_events) | ||
| _associate_asset_events_with_dag_run( | ||
| dag_run=dag_run, | ||
| asset_event_ids_select=select(selected_asset_events.c.event_id) | ||
| .where( | ||
| selected_asset_events.c.timestamp <= triggered_date, | ||
| selected_asset_events.c.event_id <= max_selected_event_id, | ||
|
Comment on lines
+2769
to
+2770
Member
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 looks like dead weight unless the reader understands the CTE is re-evaluated fresh in this second, independent query and could pick up events inserted after the aggregate read. Add a comment explaining this is a snapshot bound against a race, not a no-op. |
||
| ) | ||
| .order_by( | ||
| selected_asset_events.c.timestamp.asc(), | ||
| selected_asset_events.c.event_id.asc(), | ||
| ), | ||
| session=session, | ||
| ) | ||
| consumed_asset_event_count = ( | ||
| session.scalar( | ||
| select(func.count()) | ||
| .select_from(association_table) | ||
| .where(association_table.c.dag_run_id == dag_run.id) | ||
| ) | ||
| or 0 | ||
| ) | ||
|
Comment on lines
+2778
to
+2785
Member
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. We can probably use |
||
| self.log.info( | ||
| "Created asset-triggered DagRun for '%s': run_id=%s, consumed %d asset events", | ||
| dag.dag_id, | ||
| dag_run.run_id, | ||
| len(asset_events), | ||
| consumed_asset_event_count, | ||
| ) | ||
| else: | ||
| self.log.info( | ||
|
|
||
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.
Worth a comment explaining why manual expiration is needed.