Page MenuHomePhabricator

Refactor SQL-based event log storages to use SQLAlchemy
AbandonedPublic

Authored by max on Nov 20 2019, 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 OK
Unit
No Unit Test Coverage

Event Timeline

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

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

alangenfeld requested changes to this revision.Nov 20 2019, 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

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

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.Nov 20 2019, 5:32 PM
max updated this revision to Diff 6781.Nov 22 2019, 1:12 AM

Sketch

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

Rebase

alangenfeld requested changes to this revision.Nov 22 2019, 4:49 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/storage/event_log/event_log.py
26 ↗(On Diff #6789)

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 ↗(On Diff #6789)

is this actually useful user facing information?

141–291 ↗(On Diff #6789)

move SqlEventLogStorage to own file?

python_modules/dagster/dagster/core/storage/event_log/sqlite/sqlite_event_log.py
154–176 ↗(On Diff #6789)

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 ↗(On Diff #6789)

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.Nov 22 2019, 4:49 PM
max abandoned this revision.Nov 23 2019, 1:00 AM

Closing this to deal with my git woes.