add RESET_REQUESTED state to standalone activity#10364
Conversation
1595173 to
5cacf1e
Compare
dandavison
left a comment
There was a problem hiding this comment.
Did a pass. Looks good; a few things suggested in comments.
| func(a *Activity, ctx chasm.MutableContext, event completeEvent) error { | ||
| return a.StoreOrSelf(ctx).RecordCompleted(ctx, func(ctx chasm.MutableContext) error { | ||
| a.ActivityReset = false | ||
| a.PauseState = nil |
There was a problem hiding this comment.
We shouldn't clear PauseState because no logic should be expressed in terms of PauseState and it's cleaner just to let it be, rather than have the code imply that something would go wrong if it's not cleared in all the right places.
Same comment applies to all the places below where PauseState is cleared.
This will make it like CancelState and TerminateState. As I mentioned in other comments, I'd be open to calling it LastPauseState to make it clear that this one might be non-current unlike CancelState and TerminateState which I believe are always current (but lmk if that's wrong).
| CancelRequested: a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_CANCEL_REQUESTED, | ||
| ActivityPaused: a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_PAUSE_REQUESTED, | ||
| ActivityReset: a.ActivityReset, | ||
| ActivityPaused: (a.PauseState != nil || a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_PAUSE_REQUESTED) && a.Status != activitypb.ACTIVITY_EXECUTION_STATUS_CANCEL_REQUESTED, |
There was a problem hiding this comment.
With the change to PauseRequested we should not be using PauseState as a flag any longer. And IMO we shouldn't ever clear it: just let it be set without clearing, like CancelState and TerminateState. We could call it LastPauseState to emphasize that it isn't necessarily current -- does that rename appeal to you?
I think we'll need to store ResetKeepPaused on the activity state.
So this line should look like
ActivityPaused: a.Status == PAUSE_REQUESTED || (a.Status == RESET_REQUESTED && a.ResetKeepPaused)
| if !keepPaused { | ||
| // Clear PauseState now so the deferred reset can dispatch on retry without being | ||
| // blocked by the validator (which drops dispatch tasks when PauseState != nil). | ||
| a.PauseState = nil |
There was a problem hiding this comment.
The task validators no longer gate on PauseState so this is not needed.
| return a.StoreOrSelf(ctx).RecordCompleted(ctx, func(ctx chasm.MutableContext) error { | ||
| req := event.req.GetFailedRequest() | ||
| a.ActivityReset = false | ||
| a.PauseState = nil |
There was a problem hiding this comment.
No need to clear PauseState (see comment above)
| RequestId: event.request.RequestID, | ||
| } | ||
| a.ActivityReset = false | ||
| a.PauseState = nil |
There was a problem hiding this comment.
No need to clear PauseState (see comment above)
| }, | ||
| } | ||
| a.ActivityReset = false | ||
| a.PauseState = nil |
There was a problem hiding this comment.
No need to clear PauseState (see comment above)
| } | ||
|
|
||
| a.ActivityReset = false | ||
| a.PauseState = nil |
There was a problem hiding this comment.
No need to clear PauseState (see comment above)
| // keepPaused on a paused (PAUSE_REQUESTED) activity preserves the pause: when the worker | ||
| // yields the activity lands back in PAUSED rather than SCHEDULED. Recorded as an explicit | ||
| // flag so no logic has to gate on the descriptive LastPauseState field. | ||
| a.ResetKeepPaused = keepPaused && a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_PAUSE_REQUESTED |
There was a problem hiding this comment.
The second part of the comment will be hard for a future reader to understand.
| // keepPaused on a paused (PAUSE_REQUESTED) activity preserves the pause: when the worker | |
| // yields the activity lands back in PAUSED rather than SCHEDULED. Recorded as an explicit | |
| // flag so no logic has to gate on the descriptive LastPauseState field. | |
| a.ResetKeepPaused = keepPaused && a.Status == activitypb.ACTIVITY_EXECUTION_STATUS_PAUSE_REQUESTED | |
| // keepPaused on a paused (PAUSE_REQUESTED) activity preserves the pause: when the worker | |
| // yields the activity lands back in PAUSED rather than SCHEDULED. |
| // keepPaused=true while the activity was in PAUSE_REQUESTED). The failed attempt is recorded, the | ||
| // attempt count is reset to 1, and no dispatch task is emitted — the activity stays paused until | ||
| // an explicit unpause. | ||
| var TransitionResetAttemptFailedToPaused = chasm.NewTransition( |
There was a problem hiding this comment.
We might want to look again over our transition names to see if we're happy with how consistent they are (I used While but after some back and forth, and I know yours have to distinguish between the two destinations). But we don't have to do that now since tit has no impact on persisted data or public exposure -- can fiddle with them at any time in the future.
## What changed? Replace the flagged base `RESET` state with a `RESET_REQUESTED` state. This takes fuller advantage of the CHASM state transition functionality. ## Why? Reduces the amount of special casing code and replaces this with an additional state (`RESET_REQUESTED`) and state transitions to handle the case where a `STARTED` activity is reset and has to be delivered on the next heartbeat or when an activity completes the attempt. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [X] covered by existing tests - [ ] added new unit test(s) - [X] added new functional test(s) ## Potential risks NA, unreleased code.
## What changed? Replace the flagged base `RESET` state with a `RESET_REQUESTED` state. This takes fuller advantage of the CHASM state transition functionality. ## Why? Reduces the amount of special casing code and replaces this with an additional state (`RESET_REQUESTED`) and state transitions to handle the case where a `STARTED` activity is reset and has to be delivered on the next heartbeat or when an activity completes the attempt. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [X] covered by existing tests - [ ] added new unit test(s) - [X] added new functional test(s) ## Potential risks NA, unreleased code.
Implements Pause, Unpause, Reset, and UpdateActivityExecutionOptions RPCs for standalone CHASM activities. Adds matching frontend and history service handlers in chasm/lib/activity that dispatch to either the workflow-activity path or the standalone-activity path based on the presence of a workflow ID. Also adds PAUSE_REQUESTED and RESET_REQUESTED state machine states for standalone activities, the original_options snapshot for restore-on-reset semantics, the activity-options merge helpers (common/activityoptions), and the supporting proto schema changes (start_request_id, sdk_name, sdk_version, etc.). Squash of the 29 commits previously on this branch. Original PRs: #9693 RPC boilerplate and code generation #9850 Implement UpdateActivityExecutionOptions #9851 Implement Pause/UnpauseActivityExecution for SAA #9852 Implement ResetActivityExecution for SAA #10001 Pause returns FailedPrecondition when already paused #10025 Validator function fixes #10265 Replace PauseState flag with PAUSE_REQUESTED state #10364 Add RESET_REQUESTED state to standalone activity
Implements Pause, Unpause, Reset, and UpdateActivityExecutionOptions RPCs for standalone CHASM activities. Adds matching frontend and history service handlers in chasm/lib/activity that dispatch to either the workflow-activity path or the standalone-activity path based on the presence of a workflow ID. Also adds PAUSE_REQUESTED and RESET_REQUESTED state machine states for standalone activities, the original_options snapshot for restore-on-reset semantics, the activity-options merge helpers (common/activityoptions), and the supporting proto schema changes (start_request_id, sdk_name, sdk_version, etc.). Squash of the 29 commits previously on this branch. Original PRs: #9693 RPC boilerplate and code generation #9850 Implement UpdateActivityExecutionOptions #9851 Implement Pause/UnpauseActivityExecution for SAA #9852 Implement ResetActivityExecution for SAA #10001 Pause returns FailedPrecondition when already paused #10025 Validator function fixes #10265 Replace PauseState flag with PAUSE_REQUESTED state #10364 Add RESET_REQUESTED state to standalone activity
Implements Pause, Unpause, Reset, and UpdateActivityExecutionOptions RPCs for standalone CHASM activities. Adds matching frontend and history service handlers in chasm/lib/activity that dispatch to either the workflow-activity path or the standalone-activity path based on the presence of a workflow ID. Also adds PAUSE_REQUESTED and RESET_REQUESTED state machine states for standalone activities, the original_options snapshot for restore-on-reset semantics, the activity-options merge helpers (common/activityoptions), and the supporting proto schema changes (start_request_id, sdk_name, sdk_version, etc.). Squash of the 29 commits previously on this branch. Original PRs: #9693 RPC boilerplate and code generation #9850 Implement UpdateActivityExecutionOptions #9851 Implement Pause/UnpauseActivityExecution for SAA #9852 Implement ResetActivityExecution for SAA #10001 Pause returns FailedPrecondition when already paused #10025 Validator function fixes #10265 Replace PauseState flag with PAUSE_REQUESTED state #10364 Add RESET_REQUESTED state to standalone activity
Implements Pause, Unpause, Reset, and UpdateActivityExecutionOptions RPCs for standalone CHASM activities. Adds matching frontend and history service handlers in chasm/lib/activity that dispatch to either the workflow-activity path or the standalone-activity path based on the presence of a workflow ID. Also adds PAUSE_REQUESTED and RESET_REQUESTED state machine states for standalone activities, the original_options snapshot for restore-on-reset semantics, the activity-options merge helpers (common/activityoptions), and the supporting proto schema changes (start_request_id, sdk_name, sdk_version, etc.). Squash of the 29 commits previously on this branch. Original PRs: #9693 RPC boilerplate and code generation #9850 Implement UpdateActivityExecutionOptions #9851 Implement Pause/UnpauseActivityExecution for SAA #9852 Implement ResetActivityExecution for SAA #10001 Pause returns FailedPrecondition when already paused #10025 Validator function fixes #10265 Replace PauseState flag with PAUSE_REQUESTED state #10364 Add RESET_REQUESTED state to standalone activity
Implements Pause, Unpause, Reset, and UpdateActivityExecutionOptions RPCs for standalone CHASM activities. Adds matching frontend and history service handlers in chasm/lib/activity that dispatch to either the workflow-activity path or the standalone-activity path based on the presence of a workflow ID. Also adds PAUSE_REQUESTED and RESET_REQUESTED state machine states for standalone activities, the original_options snapshot for restore-on-reset semantics, the activity-options merge helpers (common/activityoptions), and the supporting proto schema changes (start_request_id, sdk_name, sdk_version, etc.). Squash of the 29 commits previously on this branch. Original PRs: #9693 RPC boilerplate and code generation #9850 Implement UpdateActivityExecutionOptions #9851 Implement Pause/UnpauseActivityExecution for SAA #9852 Implement ResetActivityExecution for SAA #10001 Pause returns FailedPrecondition when already paused #10025 Validator function fixes #10265 Replace PauseState flag with PAUSE_REQUESTED state #10364 Add RESET_REQUESTED state to standalone activity
What changed?
Replace the flagged base
RESETstate with aRESET_REQUESTEDstate. This takes fuller advantage of the CHASM state transition functionality.Why?
Reduces the amount of special casing code and replaces this with an additional state (
RESET_REQUESTED) and state transitions to handle the case where aSTARTEDactivity is reset and has to be delivered on the next heartbeat or when an activity completes the attempt.How did you test it?
Potential risks
NA, unreleased code.