-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Scheduled Actions] CHASM scheduled actions visibility support #8292
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
fc1324f
563f68c
e74440b
39b05aa
76e46b7
70da0df
e7de996
f2fc202
0d48e52
f7cadcd
1714b8c
40eeba0
514774e
45263b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ type ( | |
| Config *Config | ||
| MetricsHandler metrics.Handler | ||
| BaseLogger log.Logger | ||
| SpecProcessor SpecProcessor | ||
|
|
||
| HistoryClient resource.HistoryClient | ||
|
|
||
|
|
@@ -540,9 +541,14 @@ func (e *InvokerExecuteTaskExecutor) startWorkflow( | |
| return nil, err | ||
| } | ||
|
|
||
| // realTime may be slightly past the time of the action's first scheduled WFT. | ||
| realTime := time.Now() | ||
| desiredTime := start.ActualTime | ||
| e.MetricsHandler.Timer(metrics.ScheduleActionDelay.Name()).Record(realTime.Sub(desiredTime.AsTime())) | ||
|
|
||
| return &schedulepb.ScheduleActionResult{ | ||
| ScheduleTime: start.ActualTime, | ||
| ActualTime: timestamppb.New(time.Now()), | ||
| ScheduleTime: desiredTime, | ||
| ActualTime: timestamppb.New(realTime), | ||
|
||
| StartWorkflowResult: &commonpb.WorkflowExecution{ | ||
| WorkflowId: workflowID, | ||
| RunId: result.RunId, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,9 @@ message SchedulerState { | |
| message GeneratorState { | ||
| // High water mark. | ||
| google.protobuf.Timestamp last_processed_time = 3; | ||
|
|
||
| // A list of upcoming times an action will be triggered. | ||
| repeated google.protobuf.Timestamp future_action_times = 4; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be persisted? Can't it be calculated on the fly when calculating the memo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it can't be, because the Memo is computed in the context of a component (not a task executor), and therefore it doesn't have the wired-in dependencies that computing this relies upon (SpecProcessor). |
||
| } | ||
|
|
||
| // CHASM scheduler's Invoker internal state. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a non retryable error to ensure this task doesn't stay in the queue?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it should - I updated it here, I also did a pass of the rest of the scheduler package for similar changes (
will open a separate PR with those momentarilyactually it's small enough I'm going to include it here).