Page MenuHomePhabricator

Create Scheduler GraphQL interface
ClosedPublic

Authored by sashank on Aug 13 2019, 9:22 AM.

Details

Reviewers
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:021cc034dea5: Create Scheduler GraphQL interface
Test Plan

unit

Diff Detail

Repository
R1 dagster
Branch
schedule-storage-graphql
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sashank created this revision.Aug 13 2019, 9:22 AM
sashank updated this revision to Diff 3634.Aug 13 2019, 9:55 AM

Add tests

alangenfeld requested changes to this revision.Aug 13 2019, 3:21 PM
alangenfeld added a subscriber: alangenfeld.
alangenfeld added inline comments.
js_modules/dagit/src/schema.graphql
299

wat

752

this seems accidental?

python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py
30
  • do a lil namedtuple, these random string key accessors strewn about seem dubious
  • probably rename _schedules to make it clear it holds these wrapper objs
85–95

actually i would consider just adding file_path as a property to RunSchedule that doesnt get exposed to graphql

This revision now requires changes to proceed.Aug 13 2019, 3:21 PM
sashank updated this revision to Diff 3657.Aug 13 2019, 6:41 PM

Use namedtuple instead of dict to store schedule and file_path

sashank planned changes to this revision.Aug 13 2019, 6:55 PM
sashank updated this revision to Diff 3666.Aug 13 2019, 8:48 PM

Change type of execution_params to String

sashank updated this revision to Diff 3667.Aug 13 2019, 8:51 PM
sashank marked an inline comment as done.

make graphql

sashank marked 2 inline comments as done.Aug 13 2019, 8:56 PM
sashank added inline comments.
python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py
85–95

I think it might be best to leave it to the Scheduler implementation, since other implementations won't won't need file paths, will need to store multiple file paths, or use a database, etc.

For ex: I'll need to add a path to the cron shell script for this scheduler.

sashank updated this revision to Diff 3668.Aug 13 2019, 10:44 PM
sashank marked an inline comment as done.

Fix diff

sashank updated this revision to Diff 3677.Aug 13 2019, 11:19 PM

Remove timestamp from schedule filenames, removing the need for a wrapper around schedules in SystemCronScheduler

sashank marked an inline comment as done.Aug 13 2019, 11:29 PM
alangenfeld accepted this revision.Aug 14 2019, 6:46 PM
alangenfeld added inline comments.
js_modules/dagit/src/schema.graphql
752

is String right here? we just send the json blob?

python_modules/dagster-graphql/dagster_graphql/schema/run_schedule.py
10–11

maybe use a string instead of 1? not sure how beneficial it actually is, depends where the value actually shows up

117

might be worth having a slightly different name since this returns a string that needs to get json decoded by the client

This revision is now accepted and ready to land.Aug 14 2019, 6:46 PM
sashank marked an inline comment as done.Aug 14 2019, 9:05 PM
sashank added inline comments.
js_modules/dagit/src/schema.graphql
752

I think so. I don't see a situation where you would want to get just a part of the execution params, and I don't see a way to make it query-able since the structure changes based on config.

sashank updated this revision to Diff 3713.Aug 14 2019, 9:19 PM

Rename execution_params to execution_params_string in DauphinRunSchedule

Harbormaster completed remote builds in B2972: Diff 3714.