Page MenuHomeElementl

Migrate db.String to db.Text to support MySQL
ClosedPublic

Authored by rexledesma on Jan 4 2021, 10:38 PM.

Details

Summary

To support MySQL, we migrate incompatible column types. For example, it does not support a bare db.String, as it is not fixed length.
Here, we migrate all uses of db.String to db.Text to accommodate for this.

Co-authored-by: Rex Ledesma <rex@elementl.com>

Test Plan

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nate edited the test plan for this revision. (Show Details)

up

prha added inline comments.
python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/versions/0da417ae1b81_change_varchar_to_text.py
27–36

this is an event_log migration, so will probably a no-op.

I'd expect only the event logs migrations here.

python_modules/dagster/dagster/core/storage/schedules/schema.py
4–5

these two tables are deprecated so you probably do not need to migrate

python_modules/libraries/dagster-postgres/dagster_postgres/event_log/alembic/versions/4ea2b1f6f67b_change_varchar_to_text.py
27–36

I'd expect a section here for each storage table that needs to be altered:

if "event_logs" in has_tables:
     # alter event_logs

if "job_ticks" in has_tables:
    # alter job_ticks

...
scripts/check_schemas.py
9

nice

rexledesma added a reviewer: nate.
rexledesma removed a reviewer: nate.
rexledesma retitled this revision from RFC - Schema changes for future MySQL support to Migrate db.String to db.Text to support MySQL.Jan 14 2021, 8:16 AM
rexledesma edited the summary of this revision. (Show Details)
rexledesma edited the test plan for this revision. (Show Details)

remove duplicate migration of secondary_indexes

python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/versions/0da417ae1b81_change_varchar_to_text.py
35–39

Just as a callout, although secondary_indexes is defined in the runs and event_log schema, the table is only altered once in the migration.

python_modules/dagster/dagster_tests/general_tests/compat_tests/test_back_compat.py
314–326

Just removed some stuff in this test to make it pass

prha requested changes to this revision.Jan 14 2021, 5:22 PM

This looks pretty solid... nice work.

Some minor things to address here, first, though.

python_modules/dagster/dagster/core/storage/runs/sqlite/alembic/versions/0da417ae1b81_change_varchar_to_text.py
33

I think this should also have a secondary index section... In the sqlite case, it's two separate tables.

python_modules/dagster/dagster_tests/general_tests/compat_tests/test_back_compat.py
306

Why do we need to upgrade here? I think the point of the test is actually to not upgrade the instance (which will trigger a data migration for run storage).

You might need to change this call to instance._run_storage.upgrade() just to do the schema upgrade. Then I think the rest of the test (without the assert removals) will work.

354–355

This is still going to be brittle. I vote to remove this check.

Or just assert that the alembic version != b22f16781a7c

This revision now requires changes to proceed.Jan 14 2021, 5:22 PM
rexledesma edited the summary of this revision. (Show Details)

address comments on test

add back migration script to secondary_indexes for runs schema

okay, looks good to me!

charizard

This revision is now accepted and ready to land.Jan 14 2021, 6:06 PM
This revision was automatically updated to reflect the committed changes.