💥 Standalone activity support - #609
Conversation
| dotnet-repo-path: ${{github.event.pull_request.head.repo.full_name}} | ||
| version: ${{github.event.pull_request.head.ref}} | ||
| version-is-repo-ref: true | ||
| features-repo-ref: standalone-activities-dotnet |
There was a problem hiding this comment.
Note, we will need to merge this branch into features repo right after this PR is merged.
| /// are required. | ||
| /// </summary> | ||
| /// <remarks>WARNING: Standalone activities are experimental.</remarks> | ||
| public class StartActivityOptions : ICloneable |
There was a problem hiding this comment.
Note, after internal discussions, we chose StartActivityOptions to not clash with existing ActivityOptions which is kinda a uniquely .NET problem of preventing clashes across the entire assembly (instead of just namespace)
| ["WorkflowId"] = WorkflowId, | ||
| ["WorkflowRunId"] = WorkflowRunId, | ||
| ["WorkflowType"] = WorkflowType, | ||
| ["Namespace"] = Namespace, |
There was a problem hiding this comment.
Should we maybe preserve WorkflowNamespace and only fill it if it's workflow activity? That way we don't wholesale break anyone who is scraping logs, especially for those who aren't using this experimental feature yet.
There was a problem hiding this comment.
We can, though the question is whether we should also emit a Namespace at that time? Is it worth the redundant scope?
There was a problem hiding this comment.
Thinking this through...
Previously, WorkflowNamespace was always added as a scope value and always had a value. Log scraping/queries who look for it a likely relying on it being there. With this change as-is, that key will no longer be there at all. So they would have to adjust their code to find the new kew.
With my suggestion, WorkflowNamespace would still be provided under those circumstances IF they don't start using standalone activities. Once they start using standalone activities, some of their log entries won't have WorkflowNamespace (for those that are standalone activities). This would avoid a break until they start using the experimental feature.
So questions are:
- do we break now while the feature is still experimental and probably not used in environments where log scraping is occurring? Maybe we can defer the break until standalone activities is GA?
- do we break only for standalone activities?
There was a problem hiding this comment.
Right, all of that makes sense, and I don't have a strong opinion on whether to break existing log scope keys now, later, or not at all. It makes sense for me to put WorkflowNamespace back for now.
What I was asking was a followup question - do we emit Namespace always or only for standalone activities?
There was a problem hiding this comment.
I think we always emit Namespace and say that we're deprecating WorkflowNamespace.
There was a problem hiding this comment.
👍 I will add the redundant log tag for the majority of users
There was a problem hiding this comment.
Meaning all workflow logs will have Namespace and WorkflowNamespace, but all new standalone activity logs will have just Namespace, assuming that is what you meant.
There was a problem hiding this comment.
I'm fine with theremoval of WorkflowNamespace now as you have in this PR. If we are ever going to remove it, I think now is better than later.
There was a problem hiding this comment.
👍 Same for other workflow tags. Can you take a look at 287ed97 and confirm it is what you were thinking?
## What was changed SDK PR temporalio/sdk-dotnet#609 (standalone activity support) made ActivityInfo.WorkflowId nullable (string?). The omes .NET worker has `TreatWarningsAsErrors` enabled, so CS8604 (possible null reference) becomes a hard build error. This causes omes runs that build .NET worker from source to fail.
2 participants
What was changed
StartActivityOptions,ActivityDescribeOptions,ActivityCancelOptions,ActivityTerminateOptions,ActivityListOptions,ActivityListPaginatedOptions,ActivityCountOptionsActivityHandleandActivityHandle<TResult>withGetResultAsync,DescribeAsync,CancelAsync,TerminateAsyncActivityExecution,ActivityExecutionDescription,ActivityExecutionCount,ActivityListPagemodel classesActivityFailedExceptionandActivityAlreadyStartedExceptionexceptionsStartActivityAsync,GetActivityHandle,ListActivitiesAsync,CountActivitiesAsync,ListActivitiesPaginatedAsynconITemporalClientExecuteActivityAsyncextension methods onITemporalClient(start + get result shortcut, 4 overloads matching workflow pattern)StartActivityInput,DescribeActivityInput,CancelActivityInput,TerminateActivityInput,ListActivitiesInput,ListActivitiesPaginatedInput,CountActivitiesInputClientOutboundInterceptorScheduleToCloseTimeoutorStartToCloseTimeoutonStartActivityOptionsISerializationContext.Activityto support standalone activities:ActivityIdadded,WorkflowId/WorkflowTypenow nullable, some fields deprecatedActivityInfowithNamespace,IsWorkflowActivity, and nullable workflow fields (WorkflowId,WorkflowNamespace,WorkflowRunId,WorkflowType)NullReferenceExceptioninActivityWorker.StartActivitywhenCurrentAttemptScheduledTimeis null for standalone activitiesStartActivityfailures complete the task as failed instead of crashing the workerv1.6.1-server-1.31.0-150.0with standalone activity dynamic config flagsChecklist