Page MenuHomePhabricator

Refactor SQL-based event log storages to use SQLAlchemy
AbandonedPublic

Authored by max on Wed, Nov 20, 3:20 AM.

Details

Summary

This resolves https://github.com/dagster-io/dagster/issues/1798 as well as performance issues stemming from calls to the default implementation of get_stats_for_run.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Branch
sqlalchemy-postgres
Lint
Lint ErrorsExcuse: this is ok
Unit
No Unit Test Coverage

Event Timeline

max created this revision.Wed, Nov 20, 3:20 AM
max added a comment.Wed, Nov 20, 3:25 AM

@alangenfeld this breaks backwards compat, I can adjust it to use an identical schema

alangenfeld requested changes to this revision.Wed, Nov 20, 5:32 PM

ya lets do the back compat dance - doesnt seem to be a lot of value to be reaped from breaking changes at this juncture

python_modules/dagster/dagster/core/storage/event_log.py
235–236 ↗(On Diff #6734)

I think it might be worth shuffling classes around since I think this or related assumptions could easily be missed

SQLEventLogStorage -> SingleTableEventLogStorage -> PostgresEventLogStorage

SQLEventLogStorage -> SqliteEventLogStorage

python_modules/dagster/dagster/core/storage/sqlite_event_log.py
56 ↗(On Diff #6734)

worth considering removing WatchableEventLogStorage and migrating the functions on the base class and implement the few missing functions on the in-memory store

This revision now requires changes to proceed.Wed, Nov 20, 5:32 PM
max updated this revision to Diff 6781.Fri, Nov 22, 1:12 AM

Sketch

max updated this revision to Diff 6789.Fri, Nov 22, 3:35 AM

Rebase

alangenfeld requested changes to this revision.Fri, Nov 22, 4:49 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/storage/event_log/event_log.py
26

I think all our other errors are Dagster prefixed - worth staying consistent. We could get away with a single DagsterInstanceMigrationRequired error for all these types of issues

29–30

is this actually useful user facing information?

141–291

move SqlEventLogStorage to own file?

python_modules/dagster/dagster/core/storage/event_log/sqlite/sqlite_event_log.py
154–176

this broad except / raise_upgrade_error setup worries me a bit. If one of the lines before raise_upgrade_error = True has a typo/bad arg/whatever we will silently eat the error forever right?

python_modules/dagster/dagster_tests/compat_tests/test_back_compat.py
12–40

should we change this test or add a different case that copies over to a temp dir, runs the migration, and ensures it works

This revision now requires changes to proceed.Fri, Nov 22, 4:49 PM
max abandoned this revision.Sat, Nov 23, 1:00 AM

Closing this to deal with my git woes.