-
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 2 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
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,13 +1,19 @@ | ||||||||
| package scheduler | ||||||||
|
|
||||||||
| import ( | ||||||||
| "bytes" | ||||||||
| "fmt" | ||||||||
| "time" | ||||||||
|
|
||||||||
| commonpb "go.temporal.io/api/common/v1" | ||||||||
| enumspb "go.temporal.io/api/enums/v1" | ||||||||
| schedulepb "go.temporal.io/api/schedule/v1" | ||||||||
| "go.temporal.io/server/chasm" | ||||||||
| "go.temporal.io/server/chasm/lib/scheduler/gen/schedulerpb/v1" | ||||||||
| "go.temporal.io/server/common" | ||||||||
| "go.temporal.io/server/common/payload" | ||||||||
| "go.temporal.io/server/common/primitives/timestamp" | ||||||||
| "go.temporal.io/server/common/searchattribute" | ||||||||
| "go.temporal.io/server/common/util" | ||||||||
| "go.temporal.io/server/service/worker/scheduler" | ||||||||
| "google.golang.org/protobuf/types/known/timestamppb" | ||||||||
|
|
@@ -29,6 +35,8 @@ type Scheduler struct { | |||||||
| Invoker chasm.Field[*Invoker] | ||||||||
| Backfillers chasm.Map[string, *Backfiller] // Backfill ID => *Backfiller | ||||||||
|
|
||||||||
| Visibility chasm.Field[*chasm.Visibility] | ||||||||
|
|
||||||||
| // Locally-cached state, invalidated whenever cacheConflictToken != ConflictToken. | ||||||||
| cacheConflictToken int64 | ||||||||
| compiledSpec *scheduler.CompiledSpec // compiledSpec is only ever replaced whole, not mutated. | ||||||||
|
|
@@ -37,6 +45,12 @@ type Scheduler struct { | |||||||
| const ( | ||||||||
| // How many recent actions to keep on the Info.RecentActions list. | ||||||||
| recentActionCount = 10 | ||||||||
|
|
||||||||
| // Item limit per spec field on the ScheduleInfo memo. | ||||||||
| listInfoSpecFieldLimit = 10 | ||||||||
|
|
||||||||
| // Field in which the schedule's memo is stored. | ||||||||
| visibilityMemoFieldInfo = "ScheduleInfo" | ||||||||
| ) | ||||||||
|
|
||||||||
| // NewScheduler returns an initialized CHASM scheduler root component. | ||||||||
|
|
@@ -66,10 +80,14 @@ func NewScheduler( | |||||||
| } | ||||||||
|
|
||||||||
| invoker := NewInvoker(ctx, sched) | ||||||||
| generator := NewGenerator(ctx, sched, invoker) | ||||||||
| sched.Invoker = chasm.NewComponentField(ctx, invoker) | ||||||||
|
|
||||||||
| generator := NewGenerator(ctx, sched, invoker) | ||||||||
| sched.Generator = chasm.NewComponentField(ctx, generator) | ||||||||
|
|
||||||||
| visibility := chasm.NewVisibility(ctx) | ||||||||
| sched.Visibility = chasm.NewComponentField(ctx, visibility) | ||||||||
|
|
||||||||
| return sched | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -290,3 +308,165 @@ func (s *Scheduler) recordActionResult(result *schedulerActionResult) { | |||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // UpdateVisibility updates the schedule's visibility record, including memo | ||||||||
| // and search attributes. customSearchAttributes is optional, and custom search | ||||||||
| // attributes will be left as-is with it unset. | ||||||||
| // | ||||||||
| // See mergeCustomSearchAttributes for how custom search attributes are merged. | ||||||||
| func (s *Scheduler) UpdateVisibility( | ||||||||
|
||||||||
| ctx chasm.MutableContext, | ||||||||
| specProcessor SpecProcessor, | ||||||||
| customSearchAttributes *commonpb.SearchAttributes, | ||||||||
| ) error { | ||||||||
| needsTask := false // Set to true if we need to write anything to Visibility. | ||||||||
| visibility, err := s.Visibility.Get(ctx) | ||||||||
|
||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| // Update the schedule's search attributes. This includes both Temporal-managed | ||||||||
| // fields (paused state), as well as upserts for customer-specified fields. | ||||||||
| upsertAttrs := make(map[string]any) | ||||||||
| currentAttrs, err := visibility.GetSearchAttributes(ctx) | ||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| // Only attempt to merge additional search attributes if any are given, otherwise | ||||||||
| // we'll unset existing attributes. | ||||||||
| if customSearchAttributes != nil && | ||||||||
| len(customSearchAttributes.GetIndexedFields()) > 0 { | ||||||||
|
||||||||
| if customSearchAttributes != nil && | |
| len(customSearchAttributes.GetIndexedFields()) > 0 { | |
| if len(customSearchAttributes.GetIndexedFields()) > 0 { |
Outdated
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.
This is exactly why I didn't want components to use imperative logic to update their search attributes. This logic should be part of the framework.
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.
If we can afford it timeline-wise I'd much prefer if we coded up a better abstraction in the framework.
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.
In general I agree. Also acknowledging that imperative logic means component author needs to remember update those attributes.
We discussed about the field tag approach which addresses this concern and also the SA registration problem but decided to do that as a follow up to unblock the Scheduler migration and deliver value sooner. It's more of a ROI & priority discussion for the entire OSS team.
Outdated
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.
nit: can use the generic chasm.GetSearchAttribute() method here as well and you don't need to do the decoding.
Outdated
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.
This is more like a "refresh visibility" approach. Do we have a like PauseSchedule method where we can update the field?
Outdated
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.
nit: can use the generic chasm.GetMemo() function which returns a strongly typed value instead of Payload
Outdated
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.
Proto marshal is not deterministic by default, this comparison will give you false negatives. You will want to unmarshal and use proto.Equal instead.
Another reason why component authors shouldn't implement all of this logic.
Outdated
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.
UpsertMemo will perform the encoding to Payload type, should just use newInfo here.
Outdated
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.
This is unnecessary, Visibility component automatically generates a task when it's updated.
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).