Page MenuHomeElementl
Differential D6617

[dagster-mysql] Add dagster-mysql to libraries
ClosedPublic
Actions

Authored by sidkmenon on Feb 19 2021, 10:50 PM.

Details

Reviewers
max
prha
catherinewu
rexledesma
Commits
R1:7eff7a85a436: [dagster-mysql] Add dagster-mysql to libraries
Summary

This commit follows the scaffold set out by @nate in D5710, but I also utilize

  1. schema changes found in D6406
  2. the SqlPollingEventWatcher found in D6616
  3. MySQL-specific dialect statements in Sqlalchemy for run-storage (run_storage.py::add_daemon_heartbeat) & asset key storage (event_log.py::store_asset_key)

Co-authored by: Nate Kupp <nate@elementl.com>
Co-authored by: Sid Menon <sid@elementl.com>

Depends on D6519, D6406, D6616, D6701, D6714

Test Plan

Integration + bk (+dagster-mysql tests)

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sidkmenon created this revision.Feb 19 2021, 10:50 PM
sidkmenon edited the summary of this revision. (Show Details)Feb 19 2021, 10:58 PM
sidkmenon edited the test plan for this revision. (Show Details)
sidkmenon added parent revisions: D6519: [buildkite][core] Adding checks for `db.text("CURRENT_TIMESTAMP")`, D6616: [core] Event watcher polling implementation, D6406: [core] schema changes for MySQL compatability.
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 19 2021, 11:07 PM
Harbormaster failed remote builds in B26238: Diff 32053!
sidkmenon updated this revision to Diff 32068.Feb 21 2021, 12:17 AM

up

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 21 2021, 12:35 AM
Harbormaster failed remote builds in B26252: Diff 32068!
sidkmenon updated this revision to Diff 32069.Feb 21 2021, 6:46 AM

updating dagster-mysql to be consistent w dagster-postgres

Harbormaster completed remote builds in B26253: Diff 32069.Feb 21 2021, 7:06 AM
sidkmenon requested review of this revision.Feb 21 2021, 7:06 AM
sidkmenon added reviewers: max, prha, catherinewu, rexledesma.Feb 22 2021, 3:53 PM
sidkmenon edited the summary of this revision. (Show Details)
prha requested changes to this revision.Feb 22 2021, 5:31 PM
prha added a subscriber: yuhan.

I know this is mirroring the structure of the postgres library, but in light of the tedious way we manage alembic for postgres, I'd like to propose an alternate structure. I'd like there to be a single alembic migrations directory for all three storages. Each storage would point to that migrations directory via the script locations config in alembic.ini. That way we don't have to do this whole dance of maintaining 3 copies of the same migration file.

We'll also need to add a section to the https://elementl.quip.com/l3XaAJ2YjRSb/Storage-migrations section for MySQL (separate from this diff).

python_modules/libraries/dagster-mysql/dagster_mysql/event_log/event_log.py
36

dagster-mysql?

Also, I think you'll probably have to create a docs snippet about this...

Should also consider if we have to start including all the different storage options in the AWS/GCP deployment docs (cc @yuhan, @catherinewu )

67

uncomment? Pretty sure we need this...

74

remove...

102–113

i'm confused.... do we need to call close on the result when we execute?

It looks like that's the only difference between this and the overridden method from the parent class.

168

what does this mean?

python_modules/libraries/dagster-mysql/dagster_mysql/run_storage/run_storage.py
29

same here

56

We need this, right?

python_modules/libraries/dagster-mysql/dagster_mysql/schedule_storage/schedule_storage.py
26

same here

python_modules/libraries/dagster-mysql/dagster_mysql/utils.py
28–29

remove

76

what's this about?

99

?

python_modules/libraries/dagster-mysql/dagster_mysql_tests/docker-compose-multi.yml
11–12

you have to specify both the root password and mysql password?

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_event_log.py
38

instead of copy/paste, can we use a similar structure to the run_storage tests and create a TestEventLogStorage test suite and add the MySQL storage to the set of fixtures?

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_run_storage.py
35–106

I'm confused.... are these just extra tests? Are they not covered in the TestRunStorage suite?

134

TEST_MYSQL_PASSWORD?

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_schedule_storage.py
11–13

It's a little weird that we're creating the clean_schedule_storage and then only using it here in another fixture instead of just calling MySQLScheduleStorage.create_clean_storage here

This revision now requires changes to proceed.Feb 22 2021, 5:31 PM
sidkmenon added inline comments.Feb 22 2021, 5:44 PM
python_modules/libraries/dagster-mysql/dagster_mysql/event_log/event_log.py
67

TODO: setup base revision in versions/ folder

python_modules/libraries/dagster-mysql/dagster_mysql/run_storage/run_storage.py
56

yup absolutely - I need to setup the versions/ folder for it to work, though

sidkmenon updated this revision to Diff 32152.Feb 23 2021, 5:46 AM

addressing (some of) Phil's comments: alembic refactor + addressing hanging "TODOs"

sidkmenon updated this revision to Diff 32153.Feb 23 2021, 5:58 AM

modifying setup.py and MANIFEST.in

Harbormaster completed remote builds in B26313: Diff 32152.Feb 23 2021, 6:07 AM
Harbormaster completed remote builds in B26314: Diff 32153.Feb 23 2021, 6:16 AM
sidkmenon updated this revision to Diff 32157.Feb 23 2021, 6:24 AM
sidkmenon marked 4 inline comments as done.

up - addressing more comments (getting rid of TODOs and unneccessary methods in MySQLEventLogStorage)

Harbormaster completed remote builds in B26316: Diff 32157.Feb 23 2021, 6:43 AM
prha mentioned this in D6684: have postgres storages all point to the same migration folder.Feb 25 2021, 11:10 PM
sidkmenon edited the summary of this revision. (Show Details)Feb 26 2021, 5:34 PM
sidkmenon added a parent revision: D6701: [core] Adding shared Event Log storage test suite.
prha mentioned this in R1:4128f16054c4: have postgres storages all point to the same migration folder.Feb 26 2021, 7:08 PM
sidkmenon mentioned this in D6714: [core][dagster-postgres] Adding dagster-postgres tests to `TestRunStorage` suite.Feb 26 2021, 10:54 PM
sidkmenon mentioned this in D6715: [dagster-postgres] cleaning up ScheduleStorage suite.Feb 26 2021, 10:57 PM
sidkmenon mentioned this in D6701: [core] Adding shared Event Log storage test suite.Feb 26 2021, 10:59 PM
sidkmenon marked 12 inline comments as done.Feb 26 2021, 11:27 PM
sidkmenon added inline comments.
python_modules/libraries/dagster-mysql/dagster_mysql/event_log/event_log.py
102–113

You're totally right - I was porting over the stuff from dagster-postgres and didn't notice the 'close' call. Should be fixed now.

168

Honestly not sure - I see the same description in dagster-postgres and figured that the comment was there for a reason? Since I don't know what it means though I'll definitely remove it though

python_modules/libraries/dagster-mysql/dagster_mysql/utils.py
76

for future reference - this and the below TODO were about the lack of specificity on the catch-all Exception

python_modules/libraries/dagster-mysql/dagster_mysql_tests/docker-compose-multi.yml
11–12

Yes I think so - looks like the ROOT_PASSWORD is required, and the MYSQL_PASSWORD is necessary to set up the user account that we use in tests. Could definitely be wrong though - here's the documentation I looked at for clarification - https://hub.docker.com/_/mysql

sidkmenon updated this revision to Diff 32466.Feb 26 2021, 11:28 PM
sidkmenon marked 2 inline comments as done.

Addressing the rest of @prha's comments in refactoring test suite & general cleanup

sidkmenon edited the summary of this revision. (Show Details)Feb 26 2021, 11:38 PM
sidkmenon added a parent revision: D6714: [core][dagster-postgres] Adding dagster-postgres tests to `TestRunStorage` suite.
Harbormaster completed remote builds in B26572: Diff 32466.Feb 26 2021, 11:49 PM
sidkmenon updated this revision to Diff 32510.Mar 1 2021, 3:33 AM

Fixing alembic config to match D6684

sidkmenon edited the summary of this revision. (Show Details)Mar 1 2021, 3:41 AM
Harbormaster completed remote builds in B26606: Diff 32510.Mar 1 2021, 3:55 AM
prha accepted this revision.Mar 1 2021, 6:06 PM

I think this looks great!!!

animated_excited_guy

python_modules/libraries/dagster-mysql/setup.py
33–36

nice catch, will need to patch this for pg.

This revision is now accepted and ready to land.Mar 1 2021, 6:06 PM
prha added a comment.Mar 1 2021, 6:07 PM

herewego

sidkmenon updated this revision to Diff 32533.Mar 1 2021, 6:32 PM

Two small changes:

  1. Updating MANIFEST.in and setup.py for new alembic structure
  2. Updating test_event_log.py bc of downstream rebase
Harbormaster completed remote builds in B26626: Diff 32533.Mar 1 2021, 6:50 PM
Sidharth Menon <menon.sid.k@gmail.com> mentioned this in R1:1dec82e94d0e: [core][dagster-postgres] Adding dagster-postgres tests to `TestRunStorage` suite.Mar 1 2021, 7:13 PM
Sidharth Menon <menon.sid.k@gmail.com> mentioned this in R1:ac7bbd22d937: [dagster-postgres] cleaning up ScheduleStorage suite.
Sidharth Menon <menon.sid.k@gmail.com> mentioned this in R1:3f17d53e951b: [core] Adding shared Event Log storage test suite.Mar 1 2021, 8:04 PM
Closed by commit R1:7eff7a85a436: [dagster-mysql] Add dagster-mysql to libraries (authored by Sidharth Menon <menon.sid.k@gmail.com>). · Explain WhyMar 1 2021, 8:10 PM
This revision was automatically updated to reflect the committed changes.
Sidharth Menon <menon.sid.k@gmail.com> added a commit: R1:7eff7a85a436: [dagster-mysql] Add dagster-mysql to libraries.

Revision Contents
Changeset List

PathSize
.buildkite/
dagster-buildkite/
dagster_buildkite/
steps/
23 lines
docs/
sections/
api/
apidocs/
libraries/
10 lines
examples/
docs_snippets/
docs_snippets/
deploying/
32 lines
python_modules/
dagster/
dagster/
utils/
test/
233 lines
libraries/
dagster-mysql/
2 lines
201 lines
2 lines
4 lines
dagster_mysql/
9 lines
alembic/
3 lines
50 lines
74 lines
24 lines
versions/
21 lines
event_log/
1 line
159 lines
run_storage/
1 line
120 lines
schedule_storage/
1 line
89 lines
168 lines
1 line
dagster_mysql_tests/
16 lines
33 lines
20 lines
112 lines
95 lines
73 lines
15 lines
5 lines
2 lines
37 lines
28 lines

Diff 32539

View Options

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

Loading...
View Options

docs/sections/api/apidocs/libraries/dagster_mysql.rst

Loading...
View Options

examples/docs_snippets/docs_snippets/deploying/dagster-mysql.yaml

Loading...
View Options

python_modules/dagster/dagster/utils/test/mysql_instance.py

Loading...
View Options

python_modules/libraries/dagster-mysql/.coveragerc

Loading...
View Options

python_modules/libraries/dagster-mysql/LICENSE

Loading...
View Options

python_modules/libraries/dagster-mysql/MANIFEST.in

Loading...
View Options

python_modules/libraries/dagster-mysql/README.md

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/__init__.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/alembic/README

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/alembic/alembic.ini

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/alembic/env.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/alembic/script.py.mako

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/alembic/versions/ba4050312958_initial_revision_for_mysql_support.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/event_log/__init__.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/event_log/event_log.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/run_storage/__init__.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/run_storage/run_storage.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/schedule_storage/__init__.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/schedule_storage/schedule_storage.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/utils.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql/version.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/__init__.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/conftest.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/docker-compose-multi.yml

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/docker-compose.yml

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_event_log.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_instance.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_run_storage.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_schedule_storage.py

Loading...
View Options

python_modules/libraries/dagster-mysql/dagster_mysql_tests/test_version.py

Loading...
View Options

python_modules/libraries/dagster-mysql/setup.cfg

Loading...
View Options

python_modules/libraries/dagster-mysql/setup.py

Loading...
View Options

python_modules/libraries/dagster-mysql/tox.ini

Loading...