-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Post-merge changes to standalone-activity #8770
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
Post-merge changes to standalone-activity #8770
Conversation
- entity key => execution key - chasm foo.Get(ctx) does not return error - swap statemachine Apply args
The test expects a timeout for activity that is started but not completed. But the fix in #8723 ensures that we do schedule retries even when `schedule-to-close` is not set. So, add a retry policy that only permits one attempt.
chasm/lib/activity/fx.go
Outdated
| return []*chasm.RegistrableComponent{ | ||
| chasm.NewRegistrableComponent[*Activity]("activity"), | ||
| } | ||
| } |
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.
@bergundy I am not sure that what I've done here is the right fix. I need to read the code a bit more to understand fx and ChasmLibraryOptions better but I haven't had time yet. The problem being addressed is that after merging main, the FooByID tests fail with:
unknown chasm component type: *activity.Activity
It's related to the changes in #8704.
e.g. TestStandaloneActivityTestSuite/TestActivityCancelledByID fails without this.
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.
Commented above.
| ) (*workflowservice.UpdateActivityExecutionOptionsResponse, error) { | ||
| return nil, serviceerror.NewUnimplemented("temporary stub during Standalone Activity prototyping") | ||
| } | ||
|
|
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 at least partly Git's fault I believe. It gets confused about shared lines in merge conflict markers. I guess it would technically be possible to squash it into the merge commit but the code prior to this PR (after the merge commit) didn't compile anyway do maybe let's just leave it.
| info := activityResp.GetInfo() | ||
| require.Equal(t, enumspb.ACTIVITY_EXECUTION_STATUS_CANCELED, info.GetStatus()) | ||
| require.Equal(t, enumspb.ACTIVITY_EXECUTION_STATUS_CANCELED, info.GetStatus(), | ||
| "expected Canceled but is %s", info.GetStatus()) |
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.
Maybe there's a helper to make helpful error messages in this situation, or if not maybe we can create one. But for now these error messages are much more helpful than the integer codes in test output.
| StartToCloseTimeout: durationpb.New(1 * time.Second), | ||
| RetryPolicy: &commonpb.RetryPolicy{ | ||
| MaximumAttempts: 1, | ||
| }, |
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.
#8723 fixed things so that we do schedule retries when schedule-to-close is missing. But this test was expecting that the activity would fail on a start-to-close timeout. Hence it needed to be changed to limit to one attempt. This was actually broken on pre-merge standalone-activity.
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.
Put a comment in the code?
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.
Done
| } | ||
| func (a *Activity) PopulateRecordStartedResponse(ctx chasm.Context, key chasm.ExecutionKey, response *historyservice.RecordActivityTaskStartedResponse) error { | ||
| attempt := a.LastAttempt.Get(ctx) | ||
| lastHeartbeat, _ := a.LastHeartbeat.TryGet(ctx) |
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.
Curious why you switched to TryGet here?
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.
LastHeartbeat may be missing, and Get has been changed to panic on missing #8669
|
Seems like the misc. checks is failing with |
That GitHub Actions job is attempting to make a comparison between the PR branch and the branch the PR is merging into, to check something about whether the PR makes invalid proto changes. But in this case, the branch we're merging into doesn't compile at all (and is missing that file) so no such check can be made. We should just ignore that CI check for this PR. |
bergundy
left a comment
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.
Approved with comments, overall LGTM.
| } | ||
|
|
||
| if heartbeat == nil { | ||
| func (a *Activity) getLastHeartbeat(ctx chasm.MutableContext) *activitypb.ActivityHeartbeatState { |
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.
| func (a *Activity) getLastHeartbeat(ctx chasm.MutableContext) *activitypb.ActivityHeartbeatState { | |
| func (a *Activity) LastHeartbeatOrSetDefault(ctx chasm.MutableContext) *activitypb.ActivityHeartbeatState { |
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. I'm already changing the name in the heartbeat PR, so I'll leave it as-is here.
|
|
||
| requestData := a.RequestData.Get(ctx) | ||
| attempt := a.LastAttempt.Get(ctx) | ||
| heartbeat, _ := a.LastHeartbeat.TryGet(ctx) |
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: It's more idiomatic to use the ok boolean instead of doing a nil check but both are valid because the zero value of a pointer is nil.
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.
I don't quite understand this comment -- there's no nil check done in this function; we defer to the proto getters for that.
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.
Disregard, what you have is fine.
chasm/lib/activity/activity.go
Outdated
|
|
||
| // GetStore returns the store for the activity. If the store is not set as a field (e.g. standalone | ||
| // activities), it returns the activity itself. | ||
| func (a *Activity) GetStore(ctx chasm.MutableContext) ActivityStore { |
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.
It should be accessible with an immutable context, and please avoid using Get for getters in Go.
| func (a *Activity) GetStore(ctx chasm.MutableContext) ActivityStore { | |
| func (a *Activity) StoreOrSelf(ctx chasm.Context) ActivityStore { |
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.
Good call, done. I like that name -- not hiding implementation seems helpful in this case. (I'll remember not to use Get now...)
chasm/lib/activity/activity_tasks.go
Outdated
|
|
||
| valid := activity.Status == activitypb.ACTIVITY_EXECUTION_STATUS_STARTED && task.Attempt == attempt.Count | ||
| return valid, nil | ||
| return (activity.Status == activitypb.ACTIVITY_EXECUTION_STATUS_STARTED && |
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: I think the valid boolean helped readability here.
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.
Yes I don't disagree. Reinstated named valid variable.
chasm/lib/activity/fx.go
Outdated
|
|
||
| // componentOnlyLibrary only registers the Activity component. Used by frontend which needs to | ||
| // serialize ComponentRefs but doesn't need task executors. | ||
| type componentOnlyLibrary struct { |
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 can be the same library with two separate constructors. We need to iron out the library registration experience for CHASM on the frontend still but I am thinking that we would not redefine the libraries.
I would define this library in library.go and embed this in the full library struct.
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.
Thanks, done
chasm/lib/activity/fx.go
Outdated
| return []*chasm.RegistrableComponent{ | ||
| chasm.NewRegistrableComponent[*Activity]("activity"), | ||
| } | ||
| } |
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.
Commented above.
| } | ||
|
|
||
| testComponentTypeID, ok := s.mockShard.ChasmRegistry().ComponentIDFor(&testComponent{}) | ||
| s.Require().True(ok) |
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.
| s.Require().True(ok) | |
| s.True(ok) |
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.
Done
| StartToCloseTimeout: durationpb.New(1 * time.Second), | ||
| RetryPolicy: &commonpb.RetryPolicy{ | ||
| MaximumAttempts: 1, | ||
| }, |
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.
Put a comment in the code?
b778f07
into
standalone-activity-with-main-merged
This PR contains all changes needed to make tests compile and pass after merging main into standalone-activity.
The PR targets a branch named
standalone-activity-with-main-mergedwhich is the merge commit. That branch will become the newstandalone-activity.The changes from the merge commit itself (ce0afe4) are in the following diff:
git show ce0afe4 --cc
Note
Migrates CHASM to ExecutionKey and adds component_ref support for standalone activities, wiring new activity-execution APIs through frontend/history/matching and clients, plus related proto, notifier, and engine updates.
EntityKey→ExecutionKey; update engine interfaces (NotifyExecution, polling), transition-history comparisons, and notifier/subscription to use execution keys.NewExecutionandExecutionKeythroughout; introduceStoreOrSelf; simplify getters (TryGet), heartbeat/outcome handling.business_id.component_ref; History adds execution notifier and polling logic with execution keys.component_reffor standalone activity tasks; implement workflow pause/unpause; add scheduler client wiring.RecordActivityTaskStartedRequestaddstask_dispatch_revision_numberandcomponent_ref(field 14); generated code updated.go.temporal.io/apidependency.ExecutionKey, new fields, and assertions.Written by Cursor Bugbot for commit 92907df. This will update automatically on new commits. Configure here.