Page MenuHomePhabricator

(1/n scheduler-refactor) Remove schedule_id
ClosedPublic

Authored by sashank on Jan 22 2020, 10:25 PM.

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

sashank created this revision.Jan 22 2020, 10:25 PM
sashank retitled this revision from [RFC} Remove schedule id to [RFC] Remove schedule id.Jan 22 2020, 11:00 PM
sashank updated this revision to Diff 8822.Jan 22 2020, 11:00 PM
sashank retitled this revision from [RFC] Remove schedule id to [RFC} Remove schedule id.

Up

Harbormaster failed remote builds in B7159: Diff 8825!
sashank retitled this revision from [RFC} Remove schedule id to [RFC] Remove schedule id.Jan 23 2020, 12:11 AM
sashank updated this revision to Diff 8843.Jan 23 2020, 12:25 AM

Fix AppCache dataIdFromObject for RunningSchedule

sashank updated this revision to Diff 8850.Jan 23 2020, 1:46 AM

rebase on master

prha accepted this revision.Jan 23 2020, 6:20 PM

This shouldn't break existing running schedules because the scheduler manages those all by name, correct?

This revision is now accepted and ready to land.Jan 23 2020, 6:20 PM

This shouldn't break existing running schedules because the scheduler manages those all by name, correct?

Unfortunately, we managed all of them by id currently. So this is a hard breaking change. All existing schedules on the crontab will be invalid, and so will all schedule attempts. The user will need to restart all schedules with dagster schedule wipe && dagster schedule up, but it's possible we could write a migration script.

Not sure if the migration script will be worth it.

prha requested changes to this revision.Jan 23 2020, 7:27 PM

I think it should be fine, but we should add something to the changelog, saying "Need to run dagster schedule ..."

This revision now requires changes to proceed.Jan 23 2020, 7:27 PM
prha added a comment.Jan 23 2020, 7:28 PM

as in, not having the migration seems fine, when you can accomplish the same thing by running wipe and up

prha accepted this revision.Fri, Jan 24, 5:13 PM

is this still needed?

This revision is now accepted and ready to land.Fri, Jan 24, 5:13 PM
sashank planned changes to this revision.Fri, Jan 24, 8:30 PM

Yup, working on a stack that's based on this. Will also add to the changelog

sashank updated this revision to Diff 8916.Sat, Jan 25, 12:39 AM

Fix pylint

This revision is now accepted and ready to land.Sat, Jan 25, 12:39 AM
sashank planned changes to this revision.Sat, Jan 25, 12:39 AM
sashank retitled this revision from [RFC] Remove schedule id to (1/n scheduler-refactor) Remove schedule_id.Wed, Jan 29, 1:10 AM
sashank updated this revision to Diff 9297.Wed, Feb 5, 10:19 PM

rebase on master

This revision is now accepted and ready to land.Wed, Feb 5, 10:19 PM
This revision was automatically updated to reflect the committed changes.