Filled remaining ActivityExecutionInfo fields #8857
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fills in the remaining fields for ActivityExecutionInfo in the activity execution describe API and enables a previously skipped test that was blocked by a chasm engine bug. The changes ensure that GET RPCs return all the necessary activity execution information fields.
Key Changes:
- Populated 15+ missing fields in the
ActivityExecutionInfostructure including execution duration, expiration time, retry policy, timeouts, search attributes, and user metadata - Re-enabled and updated the
TestActivityImmediatelyCancelled_WhenInScheduledStatetest that was previously skipped - Enhanced test assertions to use
cmp.Diffwithprotocmp.Transform()for comprehensive field validation while properly handling non-deterministic fields
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/standalone_activity_test.go | Added comprehensive test assertions using cmp.Diff for validating all ActivityExecutionInfo fields; re-enabled previously skipped cancellation test; added new test constants and default values for headers, search attributes, and user metadata |
| chasm/lib/activity/activity.go | Implemented complete field population in buildActivityExecutionInfo() including execution duration, expiration time, close time, next attempt schedule time, retry policy, timeouts, search attributes, task queue, and user metadata |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| completeTime := attempt.GetCompleteTime() | ||
| if interval != nil && interval.AsDuration() > 0 && completeTime != nil { | ||
| nextAttemptScheduleTime = timestamppb.New(completeTime.AsTime().Add(interval.AsDuration())) | ||
| } |
There was a problem hiding this comment.
I'm thinking it would be better to store this in the component data when we actually schedule the task, rather than trying to recapitulate the logic here.
The number we compute like this won't exactly match. And in the future we might add the ability to update the retry interval (right?) making it completely wrong.
There was a problem hiding this comment.
Passed in currentTime into recordFailedAttempt and added TODO to debate if we should persist this field, as we discussed.
| var closeTime *timestamppb.Timestamp | ||
| if a.LifecycleState(ctx) != chasm.LifecycleStateRunning { | ||
| closeTime = attempt.GetCompleteTime() | ||
| } |
There was a problem hiding this comment.
nit: could just trust attempt.GetCompleteTime() to be nil when appropriate?
There was a problem hiding this comment.
the attempt complete time could actually be nil on schedule-to-start, schedule-to-close timeouts as those timeouts don't overrwrite attemptState. Also same for cancelled and terminated.
dandavison
left a comment
dandavison
left a comment
There was a problem hiding this comment.
LGTM! We've discussed the execution duration calculation and agreed that it's not quite right yet, but are not blocking this PR on it.
|
|
||
| var closeTime *timestamppb.Timestamp | ||
| var executionDuration = durationpb.New(0) | ||
| if a.LifecycleState(ctx) != chasm.LifecycleStateRunning && attempt.GetCompleteTime() != nil { |
There was a problem hiding this comment.
not blocking at all but there is an API a.LifecycleState(ctx).IsClosed() so I guess we should use that.
| require.Equal(t, enumspb.ACTIVITY_EXECUTION_STATUS_CANCELED, info.GetStatus(), | ||
| "expected Canceled but is %s", info.GetStatus()) | ||
| require.Equal(t, "Test Cancellation", info.GetCanceledReason()) | ||
| require.Equal(t, info.GetExecutionDuration().AsDuration(), time.Duration(0)) // Canceled doesn't set attempt completion, thus expect 0 here |
There was a problem hiding this comment.
For Canceled here, and also for Terminated, I'm thinking that we do actually want to return a non-zero execution time. Workflow does I believe. Discussed offline and agreed to track this pending decisions on how to implement it.
There was a problem hiding this comment.
To be discussed offline
| RunId: startResp.RunId, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Greater(t, describeResp.GetInfo().GetExecutionDuration().AsDuration(), time.Duration(0)) // should have non-zero as attempts have been made |
There was a problem hiding this comment.
Same as comment elswehere -- we've discussed this and are tracking it. Whether or not the execution duration is returned shouldn't depend on whether attempts were made.
There was a problem hiding this comment.
To be discussed offline
## What changed? Filled remaining ActivityExecutionInfo fields. Enabled test that was blocked by chasm bug. Added schedule-to-start-timeout test. Test refactor. ## Why? We need to return all the fields for GET RPCs. Test cleanup/completion before merge to main. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s) --------- Co-authored-by: Dan Davison <dan.davison@temporal.io>
What changed?
Filled remaining ActivityExecutionInfo fields. Enabled test that was blocked by chasm bug. Added schedule-to-start-timeout test. Test refactor.
Why?
We need to return all the fields for GET RPCs. Test cleanup/completion before merge to main.
How did you test it?