Page MenuHomePhabricator
Differential D5805

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

Authored by rexledesma on Jan 4 2021, 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
Branch
arcpatch-D5805
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster failed remote builds in B24175: Diff 29408!Jan 12 2021, 8:10 PM
nate edited the test plan for this revision. (Show Details)Jan 12 2021, 8:14 PM
nate updated this revision to Diff 29413.Jan 12 2021, 8:14 PM
nate edited the test plan for this revision. (Show Details)

up

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

up

Harbormaster failed remote builds in B24183: Diff 29419!Jan 12 2021, 9:09 PM
prha added a subscriber: prha.Jan 13 2021, 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.Jan 13 2021, 11:33 PM
rexledesma added a reviewer: nate.
rexledesma removed a reviewer: nate.
rexledesma removed a subscriber: prha.Jan 14 2021, 6:11 AM
rexledesma updated this revision to Diff 29658.Jan 14 2021, 6:12 AM

rebase

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

up

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

update down revisions

rexledesma updated this revision to Diff 29665.Jan 14 2021, 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.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)
Harbormaster failed remote builds in B24373: Diff 29664!Jan 14 2021, 8:20 AM
Harbormaster failed remote builds in B24374: Diff 29665!Jan 14 2021, 8:28 AM
rexledesma updated this revision to Diff 29666.Jan 14 2021, 8:38 AM
rexledesma edited the summary of this revision. (Show Details)

up

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

up

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

up

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

up

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

up

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

up

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

uppppppp

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

up

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

up

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

up

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

remove duplicate migration of secondary_indexes

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

update broken tests

rexledesma edited the summary of this revision. (Show Details)Jan 14 2021, 5:08 PM
rexledesma added reviewers: nate, prha.
rexledesma added inline comments.Jan 14 2021, 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–324

Just removed some stuff in this test to make it pass

Harbormaster completed remote builds in B24403: Diff 29703.Jan 14 2021, 5:17 PM
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
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
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.

347

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

address comments on test

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

add back migration script to secondary_indexes for runs schema

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

rebase

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

okay, looks good to me!

charizard

This revision is now accepted and ready to land.Jan 14 2021, 6:06 PM
rexledesma edited the summary of this revision. (Show Details)Jan 14 2021, 6:16 PM
Harbormaster completed remote builds in B24411: Diff 29711.Jan 14 2021, 6:17 PM
Closed by commit R1:a2d6677f4c5d: Migrate db.String to db.Text to support MySQL (authored by nate, committed by rexledesma). · Explain WhyJan 14 2021, 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.
sidkmenon mentioned this in D6406: [core] schema changes for MySQL compatability.Wed, Feb 10, 4:51 PM

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/
54 lines
schedules/
4 lines
sqlite/
alembic/
versions/
42 lines
dagster_tests/
general_tests/
compat_tests/
7 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 29703

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...