State machine for CHASM activities #8496
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| var _ chasm.StateMachine[activitypb.ActivityExecutionStatus] = (*Activity)(nil) | ||
|
|
||
| // State returns the current status of the activity. | ||
| func (a *Activity) State() activitypb.ActivityExecutionStatus { |
There was a problem hiding this comment.
Should we change the state machine abstraction to expect Status and SetStatus as it seems we are converging on that terminology?
There was a problem hiding this comment.
I'm not sure I like that. I feel that this makes sense:
- A state machine library has
State()andSetState() - For a given client of that library, "status" is the field name used for the state that is governed by the state machine library.
chasm/lib/activity/statemachine.go
Outdated
| if a.ActivityState == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Let it panic, this should be an invalid state.
| "go.temporal.io/server/chasm/lib/activity/gen/activitypb/v1" | ||
| ) | ||
|
|
||
| func TestValidTransitions(t *testing.T) { |
There was a problem hiding this comment.
I don't like this type of test, you are just testing configuration here and duplicating what is defined in the code being tested.
There was a problem hiding this comment.
Agree! Deleted test. We'll start having test coverage when we add the API method PRs.
| } | ||
| } | ||
|
|
||
| func TestInvalidTransitions(t *testing.T) { |
chasm/lib/activity/statemachine.go
Outdated
| a.ActivityState.Status = state | ||
| } | ||
|
|
||
| var ( |
There was a problem hiding this comment.
Can you get rid of this redundant nesting? By the time you get to the function implementation, you're three levels in.
chasm/lib/activity/statemachine.go
Outdated
| activitypb.ACTIVITY_EXECUTION_STATUS_UNSPECIFIED, | ||
| }, | ||
| activitypb.ACTIVITY_EXECUTION_STATUS_SCHEDULED, | ||
| func(_ chasm.MutableContext, _ *Activity, _ any) error { |
There was a problem hiding this comment.
It might be nice to change the transition function's signature to take SM as the first argument so you can pass a method pointer here.
There was a problem hiding this comment.
That seems like a good idea. Done.
chasm/lib/activity/statemachine.go
Outdated
| } | ||
|
|
||
| var ( | ||
| TransitionScheduled = chasm.NewTransition( |
There was a problem hiding this comment.
Should we add some docstrings to explain what this is?
There was a problem hiding this comment.
Yes good call, done.
dandavison
force-pushed
the
standalone-activity-state-machine
branch
from
e518ec7 to
984ed80
Compare
dandavison
changed the base branch from
standalone-activity
to
standalone-activity-post-rebase-fixups
dandavison
force-pushed
the
standalone-activity-state-machine
branch
from
984ed80 to
f722d1c
Compare
|
Ready for review again |
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
LGTM, but you'll want to rebase and not include the proto changes here.
dandavison
force-pushed
the
standalone-activity-state-machine
branch
from
193fa7f to
59cd168
Compare
Adds a state machine defining valid transitions for a CHASM activity. The state machine does not include transitions related to cancel and pause states: we can add those when we implement those features. Part of foundations for CHASM activity implementation / Standalone Activity. - Built (a unit test was written but not committed)
Adds a state machine defining valid transitions for a CHASM activity. The state machine does not include transitions related to cancel and pause states: we can add those when we implement those features. Part of foundations for CHASM activity implementation / Standalone Activity. - Built (a unit test was written but not committed)
What changed?
Adds a state machine defining valid transitions for a CHASM activity.
The state machine does not include transitions related to cancel and pause states: we can add those when we implement those features.
Why?
Part of foundations for CHASM activity implementation / Standalone Activity.
How did you test it?
Note
Introduces an Activity state machine with core execution transitions and updates the chasm Transition API to take (SM, Context, Event) parameter order.
chasm/lib/activity/statemachine.go):activity.Activityaschasm.StateMachine[activitypb.ActivityExecutionStatus]withState()/SetState().UNSPECIFIED -> SCHEDULEDSCHEDULED -> STARTEDSTARTED -> COMPLETED | FAILEDSCHEDULED | STARTED -> TERMINATED | TIMED_OUTchasm/statemachine.go):Transition.applysignature toapply(SM, MutableContext, E)and updatesNewTransition/Applyaccordingly.Written by Cursor Bugbot for commit 59cd168. This will update automatically on new commits. Configure here.