Skip to content

Commit 1fc1356

Browse files
prhaDagster Devtools
authored andcommitted
Group event log index writes in transactions (#25660)
## Summary & Motivation As we prepare to write more derived tables on event log writes, we should group the writes into transactions. ## Test Plan BK Internal-RevId: 1d689b77cfa6a046a573c4f0939006bf1886e5e4
1 parent 3de9ad1 commit 1fc1356

4 files changed

Lines changed: 90 additions & 62 deletions

File tree

python_modules/dagster/dagster/_core/storage/event_log/sql_event_log.py

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,26 @@ def _event_to_row(self, event: EventLogEntry) -> dict[str, Any]:
229229

230230
def has_asset_key_col(self, column_name: str) -> bool:
231231
with self.index_connection() as conn:
232-
column_names = [x.get("name") for x in db.inspect(conn).get_columns(AssetKeyTable.name)]
233-
return column_name in column_names
232+
return self._has_asset_key_col_on_connection(conn, column_name)
233+
234+
def _has_asset_key_col_on_connection(self, conn: Connection, column_name: str) -> bool:
235+
column_names = [x.get("name") for x in db.inspect(conn).get_columns(AssetKeyTable.name)]
236+
return column_name in column_names
234237

235238
def has_asset_key_index_cols(self) -> bool:
236239
return self.has_asset_key_col("last_materialization_timestamp")
237240

241+
def _has_asset_key_index_cols_on_connection(self, conn: Connection) -> bool:
242+
return self._has_asset_key_col_on_connection(conn, "last_materialization_timestamp")
243+
238244
def store_asset_event(self, event: EventLogEntry, event_id: int):
239245
check.inst_param(event, "event", EventLogEntry)
246+
check.int_param(event_id, "event_id")
247+
248+
with self.index_transaction() as conn:
249+
self._store_asset_event(conn, event, event_id)
240250

251+
def _store_asset_event(self, conn: Connection, event: EventLogEntry, event_id: int) -> None:
241252
if not (event.dagster_event and event.dagster_event.asset_key):
242253
return
243254

@@ -253,7 +264,9 @@ def store_asset_event(self, event: EventLogEntry, event_id: int):
253264
#
254265
# https://github.com/dagster-io/dagster/issues/3945
255266

256-
values = self._get_asset_entry_values(event, event_id, self.has_asset_key_index_cols())
267+
values = self._get_asset_entry_values(
268+
event, event_id, self._has_asset_key_index_cols_on_connection(conn)
269+
)
257270
if not values:
258271
return
259272
insert_statement = AssetKeyTable.insert().values(
@@ -267,11 +280,10 @@ def store_asset_event(self, event: EventLogEntry, event_id: int):
267280
)
268281
)
269282

270-
with self.index_connection() as conn:
271-
try:
272-
conn.execute(insert_statement)
273-
except db_exc.IntegrityError:
274-
conn.execute(update_statement)
283+
try:
284+
conn.execute(insert_statement)
285+
except db_exc.IntegrityError:
286+
conn.execute(update_statement)
275287

276288
def _get_asset_entry_values(
277289
self, event: EventLogEntry, event_id: int, has_asset_key_index_cols: bool
@@ -337,6 +349,12 @@ def store_asset_event_tags(
337349
check.sequence_param(events, "events", EventLogEntry)
338350
check.sequence_param(event_ids, "event_ids", int)
339351

352+
with self.index_transaction() as conn:
353+
self._store_asset_event_tags(conn, events, event_ids)
354+
355+
def _store_asset_event_tags(
356+
self, conn: Connection, events: Sequence[EventLogEntry], event_ids: Sequence[int]
357+
) -> None:
340358
all_values = [
341359
dict(
342360
event_id=event_id,
@@ -353,8 +371,7 @@ def store_asset_event_tags(
353371
# migration to create the table. On read, we will throw an error if the table does not
354372
# exist.
355373
if len(all_values) > 0 and self.has_table(AssetEventTagsTable.name):
356-
with self.index_connection() as conn:
357-
conn.execute(AssetEventTagsTable.insert(), all_values)
374+
conn.execute(AssetEventTagsTable.insert(), all_values)
358375

359376
def _tags_for_asset_event(self, event: EventLogEntry) -> Mapping[str, str]:
360377
tags = {}
@@ -388,14 +405,14 @@ def store_event(self, event: EventLogEntry) -> None:
388405
and event.dagster_event_type in ASSET_EVENTS
389406
and event.dagster_event.asset_key # type: ignore
390407
):
391-
self.store_asset_event(event, event_id)
392-
393408
if event_id is None:
394409
raise DagsterInvariantViolationError(
395410
"Cannot store asset event tags for null event id."
396411
)
397412

398-
self.store_asset_event_tags([event], [event_id])
413+
with self.index_transaction() as conn:
414+
self._store_asset_event_tags(conn, [event], [event_id])
415+
self._store_asset_event(conn, event, event_id)
399416

400417
if event.is_dagster_event and event.dagster_event_type in ASSET_CHECK_EVENTS:
401418
self.store_asset_check_event(event, event_id)

python_modules/dagster/dagster/_core/storage/event_log/sqlite/sqlite_event_log.py

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -275,20 +275,17 @@ def store_event(self, event: EventLogEntry) -> None:
275275
event_id = None
276276

277277
# mirror the event in the cross-run index database
278-
with (
279-
self.index_connection() as conn,
280-
db_result(conn, insert_event_statement) as result,
281-
):
282-
event_id = result.inserted_primary_key[0]
283-
284-
self.store_asset_event(event, event_id)
278+
with self.index_transaction() as conn:
279+
with db_result(conn, insert_event_statement) as result:
280+
event_id = result.inserted_primary_key[0]
285281

286-
if event_id is None:
287-
raise DagsterInvariantViolationError(
288-
"Cannot store asset event tags for null event id."
289-
)
282+
if event_id is None:
283+
raise DagsterInvariantViolationError(
284+
"Cannot store asset event tags for null event id."
285+
)
290286

291-
self.store_asset_event_tags([event], [event_id])
287+
self._store_asset_event_tags(conn, [event], [event_id])
288+
self._store_asset_event(conn, event, event_id)
292289

293290
if event.is_dagster_event and event.dagster_event_type in ASSET_CHECK_EVENTS:
294291
# mirror the event in the cross-run index database

python_modules/libraries/dagster-mysql/dagster_mysql/event_log/event_log.py

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -149,33 +149,42 @@ def get_server_version(self) -> str | None:
149149
return cast("str", row[0])
150150

151151
def store_asset_event(self, event: EventLogEntry, event_id: int) -> None:
152+
check.inst_param(event, "event", EventLogEntry)
153+
check.int_param(event_id, "event_id")
154+
155+
with self.index_transaction() as conn:
156+
self._store_asset_event(conn, event, event_id)
157+
158+
def _store_asset_event(self, conn: Connection, event: EventLogEntry, event_id: int) -> None:
152159
# last_materialization_timestamp is updated upon observation, materialization, materialization_planned
153160
# See SqlEventLogStorage.store_asset_event method for more details
154161

162+
if not (event.dagster_event and event.dagster_event.asset_key):
163+
return
164+
155165
values = self._get_asset_entry_values(
156166
event, event_id, self.has_secondary_index(ASSET_KEY_INDEX_COLS)
157167
)
158-
with self.index_connection() as conn:
159-
if values:
168+
if values:
169+
conn.execute(
170+
db_dialects.mysql.insert(AssetKeyTable)
171+
.values(
172+
asset_key=event.dagster_event.asset_key.to_string(),
173+
**values,
174+
)
175+
.on_duplicate_key_update(
176+
**values,
177+
)
178+
)
179+
else:
180+
try:
160181
conn.execute(
161-
db_dialects.mysql.insert(AssetKeyTable)
162-
.values(
163-
asset_key=event.dagster_event.asset_key.to_string(), # type: ignore # (possible none)
164-
**values,
165-
)
166-
.on_duplicate_key_update(
167-
**values,
182+
db_dialects.mysql.insert(AssetKeyTable).values(
183+
asset_key=event.dagster_event.asset_key.to_string(),
168184
)
169185
)
170-
else:
171-
try:
172-
conn.execute(
173-
db_dialects.mysql.insert(AssetKeyTable).values(
174-
asset_key=event.dagster_event.asset_key.to_string(), # type: ignore # (possible none)
175-
)
176-
)
177-
except db_exc.IntegrityError:
178-
pass
186+
except db_exc.IntegrityError:
187+
pass
179188

180189
def _connect(self) -> ContextManager[Connection]:
181190
return create_mysql_connection(self._engine, __file__, "event log")

python_modules/libraries/dagster-postgres/dagster_postgres/event_log/event_log.py

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -209,14 +209,14 @@ def store_event(self, event: EventLogEntry) -> None:
209209
and event.dagster_event_type in ASSET_EVENTS
210210
and event.dagster_event.asset_key # type: ignore
211211
):
212-
self.store_asset_event(event, event_id)
213-
214212
if event_id is None:
215213
raise DagsterInvariantViolationError(
216214
"Cannot store asset event tags for null event id."
217215
)
218216

219-
self.store_asset_event_tags([event], [event_id])
217+
with self.index_transaction() as conn:
218+
self._store_asset_event_tags(conn, [event], [event_id])
219+
self._store_asset_event(conn, event, event_id)
220220

221221
if event.is_dagster_event and event.dagster_event_type in ASSET_CHECK_EVENTS:
222222
self.store_asset_check_event(event, event_id)
@@ -250,20 +250,26 @@ def store_event_batch(self, events: Sequence[EventLogEntry]) -> None:
250250
)
251251
event_ids = [cast("int", row[0]) for row in result.fetchall()]
252252

253-
# We only update the asset table with the last event
254-
self.store_asset_event(events[-1], event_ids[-1])
255-
256253
if any(event_id is None for event_id in event_ids):
257254
raise DagsterInvariantViolationError(
258255
"Cannot store asset event tags for null event id."
259256
)
260257

261-
self.store_asset_event_tags(events, event_ids)
258+
with self.index_transaction() as conn:
259+
self._store_asset_event_tags(conn, events, event_ids)
260+
# We only update the asset table with the last event.
261+
self._store_asset_event(conn, events[-1], event_ids[-1])
262262
else:
263263
return super().store_event_batch(events)
264264

265265
def store_asset_event(self, event: EventLogEntry, event_id: int) -> None:
266266
check.inst_param(event, "event", EventLogEntry)
267+
check.int_param(event_id, "event_id")
268+
269+
with self.index_transaction() as conn:
270+
self._store_asset_event(conn, event, event_id)
271+
272+
def _store_asset_event(self, conn: Connection, event: EventLogEntry, event_id: int) -> None:
267273
if not (event.dagster_event and event.dagster_event.asset_key):
268274
return
269275

@@ -300,19 +306,18 @@ def store_asset_event(self, event: EventLogEntry, event_id: int) -> None:
300306
values = self._get_asset_entry_values(
301307
event, event_id, self.has_secondary_index(ASSET_KEY_INDEX_COLS)
302308
)
303-
with self.index_connection() as conn:
304-
query = db_dialects.postgresql.insert(AssetKeyTable).values(
305-
asset_key=event.dagster_event.asset_key.to_string(),
306-
**values,
309+
query = db_dialects.postgresql.insert(AssetKeyTable).values(
310+
asset_key=event.dagster_event.asset_key.to_string(),
311+
**values,
312+
)
313+
if values:
314+
query = query.on_conflict_do_update(
315+
index_elements=[AssetKeyTable.c.asset_key],
316+
set_=dict(**values),
307317
)
308-
if values:
309-
query = query.on_conflict_do_update(
310-
index_elements=[AssetKeyTable.c.asset_key],
311-
set_=dict(**values),
312-
)
313-
else:
314-
query = query.on_conflict_do_nothing()
315-
conn.execute(query)
318+
else:
319+
query = query.on_conflict_do_nothing()
320+
conn.execute(query)
316321

317322
def add_dynamic_partitions(
318323
self, partitions_def_name: str, partition_keys: Sequence[str]

0 commit comments

Comments
 (0)