Page MenuHomePhabricator
Differential D5805

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

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

Details

Reviewers
nate
prha
Commits
R1:a2d6677f4c5d: Migrate db.String to db.Text to support MySQL
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nate added a comment.Tue, Jan 12, 7:53 PM

up

Harbormaster failed remote builds in B24175: Diff 29408!Tue, Jan 12, 8:10 PM
nate edited the test plan for this revision. (Show Details)Tue, Jan 12, 8:14 PM
nate updated this revision to Diff 29413.Tue, Jan 12, 8:14 PM
nate edited the test plan for this revision. (Show Details)

up

Harbormaster completed remote builds in B24180: Diff 29413.Tue, Jan 12, 8:34 PM
nate updated this revision to Diff 29419.Tue, Jan 12, 8:55 PM

up

Harbormaster failed remote builds in B24183: Diff 29419!Tue, Jan 12, 9:09 PM
prha added a subscriber: prha.Wed, Jan 13, 4:58 PM
prha added inline comments.
python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/versions/0da417ae1b81_change_varchar_to_text.py
26–35

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
26–35

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
8

nice

rexledesma commandeered this revision.Wed, Jan 13, 11:33 PM
rexledesma added a reviewer: nate.
rexledesma removed a reviewer: nate.
rexledesma removed a subscriber: prha.Thu, Jan 14, 6:11 AM
rexledesma updated this revision to Diff 29658.Thu, Jan 14, 6:12 AM

rebase

Harbormaster failed remote builds in B24367: Diff 29658!Thu, Jan 14, 6:27 AM
rexledesma updated this revision to Diff 29663.Thu, Jan 14, 7:23 AM

up

Harbormaster failed remote builds in B24372: Diff 29663!Thu, Jan 14, 7:39 AM
rexledesma updated this revision to Diff 29664.Thu, Jan 14, 8:00 AM

update down revisions

rexledesma updated this revision to Diff 29665.Thu, Jan 14, 8:10 AM

up

rexledesma retitled this revision from RFC - Schema changes for future MySQL support to Migrate db.String to db.Text to support MySQL.Thu, Jan 14, 8:16 AM
rexledesma edited the summary of this revision. (Show Details)
rexledesma edited the test plan for this revision. (Show Details)
Harbormaster failed remote builds in B24373: Diff 29664!Thu, Jan 14, 8:20 AM
Harbormaster failed remote builds in B24374: Diff 29665!Thu, Jan 14, 8:28 AM
rexledesma updated this revision to Diff 29666.Thu, Jan 14, 8:38 AM
rexledesma edited the summary of this revision. (Show Details)

up

Harbormaster failed remote builds in B24375: Diff 29666!Thu, Jan 14, 8:57 AM
rexledesma updated this revision to Diff 29667.Thu, Jan 14, 9:10 AM

up

rexledesma updated this revision to Diff 29668.Thu, Jan 14, 9:20 AM

up

Harbormaster failed remote builds in B24376: Diff 29667!Thu, Jan 14, 9:28 AM
Harbormaster failed remote builds in B24377: Diff 29668!Thu, Jan 14, 9:38 AM
rexledesma updated this revision to Diff 29669.Thu, Jan 14, 9:42 AM

up

rexledesma updated this revision to Diff 29670.Thu, Jan 14, 9:58 AM

up

rexledesma updated this revision to Diff 29671.Thu, Jan 14, 10:01 AM

up

Harbormaster failed remote builds in B24378: Diff 29669!Thu, Jan 14, 10:01 AM
Harbormaster failed remote builds in B24379: Diff 29670!Thu, Jan 14, 10:11 AM
Harbormaster failed remote builds in B24380: Diff 29671!Thu, Jan 14, 10:19 AM
rexledesma updated this revision to Diff 29672.Thu, Jan 14, 11:36 AM

uppppppp

Harbormaster failed remote builds in B24381: Diff 29672!Thu, Jan 14, 11:54 AM
rexledesma updated this revision to Diff 29685.Thu, Jan 14, 3:57 PM

up

rexledesma updated this revision to Diff 29686.Thu, Jan 14, 4:00 PM

up

rexledesma updated this revision to Diff 29694.Thu, Jan 14, 4:14 PM

up

Harbormaster failed remote builds in B24391: Diff 29685!Thu, Jan 14, 4:17 PM
Harbormaster failed remote builds in B24392: Diff 29686!
rexledesma updated this revision to Diff 29698.Thu, Jan 14, 4:25 PM

remove duplicate migration of secondary_indexes

Harbormaster failed remote builds in B24398: Diff 29694!Thu, Jan 14, 4:31 PM
Harbormaster failed remote builds in B24402: Diff 29698!Thu, Jan 14, 4:41 PM
rexledesma updated this revision to Diff 29703.Thu, Jan 14, 4:58 PM

update broken tests

rexledesma edited the summary of this revision. (Show Details)Thu, Jan 14, 5:08 PM
rexledesma added reviewers: nate, prha.
rexledesma added inline comments.Thu, Jan 14, 5:12 PM
python_modules/dagster/dagster/core/storage/event_log/sqlite/alembic/versions/0da417ae1b81_change_varchar_to_text.py
34–38

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

Harbormaster completed remote builds in B24403: Diff 29703.Thu, Jan 14, 5:17 PM
prha requested changes to this revision.Thu, Jan 14, 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
32

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
305

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.Thu, Jan 14, 5:22 PM
rexledesma updated this revision to Diff 29707.Thu, Jan 14, 5:30 PM
rexledesma edited the summary of this revision. (Show Details)

address comments on test

rexledesma updated this revision to Diff 29708.Thu, Jan 14, 5:33 PM

add back migration script to secondary_indexes for runs schema

Harbormaster completed remote builds in B24407: Diff 29707.Thu, Jan 14, 5:49 PM
Harbormaster completed remote builds in B24408: Diff 29708.Thu, Jan 14, 5:52 PM
rexledesma updated this revision to Diff 29711.Thu, Jan 14, 5:54 PM

rebase

prha accepted this revision.Thu, Jan 14, 6:06 PM

okay, looks good to me!

charizard

This revision is now accepted and ready to land.Thu, Jan 14, 6:06 PM
rexledesma edited the summary of this revision. (Show Details)Thu, Jan 14, 6:16 PM
Harbormaster completed remote builds in B24411: Diff 29711.Thu, Jan 14, 6:17 PM
Closed by commit R1:a2d6677f4c5d: Migrate db.String to db.Text to support MySQL (authored by nate, committed by rexledesma). · Explain WhyThu, Jan 14, 6:18 PM
This revision was automatically updated to reflect the committed changes.
rexledesma added a commit: R1:a2d6677f4c5d: Migrate db.String to db.Text to support MySQL.

Revision Contents
Changeset List

PathSize
.buildkite/
dagster-buildkite/
dagster_buildkite/
steps/
10 lines
python_modules/
dagster/
dagster/
core/
storage/
event_log/
10 lines
sqlite/
alembic/
versions/
70 lines
runs/
14 lines
sqlite/
alembic/
versions/
66 lines
schedules/
4 lines
sqlite/
alembic/
versions/
42 lines
dagster_tests/
general_tests/
compat_tests/
3 lines
libraries/
dagster-postgres/
dagster_postgres/
event_log/
alembic/
versions/
90 lines
run_storage/
alembic/
versions/
90 lines
schedule_storage/
alembic/
versions/
90 lines
scripts/
46 lines

Diff 29715

View Options

.buildkite/dagster-buildkite/dagster_buildkite/steps/dagster.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/event_log/schema.py

Loading...
View Options

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

Loading...
View Options

python_modules/dagster/dagster/core/storage/runs/schema.py

Loading...
View Options

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

Loading...
View Options

python_modules/dagster/dagster/core/storage/schedules/schema.py

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

python_modules/libraries/dagster-postgres/dagster_postgres/run_storage/alembic/versions/4ea2b1f6f67b_change_varchar_to_text.py

Loading...
View Options

python_modules/libraries/dagster-postgres/dagster_postgres/schedule_storage/alembic/versions/4ea2b1f6f67b_change_varchar_to_text.py

Loading...
View Options

scripts/check_schemas.py

Loading...