Add schedule API #943
Add schedule API #943
Conversation
internal/client.go
Outdated
| WorkflowType WorkflowType | ||
|
|
||
| // Args - Arguments of the workflow | ||
| Args *commonpb.Payloads |
There was a problem hiding this comment.
@cretz we talked about using encoded values for the description of the workflow arguments but I found they couldn't be converted back to payload for the Update function.
There was a problem hiding this comment.
This is probably fine. I would have also supported a new concept called *converter.RawValue or something that carried the payload and data converter, but that may be a bit much.
internal/client.go
Outdated
| } | ||
|
|
||
| // ScheduleUpdate describes the desired new schedule from ScheduleHandle.Update. | ||
| ScheduleUpdate struct { |
There was a problem hiding this comment.
I added another struct around Schedule incase we wanted to allow updating other parts of a schedule in the future. I was specifically thinking the Memo and SearchAttributes
internal/client.go
Outdated
| } | ||
|
|
||
| // ScheduleUpdateOptions configure the parameters for updating a schedule. | ||
| ScheduleUpdateOptions struct{} |
There was a problem hiding this comment.
This is empty right now because the server does not return a failure on conflict token mismatch. Once that is fixed we can add an option to fail on that case and retry.
internal/client.go
Outdated
| Action ScheduleAction | ||
|
|
||
| // Schedule - Describes when Actions should be taken. | ||
| Spec *ScheduleSpec |
There was a problem hiding this comment.
just curious, why pointers for some and not others?
There was a problem hiding this comment.
If you mean "why not a pointer for Action", that is because it is an interface
Quinn-With-Two-Ns
force-pushed
the
schedules
branch
from
248f69e to
332b7db
Compare
| // Start span and write to header | ||
| span, ctx, err := t.root.startSpanFromContext(ctx, &TracerStartSpanOptions{ | ||
| Operation: "CreateSchedule", | ||
| Name: in.Options.ID, |
There was a problem hiding this comment.
schedules don't really have a concept of a name so I used the ID.
| return recentActions | ||
| } | ||
|
|
||
| func encodeScheduleWorklowArgs(dc converter.DataConverter, args []interface{}) (*commonpb.Payloads, error) { |
There was a problem hiding this comment.
@cretz I did consider modifying the existing functions instead of creating new ones, but I didn't want to leak this visible change out of the experimental API
| func encodeScheduleWorklowArgs(dc converter.DataConverter, args []interface{}) (*commonpb.Payloads, error) { | ||
| payloads := make([]*commonpb.Payload, len(args)) | ||
| for i, arg := range args { | ||
| // arg is already encoded |
There was a problem hiding this comment.
I support a mix of serialized and not serialized Args. I found it was nice when writing integration tests.
There was a problem hiding this comment.
This is probably ok, but technically this is a bit different than elsewhere where we'd call DataConverter.ToPayloads (plural) w/ all args if they wanted to, say, condense them or something. But most people treat that as the same as calling each individually. And I think we're inconsistent here anyways. So you're good.
You may want to document on args, search attributes, and memos that we just pass through payloads on create/update.
| payloads := make([]*commonpb.Payload, len(args)) | ||
| for i, arg := range args { | ||
| // arg is already encoded | ||
| if enc, ok := arg.(*commonpb.Payload); ok { |
There was a problem hiding this comment.
I chose to use *commonpb.Payload here instead of EncodedValues because it limited the amount of possible user error, was consistent with current memo and search attribute usage in other parts of the SDK
There was a problem hiding this comment.
That's a fairly recent change to make those that way IIRC, but I'm fine w/ it.
| State: &schedulepb.ScheduleState{ | ||
| Notes: in.Options.Note, | ||
| Paused: in.Options.Paused, | ||
| LimitedActions: in.Options.RemainingActions != 0, |
There was a problem hiding this comment.
In describe and update, the value of RemainingActions has to be interpreted in regard of LimitedActions. Assuming LimitedActions == true, then RemainingActions == 0 would indicate that the schedule can't happens anymore (ie. it is implicitely paused), until remainingActions is reset to a non zero value.
Having only RemainingActions in create might make this semantic harder to understand for users. I'd suggest either adding LimitedActions to create options, or using a value other than 0 (ie -1) for "don't limit on action count", and using that same semantic in Describe and Update.
What was changed
Add schedule API
Why?
Checklist
Closes High level API for schedules #871
How was this tested: