Conversation
44508ad to
0dba4dd
Compare
9eb5bc4 to
fc21cb8
Compare
…er to 1.5.2-standalone-activity-server
cretz
left a comment
There was a problem hiding this comment.
Looks great, most of my concerns are minor
client/client.go
Outdated
| // Returns an ActivityExecutionAlreadyStarted error if an activity with the same ID already exists | ||
| // in this namespace, unless permitted by the specified ID conflict policy. | ||
| // | ||
| // ActivityHandle has the following methods: |
There was a problem hiding this comment.
Not sure we should have to keep an up-to-date list of the return type's interface methods here
There was a problem hiding this comment.
We do this for ExecuteWorkflow, I think it's fine to keep it for consistency
There was a problem hiding this comment.
ActivityHandle is not the same as WorkflowRun. When we add new operations for workflows, we make top-level methods, so we don't have to update that list.
Regardless, this is a perfect example of what I'm talking about. That list is missing GetWithOptions, but I wasn't around to predict that would happen then like I am now :-) It shouldn't be there either.
There was a problem hiding this comment.
yeah, that's fair, one less thing to keep synchronized, removed
| // | ||
| // Exposed as: [go.temporal.io/sdk/interceptor.ClientExecuteActivityInput] | ||
| type ClientExecuteActivityInput struct { | ||
| Options *ClientStartActivityOptions |
There was a problem hiding this comment.
FWIW, it is arguable that this should not be a pointer, but since this matches other interceptor calls, no prob
internal/internal_activity_client.go
Outdated
| if e, ok := err.(*serviceerror.ActivityExecutionAlreadyStarted); ok && | ||
| in.Options.ActivityIDConflictPolicy == enumspb.ACTIVITY_ID_CONFLICT_POLICY_USE_EXISTING { |
There was a problem hiding this comment.
This conditional doesn't really make sense. If the conflict policy was use existing, you'd never get this error. Rather, we need to confirm if, like workflows, we default to already-exists returning the handle (w/ WorkflowExecutionErrorWhenAlreadyStarted-like option to change it) or not.
There was a problem hiding this comment.
Thanks for the callout, added ActivityExecutionErrorWhenAlreadyStarted to mirror workflow
There was a problem hiding this comment.
This works for me, though may want to confirm with others on the standalone activity team they have no concerns
There was a problem hiding this comment.
Discussed offline, removing ActivityExecutionErrorWhenAlreadyStarted and instead relying on conflict policy, as that was added into workflows after WorkflowExecutionErrorWhenAlreadyStarted, we're diverging from mirroring workflows, but conflict policy should be sufficient here.
internal/internal_activity_client.go
Outdated
| RunId: in.RunID, | ||
| } | ||
|
|
||
| resp, err := w.client.WorkflowService().PollActivityExecution(grpcCtx, request) |
There was a problem hiding this comment.
I think this needs to loop while there is no outcome
There was a problem hiding this comment.
added a loop for resp.GetOutcome() != nil
…che results, clarify CompleteActivity* docs, add ActivityExecutionErrorWhenAlreadyStarted
cretz
left a comment
There was a problem hiding this comment.
Looks good, I have nothing blocking
client/client.go
Outdated
| // Returns an ActivityExecutionAlreadyStarted error if an activity with the same ID already exists | ||
| // in this namespace, unless permitted by the specified ID conflict policy. | ||
| // | ||
| // ActivityHandle has the following methods: |
There was a problem hiding this comment.
ActivityHandle is not the same as WorkflowRun. When we add new operations for workflows, we make top-level methods, so we don't have to update that list.
Regardless, this is a perfect example of what I'm talking about. That list is missing GetWithOptions, but I wasn't around to predict that would happen then like I am now :-) It shouldn't be there either.
| // NOTE: Experimental | ||
| // | ||
| // Exposed as: [go.temporal.io/sdk/client.ListActivitiesResult] | ||
| ClientListActivitiesResult struct { |
There was a problem hiding this comment.
Pedantic, but could be called ClientActivityList, but meh, this is fine
internal/internal_activity_client.go
Outdated
| if e, ok := err.(*serviceerror.ActivityExecutionAlreadyStarted); ok && | ||
| in.Options.ActivityIDConflictPolicy == enumspb.ACTIVITY_ID_CONFLICT_POLICY_USE_EXISTING { |
There was a problem hiding this comment.
This works for me, though may want to confirm with others on the standalone activity team they have no concerns
What was changed
TestActivityEnvironmentto support executing activities as standalone.testActivityToken(containing activityID and runID) are used to keep track of activities, instead of justactivityID. Most users should not be affected, only if you're manually constructing task tokens in your tests.Checklist
Closes Support standalone activities #2124
How was this tested:
New integration tests:
TestIntegrationSuite/TestExecuteActivitySuiteStandalone activity docs are in progress.