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 #62

Merged
merged 27 commits into from
Sep 27, 2022
Merged

Schedule API #62

merged 27 commits into from
Sep 27, 2022

Conversation

lorensr
Copy link
Contributor

@lorensr lorensr commented Jul 29, 2022

Re: temporalio/features#73

rendered

Some things I could add if helpful:

  • types
  • comments with the corresponding gRPC fields

I took minor liberties in choosing different terms for fields compared to gRPC. Downside is having a difference between SDK and tctl. But if we like SDK terms, maybe we submit a PR to tctl so it matches.

Also flattened / removed the schedule from { schedule: { spec, action, policies, state } }.

Note: you can hide comments for easier reading

image

@lorensr lorensr requested a review from bergundy July 29, 2022 06:22
searchAttributes,
})

const scheduleDescription = await schedule.describe()
Copy link
Member

Choose a reason for hiding this comment

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

What does this contain and what's the response type? Are we doing the same as in workflow.describe where we wrap the raw grpc message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.describe() doesn't take anything. Yeah, a ScheduleDescription wrapper that has decoded payloads and base64-encoded binary. Here's the message, I'd flatten schedule to match input.

message DescribeScheduleResponse {
    // The complete current schedule details. This may not match the schedule as
    // created because:
    // - 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;
    // Extra schedule state info.
    temporal.api.schedule.v1.ScheduleInfo info = 2;
    // The memo and search attributes that the schedule was created with.
    temporal.api.common.v1.Memo memo = 3;
    temporal.api.common.v1.SearchAttributes search_attributes = 4;

    // This value can be passed back to UpdateSchedule to ensure that the
    // schedule was not modified between a Describe and an Update, which could
    // lead to lost updates and other confusion.
    bytes conflict_token = 5;
}

Copy link
Member

Choose a reason for hiding this comment

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

One thing that could also be super useful is to be able to duplicate a schedule for a different action

Copy link
Contributor Author

@lorensr lorensr Aug 3, 2022

Choose a reason for hiding this comment

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

How's this?

const { spec, policies, state } = await schedule.describe()
const schedule = await client.create({ 
  id: 'foo', 
  spec, 
  policies,
  state,
  action: {
    startWorkflow: {
      workflowId: 'wf-biz-id',
      type: differentWorkflow,
    },
  }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ is now a lot of fields. Alternative:

const schedule = await schedule.describe()
const options = schedule.copy({ 
  id: 'foo',
  action: {
    startWorkflow: {
      workflowId: 'wf-biz-id',
      type: differentWorkflow,
    },
  }
})
client.create(options)

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Overall this LGTM.

Could use more comments to give more context, I had to make several assumptions when reading this proposal.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

I can't really give a reasonable review of this proposal without all of the types present. I would love this proposal presented as a bunch of declaration files, heh.

But if that's gonna be done at implementation time, that's fine, but there isn't enough detail here for me atm.

@cretz
Copy link
Member

cretz commented Aug 3, 2022

I would still like to see the full TypeScript definitions. But if that's at implementation time, ok, I will wait to review there.

@temporalio temporalio deleted a comment from bergundy Aug 17, 2022
Comment on lines 626 to 642
/**
* @default `[{ start: 'JANUARY' , end: 'DECEMBER' }]`
*/
month?: MonthSpecDescription;

/**
* Use full years, like `2030`
*
* @default All possible values
*/
year?: NumberSpecDescription;

/**
* @default `[{ start: 'SUNDAY' , end: 'SATURDAY' }]`
*/
dayOfWeek?: DaySpecDescription;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dnr my understanding from Roey is that DescribeScheduleResponse will contain Ranges? If yes, what will the default year and dayOfWeek be?

Copy link
Member

Choose a reason for hiding this comment

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

You saw temporalio/api#230, just clarifying here:

The intention is users can supply a calendar-type spec in one of three ways: single cron string, labeled cron string, structured proto. The first two will be validated and converted to structured proto on the frontend, so when you get a schedule back, you'll only see structured proto.

If we add ical rrule support, that may also get compiled into the structured proto, or it might need to be a third major type (calendar, interval, rrule) if it's not expressible that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's a labeled cron string?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, that's what I think of CalendarSpec as in my head

@lorensr lorensr merged commit 1aee30e into master Sep 27, 2022
@lorensr
Copy link
Contributor Author

lorensr commented Sep 27, 2022

Now that's what you call a saga 😊. I believe all issues raised so far have been either resolved or left as TODO comments for future consideration. If any future issues come up, feel free to submit an issue or PR. This doc now lives at: https://github.com/temporalio/proposals/blob/master/typescript/schedules.md

The SDKs will start implementing the Schedule API. Some things in this doc are TypeScript-specific, and different languages have different conventions or expectations, so there will be some differences between SDKs. Also, we may find that parts of this doc are suboptimal as we learn through the implementation process, so we may modify this doc. The Schedule API will be marked experimental (i.e. may have breaking changes) until it has been released in at least two SDKs and has had user testing/feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants