Schedule API updates #230

dnr · 2022-08-25T06:31:43Z

What changed?

Add StructuredCalendarSpec and specify that CalendarSpec gets compiled into it.
Add CronString which is also compiled into StructuredCalendarSpec.
Update various comments that were out of date or needed clarification.

Why?
Make the API easier to consume by SDKs and enable more advanced UI features.

Breaking changes
exclude_calendar is deprecated

lorensr

Thanks for updating! I like the comment fields ☺️

temporal/api/schedule/v1/message.proto

lorensr · 2022-08-25T15:44:37Z

temporal/api/schedule/v1/message.proto

+    // 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;


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

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.

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?

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.

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

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 }.

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

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.

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?

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.

lorensr · 2022-08-25T16:18:57Z

temporal/api/schedule/v1/message.proto

    // 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.
+    // from this copy of spec: exclude_*, jitter, timezone_data.
    ScheduleSpec spec = 1;


I'd include exclude and jitter, just to avoid having to teach users they won't be there / avoid the confusion of:

having set exclude and jitter

not seeing them in the listinfo along with the other spec fields

thinking they created the schedule wrong

The problem is that we're currently passing this value through a search attribute, as json, which is a bit of a hack until we have mutable memos. Search attributes are limited to 2048 bytes. And now the structured calendar specs are going to be a bunch larger too. I can make it try to include them and only drop them if they're too big.

I realize this is pretty awful, but it's only temporary until we have mutable memos. And it's only in list output; with describe you always get the whole thing.

Shall we target mutable memo to be available in 1.18 and switch to use that before we make this more available?

If you think we can get it all ready in time...

lorensr · 2022-08-25T16:27:43Z

temporal/api/schedule/v1/message.proto

+    // Match years (2000-).
+    repeated Range year = 6;


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

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.

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 }]?

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.

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.

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.

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

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

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.

Leaving out is my favorite solution

yiminc · 2022-08-26T05:39:59Z

temporal/api/schedule/v1/message.proto

+// 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


I didn't know we support year. Is there legit use case where you need to specify year?
hmmm, maybe you only want to run for a curtain year.... never thought about this before.

It's a natural extension and other systems have it.

It lets you do stuff like make a list of holidays (that fall on different dates each year). Those can be used for either actions or excludes.

Not quite related: I was also considering adding another type of spec

message PrecalculatedSpec { repeated google.protobuf.Timestamp timestamp = 1; }

so we can support other types on the client-side without building them into the server, as long as you're willing to expand it all. Thoughts?

temporal/api/schedule/v1/message.proto

yiminc · 2022-08-26T05:48:34Z

temporal/api/schedule/v1/message.proto

+    // Match years (2000-).
+    repeated Range year = 6;


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.

yiminc · 2022-08-26T05:52:46Z

temporal/api/schedule/v1/message.proto

+    // Calendar-based specifications of times. Note that calendar and
+    // cron_string are input-only! They will be compiled into
+    // structured_calendar when the schedule is created or updated, so when you
+    // describe a schedule, you'll only see that.


You will only see that, what is that? (I think it is structured_calendar.)
// probably because i'm not native english speaker.

it is admittedly awkward wording

Suggested change

// describe a schedule, you'll only see that.

// describe a schedule, you won't see calendar or cron_string, you'll see

// only structured_calendar and interval.

yiminc · 2022-08-26T05:57:32Z

temporal/api/schedule/v1/message.proto

+    // 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;


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

yiminc · 2022-08-26T06:01:22Z

temporal/api/schedule/v1/message.proto

    // 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.
+    // from this copy of spec: exclude_*, jitter, timezone_data.
    ScheduleSpec spec = 1;


Shall we target mutable memo to be available in 1.18 and switch to use that before we make this more available?

bergundy · 2022-08-26T22:54:17Z

temporal/api/schedule/v1/message.proto

+// calendar, in a form that's easy to work with programmatically. Each field is
+// be one or more ranges.


Suggested change

// calendar, in a form that's easy to work with programmatically. Each field is

// be one or more ranges.

// calendar, in a form that's easy to work with programmatically. Each field can

// be one or more ranges.

dnr · 2022-09-08T06:43:30Z

temporal/api/schedule/v1/message.proto

+// @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 CronString {


One more thought on this one: to make it even simpler, this could not be a message at all, we could just have repeated string cron_string in ScheduleSpec. And then to support a comment we can just stuff it in the string: document that any text after # will be treated as a comment and copied to the comment field. That seems a little more in line with the idea of having a single string shorthand. Thoughts?

dnr · 2022-09-08T06:47:14Z

temporal/api/schedule/v1/message.proto

+    // Match years (2000-).
+    repeated Range year = 6;


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

lorensr · 2022-09-12T18:43:03Z

temporal/api/schedule/v1/message.proto

+    // See comment above for cron_string format.
+    repeated string cron_string = 8;


Could move the comment here?

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

temporal/api/schedule/v1/message.proto

dnr requested review from bergundy and lorensr August 25, 2022 06:31

dnr requested review from a team as code owners August 25, 2022 06:31

lorensr approved these changes Aug 25, 2022

View reviewed changes

dnr mentioned this pull request Aug 25, 2022

Schedule API temporalio/proposals#62

Merged

yiminc reviewed Aug 26, 2022

View reviewed changes

bergundy reviewed Aug 26, 2022

View reviewed changes

dnr force-pushed the sched5 branch from 7940852 to 4802f7c Compare August 31, 2022 18:09

yiminc approved these changes Aug 31, 2022

View reviewed changes

dnr added 5 commits September 7, 2022 23:24

Schedule API updates

373a9b2

more comment

76af104

@every

ece04c1

couple more tweaks from review comments

4ac012b

@every with phase

4d69920

dnr force-pushed the sched5 branch from 4802f7c to 4d69920 Compare September 8, 2022 06:37

dnr commented Sep 8, 2022

View reviewed changes

dnr mentioned this pull request Sep 8, 2022

Implement new schedule API features temporalio/temporal#3337

Merged

cron_string is just a string

0a9f5d8

lorensr reviewed Sep 12, 2022

View reviewed changes

temporal/api/schedule/v1/message.proto Show resolved Hide resolved

dnr added 3 commits September 12, 2022 12:56

cron_string comment

9b46b52

comment

15e4dab

missing year

c48113e

dnr merged commit ddf07ab into temporalio:master Sep 13, 2022

dnr deleted the sched5 branch September 13, 2022 00:23

		// 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

	// describe a schedule, you'll only see that.
	// describe a schedule, you won't see calendar or cron_string, you'll see
	// only structured_calendar and interval.

		// calendar, in a form that's easy to work with programmatically. Each field is
		// be one or more ranges.

		// See comment above for cron_string format.
		repeated string cron_string = 8;

Schedule API updates #230

Schedule API updates #230

Conversation

dnr commented Aug 25, 2022

lorensr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment