Page MenuHomePhabricator

[sensors-7] external job storage for schedules
ClosedPublic

Authored by prha on Sat, Nov 14, 12:32 AM.

Details

Summary

Rewrites schedules to use generic job storage.

This diff is kind of beefy, but only has a couple conceptual components and is mostly mechanical:

  • Stored job ids are based on external origin, rather than origin, so we have to change how we fetch/reconcile to be based on external_origin_id
  • ScheduleState is now JobState, with job_specific_data taking ScheduleJobData or SensorJobData, depending on the job type.

This is a non-backward compatible change. It will require a schedule wipe and schedule up. I will land it only after we have split master / release branches.

Test Plan

bk

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

prha requested review of this revision.Sat, Nov 14, 12:47 AM
dgibson requested changes to this revision.Tue, Nov 17, 5:30 PM

ah sorry there's some overlap here with https://dagster.phacility.com/D4941 which I was *also* planning on landing after the branch cut. Is it worth rebasing on top of that for the origin piece once I rebase it and clean it up?

js_modules/dagit/src/DagsterRepositoryContext.tsx
45–46 ↗(On Diff #25684)

imo we should just swap the implementation of id to use the new origin? dagit is a host process, externalId is more stable, it's confusing to have two IDs exposed

js_modules/dagit/src/schema.graphql
464–467

why

python_modules/dagster-graphql/dagster_graphql/schema/schedules/schedule_state.py
119–120

There's a perfectly good external_repository_origin right there already :)
origin = self._schedule_state.origin.external_repository_origin

(creating a new location handle involves spinning up a gRPC server)

python_modules/libraries/dagster-cron/dagster_cron/cron_scheduler.py
197–199 ↗(On Diff #25684)

as above we don't want to do this, it spins up a new gRPC server. maybe rebase on top of https://dagster.phacility.com/D4941 instead?

This revision now requires changes to proceed.Tue, Nov 17, 5:30 PM

a couple of somewhat important inlines of things to revert, but other than that this looks good. Will land the origin changes this is on top of ASAP

js_modules/dagit/src/schema.graphql
34

this is gone now no?

python_modules/dagster-graphql/dagster_graphql/schema/external.py
28–43 ↗(On Diff #26030)

this should be removed, external_id is just id now

python_modules/libraries/dagster-cron/dagster_cron/cron_scheduler.py
190–225 ↗(On Diff #26030)

you should be able to revert this set of changes

This revision is now accepted and ready to land.Fri, Nov 20, 4:08 PM
This revision was automatically updated to reflect the committed changes.