| alangenfeld | |
| schrockn |
| Restricted Project |
Scheduler API:
Scheduler is an abstract class which defines methods such as create_schedule, start_schedule, etc. Each instance of a Scheduler stores a list of RunSchedules
Two implementations of Scheduler included are:
unit
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Happy to sit and do a high level API design chat if you want. The GraphQL schema change is usually a good framing for making sure things feel right.
| python_modules/dagster-graphql/dagster_graphql/implementation/pipeline_schedule_storage.py | ||
|---|---|---|
| 142–216 ↗ | (On Diff #3606) | [1] I'm not a huge fan of this use of OOP - I think it will likely be much cleaner to just have Scheduler conformant classes vend a value type PipelineSchedule and have the implementation details of storage and retrieval be in the details of the Scheduler You can check with @max on how he is planning on refactoring the run storage since were hitting issues with this set up there |
| python_modules/dagster-graphql/dagster_graphql/implementation/pipeline_schedule_storage.py | ||
|---|---|---|
| 142–216 ↗ | (On Diff #3606) | +1 |
Scheduler API:
There is an abstract base class Scheduler, which defines abstract methods such as create_schedule, start_schedule, etc.
There are two scheduler classes that implement Scheduler:
Each scheduler stores a list of Schedules. The type of the schedules depends on the type of the scheduler. TestScheduler simply uses the base Schedule class, but SystemCronScheduler uses SystemCronSchedule, which implements the logic for reading the schedule to and from the file.
this feels worth putting in its own directory /dagster_graphql/implementation/scheduler/ maybe or just /dagster_graphql/scheduler/, might be time to move away from the catch all implementation folder.
| python_modules/dagster-graphql/dagster_graphql/implementation/schedule.py | ||
|---|---|---|
| 112 ↗ | (On Diff #3622) | lets move this in to its own file |
| 140–146 ↗ | (On Diff #3622) | you can stack this diff too ahead of landing anything right? |
| 174 ↗ | (On Diff #3622) | maybe call it RunSchedule, schedule on its own feels a touch vague |
| 199–234 ↗ | (On Diff #3622) | hm i still think this makes sense just to have as logic in SystemCronScheduler and not have any inheritance for Schedule. |
Moved code to /implementation/scheduler/ and deleted SystemCronSchedule (logic moved to SystemCronScheduler)
seems like a good start. will let @alangenfeld do final approval as you two have been discussing
| python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/scheduler.py | ||
|---|---|---|
| 78 | seems like a test scheduler should only live in tests or in a test.py file or something | |
| 108 | namedtuples are nice for value objects like this | |
| python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py | ||
| 54 | this is a little cute. i think we should just stick to get_schedule_by_id | |
Use keys instead of attributes to access execution_params dict in all_schedules_for_pipeline
| python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py | ||
|---|---|---|
| 89 | may want to consider a harder failure here | |
| python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py | ||
|---|---|---|
| 89 |
ya we probably just want to let the exception bubble | |
| python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py | ||
|---|---|---|
| 89 | going to need to use six.raise_from for py2 | |
| python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py | ||
|---|---|---|
| 89 | will get this in the next diff | |
| python_modules/dagster-graphql/dagster_graphql/implementation/scheduler/system_cron_scheduler.py | ||
|---|---|---|
| 89 | cool | |
