💥 Standalone Activities support#2029
Conversation
| throw failedErr; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks like this long-polling loop will fail to wait for longer than the server's long-poll timeout due to not handling DEADLINE_EXCEEDED. Can you compare to the implementations in Go and Python and add an integration test that demonstrates it can wait for longer than the server timeout, and for longer than the per-attempt client-side deadline? And also that it responds correctly when the activity is cancelled on the server.
Python:
while True:
try:
res = await self._client.workflow_service.poll_activity_execution(
req,
retry=True,
metadata=rpc_metadata,
timeout=rpc_timeout,
)
if res.HasField("outcome"):
self._known_outcome = res.outcome
return
except RPCError as err:
if err.status == RPCStatusCode.DEADLINE_EXCEEDED:
# Deadline exceeded is expected with long polling; retry
continue
elif err.status == RPCStatusCode.CANCELLED:
raise asyncio.CancelledError() from err
else:
raise
except asyncio.CancelledError:
raiseThere was a problem hiding this comment.
Can you also check that activity cancellation is handled correctly in this loop?
There was a problem hiding this comment.
The server behavior for polling timeout is to return empty response. This is handled correctly. The code throws exception on client-side deadline, but it could be argued it's the expected behavior - we can revisit is some other time, though it does match behavior of workflow result polling. Client-side cancellation is handled correctly.
Added tests to verify long long polling and client-side cancellation.
Sushisource
left a comment
There was a problem hiding this comment.
Overall making sense but we need to find a way to provide better type safety
9cd8d9e to
8a7d1f2
Compare
Sushisource
left a comment
There was a problem hiding this comment.
LMK if this is ready for final review or not and I can make a last pass
|
@Sushisource PR is ready for final review. |
Sushisource
left a comment
There was a problem hiding this comment.
Looking good on my end but @mjameswh or @chris-olszewski should approve too
| idConflictPolicy: encodeActivityIdConflictPolicy(input.options.idConflictPolicy), | ||
| searchAttributes: { indexedFields: searchAttributes }, | ||
| header: input.headers, | ||
| userMetadata: await encodeUserMetadata(this.dataConverter, input.options.summary, undefined), |
There was a problem hiding this comment.
Ah, I guess you can only set them from inside the activity? We should support it in both places, but can be a follow up PR. Let's make sure we track it.
chris-olszewski
left a comment
There was a problem hiding this comment.
Lgtm just minor suggestions that are not blocking
| * | ||
| * @experimental Standalone Activities are experimental. APIs may be subject to change. | ||
| */ | ||
| readonly inWorkflow: boolean; |
There was a problem hiding this comment.
Non-blocking just since this is currently experimental, but making Info enforce that inWorkflow: true implies that workflowExecution etc are present would be nice
e.g.
type Info = BaseInfo & (
| { readonly inWorkflow: true;
readonly workflowExecution: { workflowId: string; runId: string };
readonly workflowNamespace: string;
readonly workflowType: string;
readonly activityRunId?: undefined }
| { readonly inWorkflow: false;
readonly activityRunId: string;
readonly workflowExecution?: undefined;
readonly workflowNamespace?: undefined;
readonly workflowType?: undefined }
);
There was a problem hiding this comment.
This won't really work in a way that's helpful to the users, because ActivityContext.current().info still has to be the most general type and TS compiler doesn't support type assertions on this.
| type ErrorDetailsName = `temporal.api.errordetails.v1.${keyof typeof temporal.api.errordetails.v1}`; | ||
| type FailureName = `temporal.api.failure.v1.${keyof typeof temporal.api.failure.v1}`; | ||
| export type ErrorDetailsName = `temporal.api.errordetails.v1.${keyof typeof temporal.api.errordetails.v1}`; | ||
| export type FailureName = `temporal.api.failure.v1.${keyof typeof temporal.api.failure.v1}`; |
There was a problem hiding this comment.
I don't believe this needs to be made public
| export type FailureName = `temporal.api.failure.v1.${keyof typeof temporal.api.failure.v1}`; | |
| type FailureName = `temporal.api.failure.v1.${keyof typeof temporal.api.failure.v1}`; |
There was a problem hiding this comment.
It's not public, just privately exported within package. It's so it can be used in activity-client.ts.
| for (const entry of getGrpcStatusDetails(err) ?? []) { | ||
| if (!entry.type_url || !entry.value) continue; | ||
| if ( | ||
| (trimGrpcTypeUrl(entry.type_url) as ErrorDetailsName) === |
There was a problem hiding this comment.
Just to be more honest with the cast
| (trimGrpcTypeUrl(entry.type_url) as ErrorDetailsName) === | |
| (trimGrpcTypeUrl(entry.type_url) as ErrorDetailsName | '') === |
| function getSchedulingWorkflowHandle(): WorkflowHandle { | ||
| const { info, client } = Context.current(); | ||
| if (!info.inWorkflow) { | ||
| throw new Error('Not in workflow'); |
There was a problem hiding this comment.
Not major, but we usually throw IllegalStateError when attempting to use workflow only code in non-workflow contexts
💥 Breaking change
This PR alters Activity Info interface in backward-incompatible way by making
workflowExecution,workflowNamespaceandworkflowTypeproperties optional. As long as Standalone Activities are not being used, it is safe to use!to remove type errors, however the recommended migration path is to check theinWorkflowproperty and handle both cases - this way the activity can be run in standalone context.What was changed
Implemented support for Standalone Activities.
Telemetry support will be done separately, see #2031
Why?
Feature request: temporalio/features#706
Checklist
Closes [Feature Request] Support standalone activities #1851
How was this tested:
test-standalone-activities.tstest-async-completion.ts