Andrushika commented on PR #66854:
URL: https://github.com/apache/airflow/pull/66854#issuecomment-5122174089
Hi @jason810496 @hkc-8010, thanks for the work here. After looking again, I
think the OOM might be fixable with a much smaller change, and this PR may
already contain it.
The bottleneck is this line in `register_asset_change()`:
```python
asset_alias_models: Iterable[AssetAliasModel] = session.scalars(
select(AssetAliasModel)
.where(AssetAliasModel.name.in_(source_alias_names))
.options(
joinedload(AssetAliasModel.scheduled_dags).joinedload(DagScheduleAssetAliasReference.dag)
)
).unique()
for asset_alias_model in asset_alias_models:
asset_alias_model.asset_events.append(asset_event)
session.add(asset_alias_model)
```
`asset_events` is a lazy relationship. To append one event, SQLAlchemy first
loads the alias's whole existing event history into memory. We never read those
rows, we only want to add one link. The direct insert already in this PR's
`manager.py` skips that load.
I benchmarked `register_asset_change` with N prior events on the alias:
| N | append (lazy load) | direct insert |
|---|---|---|
| 0 | 1.6 ms | 1.2 ms |
| 1,000 | 5.2 ms | 1.2 ms |
| 10,000 | 107 ms | 1.2 ms |
| 50,000 | 470 ms | 1.4 ms |
After that, the function only takes ~1 ms, regardless of the history size.
If that holds, I think we might not need to outbox the pattern or queue
anymore, since the lock detention time becomes really short.
Would landing just `manager.py` first and keeping `asset_event_queue` as a
separate discussion make sense?
Happy to share the benchmarking script.
--
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]
