Page MenuHomeElementl

Add CLI command to reset migration state
Changes PlannedPublic

Authored by prha on Jun 9 2021, 9:56 PM.

Details

Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
prha/reset_migration
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 9 2021, 10:16 PM
Harbormaster failed remote builds in B31924: Diff 39325!

update, pick up consolidated sqlite change

prha requested review of this revision.Jun 10 2021, 4:38 AM
rexledesma added inline comments.
python_modules/dagster/dagster/cli/instance.py
73
76

raise a UsageError here?

80

consider adding type=click.Choice(["RESET"]) or something of the like so user input can be constrained

is this safe? are all of our migrations idempotent? I faintly remember doing this manually and was getting errors on various migrations so I had to find the actual version right version to start from

It seemed to work with the specific migration that prompted this (0.10.9 => 0.11.13)

well we should at least test for sqlite and PG reseting various snapshots back to base and then doing an instance migrate.

It just seems odd to me that if create my DBs at 0.10.0 and then do this reset the instance migrate, I will play like the 0.6.0 migration. I guess we can update all the migration code to ensure they do some check before running forward

An option that lets you pass in a custom rev does seem like a good idea too (in the case before, we could have set them to the pre-0.10.0 rev that we expected them to be at)

would be nice to have a mapping alembic version to dagster version

when i had to find it I was using git tag --contains and looking at blame, it sucked

The specific migration revision is a bit tricky, because you could have three different physical dbs for the different storages, all with a different alembic version.

You could also technically be using sqlite for one and postgres for the other, which would have a different set of valid revisions. It'd be way easier if we just had a single storage concept with a single version id.

Not sure how much complexity we want to expose here, and if it's worth going through (the current state is that we manually tell people to drop the appropriate table).

add backcompat tests for resetting all snapshots and reapplying

prha planned changes to this revision.Jun 10 2021, 7:33 PM

Oh, I have an idea of how to do this mapping....

add mapping from dagster version => alembic version on a per-storage basis

This isn't the cleanest solution, because it requires a bunch of manual resolving, but I don't think there's a better way of building up this mapping.

johann added a subscriber: johann.

ill let @dgibson make the final call on this one

impressive spelunking. This seems like an improvement overall, sorry it took so long for me to get back to it

python_modules/dagster/dagster/core/storage/event_log/sqlite/migration.py
1

did you update the quip to reflect this / will a test break if you create a new migration and don't update this?

check out test_alembic_up_to_date in the internal repo for a way you might accomplish that

1

consider naming these differently (runs/schedules/event_logs) the module helps but it seems easy to accidentally swap in the wrong one

This revision is now accepted and ready to land.Jul 1 2021, 1:50 PM
prha planned changes to this revision.Jul 30 2021, 5:38 PM