Page MenuHomeElementl

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

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

Details

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

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

updating dagster-mysql to be consistent w dagster-postgres

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

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

modifying setup.py and MANIFEST.in

sidkmenon marked 4 inline comments as done.

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

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 marked 2 inline comments as done.

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

Fixing alembic config to match D6684

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

Two small changes:

  1. Updating MANIFEST.in and setup.py for new alembic structure
  2. Updating test_event_log.py bc of downstream rebase