Page MenuHomePhabricator

Raise a migration required exception when you load a pre-0.10.0 schedule storage
AbandonedPublic

Authored by dgibson on Tue, Jan 12, 4:54 PM.

Details

Summary

Normally we rely on schema errors for this, but in 0.10.0 there is no schema error (but we still need to trigger a migration). Add some hooks for checking for an old alembic revision in the schedule storage and prompting for an update

Test Plan

BK

Diff Detail

Repository
R1 dagster
Branch
addschedulercolumn6
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Tue, Jan 12, 5:10 PM
Harbormaster failed remote builds in B24154: Diff 29374!

rebase with some fixes to make it so that we stamp the revision when a postgres DB is created from scratch (like we do with sqlite) - which is tricky because we haven't historically been doing that, so it's hard to tell between an old DB (which should not be stamped) and a brand new DB (which should be stamped)

I'm not sure we actually want to do this just so that people won't have hanging cron jobs after 0.10.0 - but putting it out there for discussion.

I do think it would be good to start stamping postgres alembic revisions, just so we have the flexibility to know for sure what revision a given user is on

I do think it would be good to start stamping postgres alembic revisions, just so we have the flexibility to know for sure what revision a given user is on

ya since we have a forced migration in this release, now would be a good time to try to correct this. I would argue that calling create_all all the time is also a weird artifact.

What if we only call create_all and stamp when the primary table is missing (event_logs, runs, schedules) otherwise just proceed, and then let any missing tables turn in to migration required errors?

in theory sqlalchemy has us covered if we call create_all? That checks if tables exists before creating them, so I don't see the problem of that part in practice

just check the main tables when deciding whether to stamp

prha requested changes to this revision.Wed, Jan 13, 3:28 AM
prha added inline comments.
python_modules/dagster/dagster/core/storage/schedules/sqlite/sqlite_schedule_storage.py
79

This seems a little fragile. What if we have an enumeration of known past revisions and we update if it's in that set?

python_modules/libraries/dagster-postgres/dagster_postgres/schedule_storage/schedule_storage.py
108

same here... it gets extra gnarly to support using the same db for all three storages (and thus sharing an alembic version table), or using a different db

This revision now requires changes to proceed.Wed, Jan 13, 3:28 AM
python_modules/libraries/dagster-postgres/dagster_postgres/schedule_storage/schedule_storage.py
108

I guess the same versus different db doesn't matter so much, just that this revision will change when we add another migration to any of the storages.