Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schedule API updates #230

Merged
merged 9 commits into from
Sep 13, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 104 additions & 30 deletions temporal/api/schedule/v1/message.proto
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

// (-- api-linter: core::0203::optional=disabled
// aip.dev/not-precedent: field_behavior annotation not available in our gogo fork --)
// (-- api-linter: core::0203::input-only=disabled
// aip.dev/not-precedent: field_behavior annotation not available in our gogo fork --)

syntax = "proto3";

package temporal.api.schedule.v1;
Expand All @@ -41,21 +46,23 @@ import "temporal/api/enums/v1/schedule.proto";
import "temporal/api/workflow/v1/message.proto";

// CalendarSpec describes an event specification relative to the calendar,
// similar to a traditional cron specification. Each field can be one of:
// similar to a traditional cron specification, but with labeled fields. Each
// field can be one of:
// *: matches always
// x: matches when the field equals x
// x/y : matches when the field equals x+n*y where n is an integer
// x-z: matches when the field is between x and z inclusive
// w,x,y,...: matches when the field is one of the listed values
// Each x, y, z, ... is either a decimal integer, or a month or day of week name
// or abbreviation (in the appropriate fields).
// A second in time matches if all fields match.
// A timestamp matches if all fields match.
// Note that fields have different default values, for convenience.
// Note that the special case that some cron implementations have for treating
// day_of_month and day_of_week as "or" instead of "and" when both are set is
// not implemented.
// day_of_week can accept 0 or 7 as Sunday
// TODO: add relative-to-end-of-month
// TODO: add nth day-of-week in month
// CalendarSpec gets compiled into StructuredCalendarSpec, which is what will be
// returned if you describe the schedule.
message CalendarSpec {
// Expression to match seconds. Default: 0
string second = 1;
Expand All @@ -73,6 +80,50 @@ message CalendarSpec {
string year = 6;
// Expression to match days of the week. Default: *
string day_of_week = 7;
// Free-form comment describing the intention of this spec.
string comment = 8;
}

// Range represents a set of integer values, used to match fields of a calendar
// time in StructuredCalendarSpec. If end < start, then end is interpreted as
// equal to start. This means you can use a Range with start set to a value, and
// end and step unset (defaulting to 0) to represent a single value.
message Range {
// Start of range (inclusive).
int32 start = 1;
// End of range (inclusive).
int32 end = 2;
// Step (optional, default 1).
int32 step = 3;
}

// StructuredCalendarSpec describes an event specification relative to the
// calendar, in a form that's easy to work with programmatically. Each field can
// be one or more ranges.
// A timestamp matches if at least one range of _each_ field match the
// corresponding fields of the timestamp. (That implies that all fields must
// have at least one range if you want to match anything.)
// TODO: add relative-to-end-of-month
// TODO: add nth day-of-week in month
message StructuredCalendarSpec {
// Match seconds (0-59)
repeated Range second = 1;
// Match minutes (0-59)
repeated Range minute = 2;
// Match hours (0-23)
repeated Range hour = 3;
// Match days of the month (1-31)
// (-- api-linter: core::0140::prepositions=disabled
// aip.dev/not-precedent: standard name of field --)
repeated Range day_of_month = 4;
// Match months (1-12)
repeated Range month = 5;
// Match years (2000-).
repeated Range year = 6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will the default end value be? Like [{ start: 2000, end: 'INFINITY' }] or some large number, like max safe int32 end: 2147483647

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effective end value (the end of time 😱) is 2100, but I don't want to put that in a comment since it's an easily-adjusted constant. But there's no limit to the values that can appear in this Range.

Note that none of these have "defaults": see the comment above, you have to specify something. The range in parens is just documentation about what the meaning of the fields are, i.e. month and day of month are 1-indexed, others are zero-indexed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the year input defaults to '*' (in either 5-part cron expression or calendar spec), so I'm wondering how that's represented in the response. [{ start: 2000, end: 2100 }]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, if you get it back you'll see { start: 2000, end: 2100 }. That's not great, maybe we should have a better sentinel value there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ends at 2100 is not too bad. Maybe you want just document it that current implementation will ends at 2100, but it is easily adjustable. No big deal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a better sentinel, but if we keep 2100, how would I document it—like this?

When using '*' for the year (default), the year response value will currently be [{ start: 2000, end: 2100 }]. The end value will change to a later year when the current year approaches 2100.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a specific suggestion for sentinel values? I'm fine with anything here, just not sure what's easiest to work with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like [{ start: 2000, end: 'INFINITY' }] or some large number, like max safe int32 end: 2147483647

Between these two, integer is easier to work with. Maybe make it an even number so it's more obviously a sentinel? Like 1 billion.

If humans are still around by then, we'll probably at least be done with Earth
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are ints, so we can't use a string here. practically, I think there's not much real difference. the max possible year isn't at thing the proto level anyway, it's just a convention. (well, unless we introduce a new field.) if we establish a strong convention, or another way to signal this (e.g. leave out the year field here), it can be done in a backwards-compatible way. so I think we can just leave this unspecified for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving out is my favorite solution

// Match days of the week (0-6; 0 is Sunday).
repeated Range day_of_week = 7;
// Free-form comment describing the intention of this spec.
string comment = 8;
}

// IntervalSpec matches times that can be expressed as:
Expand All @@ -97,24 +148,52 @@ message IntervalSpec {
// definition of a time zone can change over time (most commonly, when daylight
// saving time policy changes for an area). To create a totally self-contained
// ScheduleSpec, use UTC or include timezone_data.
//
// cron_string holds a traditional cron specification as a string. It accepts 5,
// 6, or 7 fields, separated by spaces, and interprets them the same way as
// CalendarSpec.
// 5 fields: minute, hour, day_of_month, month, day_of_week
// 6 fields: minute, hour, day_of_month, month, day_of_week, year
// 7 fields: second, minute, hour, day_of_month, month, day_of_week, year
// If year is not given, it defaults to *. If second is not given, it defaults
// to 0.
// Shorthands @yearly, @monthly, @weekly, @daily, and @hourly are also accepted
// in place of the 5-7 time fields.
// Optionally, the string can be preceded by TZ=<timezone name> or
// CRON_TZ=<timezone name> to set the timezone_name field in ScheduleSpec.
// Optionally, a comment can appear after a "#".
// Note that the special case that some cron implementations have for treating
// day_of_month and day_of_week as "or" instead of "and" when both are set is
// not implemented.
// A cron_string in the format above gets compiled into a StructuredCalendarSpec,
// which is what will be returned if you describe the schedule.
// @every <interval>[/<phase>] is accepted and gets compiled into an
// IntervalSpec instead. <interval> and <phase> should have a unit suffix s, m,
// h, or d.
message ScheduleSpec {
// Calendar-based specifications of times.
// Calendar-based specifications of times. Note that calendar and
// cron_string are input-only! They will be compiled into
// describe a schedule, you won't see calendar or cron_string, you'll see
// only structured_calendar and interval.
repeated StructuredCalendarSpec structured_calendar = 7;
// See comment above for cron_string format.
repeated string cron_string = 8;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could move the comment here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. I left there because it's really long and wanted this to read more easily, but I guess it'll screw up the generated docs, so I should move it

repeated CalendarSpec calendar = 1;
// Interval-based specifications of times.
repeated IntervalSpec interval = 2;
// Any timestamps matching any of the exclude_calendar specs will be
// skipped.
repeated CalendarSpec exclude_calendar = 3;
// Any timestamps before start_time will be skipped. Together, start_time
// and end_time make an inclusive interval.
// Any timestamps matching any of exclude_* will be skipped.
repeated CalendarSpec exclude_calendar = 3 [deprecated = true]; // use exclude_structured_calendar
repeated StructuredCalendarSpec exclude_structured_calendar = 9;
Comment on lines +189 to +191
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why deprecate? If non-structured is the better-DX input format, I'd allow it here too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, none of these is a great input format for excludes: the way this works (for now) is that excludes have to match the same "second" as the thing they're excluding. So the obvious:

exclude_calendar: {"month":"12","day_of_month":"31"}

doesn't work, because of the 0 defaults for h/m/s. You have to write

exclude_calendar: {"month":"12","day_of_month":"31","hour":"*","minute:"*","second":"*"}

For structured, you'd have to spell out all those ranges.

If you can come up with a better way to do excludes, please share. I think this will eventually require some user feedback. In the meantime I'd like to keep the number of options small. Actually, that's a good argument for not supporting any of this at all until we have a good solution. Maybe I should remove this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calendar spec has default 0 h/m/s. I assume @daily also uses default 0?

So for example if I create with @daily and exclude_calendar: {"month":"12","day_of_month":"31"}, it will exclude Dec 31st?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically correct (although I haven't implemented @daily yet). But it will fail to exclude if you add jitter, for example. Basically it doesn't capture the user's intent very well. I think we need to better understand the intent first. The uses I've heard of are mostly around excluding holidays, and carving out exclusions when external systems are known to be down for maintenance or whatever.

There's also the idea of creating a shared "holiday calendar" that can be referenced from multiple schedules so they don't all have to repeat/maintain the same (potentially large) list of excludes. Of course, this can also be done inside the workflow: check the start time against a list of holidays, possibly even obtained from an external service, and exit early if it's not supposed to run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think exclude would be much easier to use if it applied to the pre-jitter-applied timestamp—would that be possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other cases in which the exclude schedule you specify won't apply as intended? I think that if we say in the exclude docs that everything including seconds must match, and someone has a spec with seconds: 30, they'd get that they'd have to put seconds: 30 or seconds: '*' in the exclude.

One hard thing might be providing interval spec and calendar exclude. Like if my interval is every 10 minutes since epoc and I want to skip every 4th, then I'd like to be able to specify exclude_interval: { every: 40 }.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for excludes, we may want to default unspecified fields to *.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely don't like the idea of changing the semantics of Calendar or CronString depending on the context.

It does seem like excludes in general could use more work. As @lorensr said, in principle there's no reason why you couldn't have an exclude_interval. So we kinda want something like:

  message SpecSet {
    repeated CalendarSpec calendar;
    repeated CronString cron_string;
    repeated IntervalSpec interval;
    repeated StructuredCalendarSpec structured_calendar;
  }

  message ScheduleSpec {
    SpecSet include;
    SpecSet exclude;
  }

But that's a fairly incompatible change and I don't think we should go there until we have a better idea of what users actually want here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better idea of what users actually want here.

Better idea during beta or after full release of schedules? If latter, are we okay breaking the API post release? If not, should we make proto changes now such that adding exclude interval etc later would be backcompat?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after. we can change things in a compatible way later. possibly more messy than doing it upfront, but we have to release something before we know exactly what to do.

// If start_time is set, any timestamps before start_time will be skipped.
// (Together, start_time and end_time make an inclusive interval.)
google.protobuf.Timestamp start_time = 4 [(gogoproto.stdtime) = true];
// Any timestamps after end_time will be skipped.
// If end_time is set, any timestamps after end_time will be skipped.
google.protobuf.Timestamp end_time = 5 [(gogoproto.stdtime) = true];
// All timestamps will be incremented by a random value from 0 to this
// amount of jitter. Default: 1 second
// amount of jitter. Default: 0
google.protobuf.Duration jitter = 6 [(gogoproto.stdduration) = true];

// Time zone to interpret all CalendarSpecs in.
// Time zone to interpret all calendar-based specs in.
//
// If unset, defaults to UTC. We recommend using UTC for your application if
// at all possible, to avoid various surprising properties of time zones.
Expand All @@ -134,21 +213,17 @@ message ScheduleSpec {
// at 2:30am and specify a time zone that follows DST, that action will not
// be triggered on the day that has no 2:30am. Similarly, an action that
// fires at 1:30am will be triggered twice on the day that has two 1:30s.
//
// Also note that no actions are taken on leap-seconds (e.g. 23:59:60 UTC).
string timezone_name = 10;
bytes timezone_data = 11;
}

message SchedulePolicies {
// Policy for overlaps.
// Note that this can be changed after a schedule has taken some actions, and we can't
// provide 100% sensible semantics for all changes. The most confusing case would be
// changes to/from ALLOW_ALL: with that policy multiple scheduled workflows can run
// concurrently, but for all other policies only one can run at a time. Changing
// between these two classes will leave all workflows with the other class alone.
// E.g., if changing from ALLOW_ALL to CANCEL_OTHER, and there are workflows running,
// those workflows will not be cancelled. If changing from ALLOW_ALL to SKIP with
// workflows running, the running workflows will not cause the next action to be
// skipped.
// Note that this can be changed after a schedule has taken some actions,
// and some changes might produce unintuitive results. In general, the later
// policy overrides the earlier policy.
temporal.api.enums.v1.ScheduleOverlapPolicy overlap_policy = 1;

// Policy for catchups:
Expand Down Expand Up @@ -195,10 +270,11 @@ message ScheduleState {
// If true, do not take any actions based on the schedule spec.
bool paused = 2;

// If limited_actions is true, decrement remaining_actions after each action, and do
// not take any more scheduled actions if remaining_actions is zero. Actions may still
// be taken by explicit request. Skipped actions (due to overlap policy) do not count
// against remaining actions.
// If limited_actions is true, decrement remaining_actions after each
// action, and do not take any more scheduled actions if remaining_actions
// is zero. Actions may still be taken by explicit request (i.e. trigger
// immediately or backfill). Skipped actions (due to overlap policy) do not
// count against remaining actions.
bool limited_actions = 3;
int64 remaining_actions = 4;
}
Expand Down Expand Up @@ -258,8 +334,7 @@ message ScheduleInfo {
google.protobuf.Timestamp create_time = 6 [(gogoproto.stdtime) = true];
google.protobuf.Timestamp update_time = 7 [(gogoproto.stdtime) = true];

// Error for invalid schedule. If this is set, no actions will be taken.
string invalid_schedule_error = 8;
string invalid_schedule_error = 8 [deprecated = true];
}

message Schedule {
Expand All @@ -273,8 +348,7 @@ message Schedule {
// that's returned in ListSchedules.
message ScheduleListInfo {
// From spec:
// Some fields are too large/unimportant for the purpose of listing, so we'll clear them
// from this copy of spec: exclude_calendar, jitter, timezone_data.
// Some fields are dropped from this copy of spec: timezone_data
ScheduleSpec spec = 1;

// From action:
Expand Down
3 changes: 3 additions & 0 deletions temporal/api/workflowservice/v1/request_response.proto
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,9 @@ message DescribeScheduleRequest {
message DescribeScheduleResponse {
// The complete current schedule details. This may not match the schedule as
// created because:
// - some types of schedule specs may get compiled into others (e.g.
// CronString into StructuredCalendarSpec)
// - some unspecified fields may be replaced by defaults
// - some fields in the state are modified automatically
// - the schedule may have been modified by UpdateSchedule or PatchSchedule
temporal.api.schedule.v1.Schedule schedule = 1;
Expand Down