Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Temporal .NET SDK Proposal (version 1.0.0-PreReleaseA) #1

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

chronoguy
Copy link
Owner

This pull request is for the convenience of leaving comments on the proposal.

@chronoguy chronoguy marked this pull request as draft December 29, 2021 10:04
@chronoguy chronoguy changed the title Temporal .NET SDK Proposal (version 0.1) Temporal .NET SDK Proposal (version 1.0.0-PreReleaseA) Dec 31, 2021
@chronoguy chronoguy marked this pull request as ready for review December 31, 2021 11:38
Copy link

@bergundy bergundy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see things coming together.

Some high level comments:

  • I'd avoid inventing new APIs that the other SDKs don't have
  • The markdown format is far more convenient for commenting than the sample code, let's add more content there
  • I'd avoid using strings for referencing workflows / activities / signals / queries, it's error prone and should only be done in the fully dynamic case
  • Cancellation could use a dedicated section.
    • How do you create sub cancellation tokens?
    • Is there an equivalent to
    • Do all operations (sleep / calling an activity) accept a cancellation token? If one is not provided do they use the current workflows cancellation token (how? with no implicit context)?

**Workflows are strictly single-threaded and asynchronous:**

**In the first version of the .NET SDK, workflows must be <u>strictly logically single-threaded</u>.**
It is not permitted to use any APIs that create or manipulate threads (implicitly or explicitly), execute thread-synchronization primitives, or perform any kind of parallel execution. Note that being _logically_ single-threaded does not mean that a given workflow is bound to a specific thread. On contrary, the Engine will suspend workflows that block waiting for Temporal events to occur, and it will resume such workflows on any (likely a different) thread when those events eventually occur. However, _logically_ workflow must be strictly single-threaded. I.e., while the execution may be performed on different underlying threads, it must never _fork_ into parallel (aka concurrent) execution sequences.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think concurrency (what you call asynchrony) is expected in Workflows, we should definitely prohibit true parallelism.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think concurrency (what you call asynchrony) is expected in Workflows, we should definitely prohibit true parallelism.

To make sure we use the same terms:

  • "Asynchrony" is about not worrying about when an operation executes or completes and only carrying about its completion / result.
  • "Parallelism" is about executing something in parallel to each other (i.e. at the same time). Strictly speaking, parallel operations are a subset of concurrent operations.
  • "Concurrency" is about running things at the same time. Strictly speaking, concurrent operations on a single core machine would not be parallel. Asynchronous operations may (or not be concurrent), but in practice they often (usually) are.

In .NET. Taks were first used to address scenarios from Parallelism (in .NET 4.0), and then from Synchrony (in .NET 4.5), which makes them confusing. :)

I'll clarify this in text and will check that the terms are used consistently.

In the SDK we want asynchrony, but no parallelism.
There is a section in the docs that says that in the first version we want to avoid tackling the challenge of taking parallel code and turning it into concurrent, non-parallel code by creating a custom scheduler. If there is demand for it, we can and will tackle is subsequently.

Does this clarify things?

Copy link

@Sushisource Sushisource Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally

parallel (aka concurrent)

These terms do have different and well-defined meanings. Parallelism is when two things actually execute simultaneously. Concurrency is the appearance of simultaneous execution. A single-core processor can execute programs concurrently, because the OS forces them to yield to each other, but can never execute them in parallel.

In this context, it means workflows can perform concurrent, but never parallel, actions (which we will enforce for/require of the user)

(Heh, which you just posted at the same time 😆 )

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we are saying the same thing with slightly different words. :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️



**.NET asynchrony does not create concurrency.** <a name="DotNetAsyncIsLogicallySingleThreaded"></a>
Crucially, async/await-based asynchrony in .NET is _not_ implicitly multi-threaded. This means that invoking or awaiting [Task](https://docs.microsoft.com/en-us/dotnet/standard/asynchronous-programming-patterns/task-based-asynchronous-pattern-tap)-based async APIs does not result in logical multi-threading, except when a parallel execution thread is explicitly requested via one of the corresponding APIs. More on this topic is discussed in section [@ToDo](.).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Tasks run on a thead pool by default.
How do we ensure that they resolve deterministically?
Are we going to create Workflow Tasks that run on a single thread?
Which Task APIs are considered safe to use from the Workflow?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Tasks run on a thread pool by default.

No that is not the case. Tasks have been used in .NET first to deal with parallel scenarios, then with asynchrony. They are not run on the thread pool dy default. They are used like a future. I'll add a section that explains that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what it says here: https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.run?view=net-6.0#System_Threading_Tasks_Task_Run_System_Action_, I'm assuming you're planning on running them in a different way.
The fact that Task is located in system.threading.tasks implies that we should be careful when using these APIs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a section on this in the next version of the doc. In a nutshell: Tasks are just futures. They represent the completion of an operation. An operation may be asynchronous (you don't care when it runs), parallel (you explictly use parallelism) or synchronous (perhaps you want a consistent API iface with some other operations that are not synchronous). There are historical reasons for some confusing API names that I briefly touch on in the new doc version. Task.Run is an API that starts a parallel (not asynchronous) operation and returns a future representing its completion. Task.Run as well as all other explicitly parallel operations must be disallowed in wf code.

Task<Foo> completion = await ReadDataAsync();
DoStuff();
Foo res = await completion;

is asynchronous, not parallel. What is does in respect to concurrency depends on the implementation of ReadDataAsync. In fact, if, say, data was already in a local buffer, then ReadDataAsync probably completed synchronously.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no need for custom awaitables in our scenarios. We can, of course, use this feature if we want to. So far, I have not see a scenario, but please let us discuss if you think I am overlooking something.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

**Alternative considerations.**
It must be noted that other language SDKs take a variety of approaches to the multi-threading restrictions. This is required asynchronous and parallel programming is handled differently across runtimes. For example, the Java SDK permits "pseudo-parallelism": It permits workflows to invoke a subset of threading API. The Java SDK takes over the scheduling of such threads and ensures that such scheduling is deterministic.

In .NET such approach is feasible, but we made an explicit decision not to provide such features in version 1. Two main reasons drove this decision:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me the 2 main reasons not to use threads are:

  1. They're expensive and will limit the number of workflows the SDK can cache
  2. Letting users use language built-in concurrency primitives like tasks and async await

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

**SDK offers deterministic alternatives to frequently used non-deterministic APIs.**
Most of the non-threading related non-deterministic APIs lack determinism, because they interact with the runtime environment. Examples include time-related APIs (including random number generators seeded by time), I/O, hardware access and configuration access. The SDK provides deterministic alternatives to most frequent such APIs in a manner similar to other SDKs.

**Data resolved via .NET dependency injection APIs is deterministic, because it is persisted in workflow history and read from there when a workflow is replayed.**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example here would go a long way.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Will add some.


### Workflow Host Configuration

The user-facing foundation of the SDK is the host. It is configured in a manner that closely follows the conventions for configuring an ASP.NET Core application host ([basic example](../src/Samples/Basic/Temporal.Samples.DesignProcess/Part1_1_TypicalWorkflow.cs#L73-L89) | [more examples](../src/Samples/Basic/Temporal.Samples.DesignProcess/Part4_1_BasicWorkflowUsage.cs#L67-L202) | [another example](../src/Samples/Basic/Temporal.Samples.DesignProcess/Part4_2_BasicWorkflowUsage_MultipleWorkers.cs#L108-L158)).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd link the reference from which you borrowed this API.
You're introducing a bunch of concepts that are not unique to this SDK and I'd avoid that at all cost.
If we're convinced that this pattern is idiomatic in .Net I'm okay with it, but I want to make sure there isn't a less verbose way.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main entry point for (almost) every .NET app that offers a website or a REST API looks like that.
Visual Studio generates that code by default for new projects of those kinds.
So, although I would rather avoid trying to set the level of usage at which something is considered idiomatic, it is definitely very common.

I'll point to some examples.

I would also prefer as little verbosity as possible. I think that typical length of the proposed start-up code is approx. equivalent to what we have in Java. Please let me know if you disagree.

public IWorkflowImplementationConfiguration WorkflowImplementationConfig { get; }
public IOrchestrationService Orchestrator { get; }
public WorkflowRunContext CurrentRun { get; }
public WorkflowPreviousRunContext LastRun { get; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't available to the workflow

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

{
public IWorkflowExecutionConfiguration WorkflowExecutionConfig { get; }
public IWorkflowImplementationConfiguration WorkflowImplementationConfig { get; }
public IOrchestrationService Orchestrator { get; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd flatten this into the WorkflowContext.

public IOrchestrationService Orchestrator { get; }
public WorkflowRunContext CurrentRun { get; }
public WorkflowPreviousRunContext LastRun { get; }
public IDeterministicApi DeterministicApi { get; set; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd flatten this into the WorkflowContext.

/// If metadata specifies an available serializer - get that one;
/// If metadata specifies an unavailable serializer - throw;
/// If metadata specified nothing - get the default form the config.</summary>
public IPayloadSerializer GetSerializer(PayloadsCollection payloads) { return null; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why invent a new name instead of using the DataConverter interface we have in the other SDKs?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this isn't exposed in the other SDKs, why is it required here?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why invent a new name instead of using the DataConverter interface we have in the other SDKs?
Serializer is a common .NET term. However, I do not mind either way, so happy to rename it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this isn't exposed in the other SDKs, why is it required here?
It can be set via config.
I'll add a clarifying example.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serializer is a common .NET term. However, I do not mind either way, so happy to rename it.

I think we should. Serializer is also the common Java (and Rust) term, and Go uses Marshaler, but "DataConverter" is still chosen. I've found when it comes to SDK parity and parlance, the more similarities without overly breaking per-lang norms, the easier it is to talk about capabilities in a general way.

DateTime DateTimeUtcNow { get; }

Random CreateNewRandom();
Guid CreateNewGuid();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same as uuid?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term for UUID in the Microsoft world is GUID.

Where Core features are lacking, we will make case-by-case decisions to add required functionality to Core or to implement it directly in .NET.

**SDK Engine hosts workflows and activities and manages their execution.**
The center of the .NET SDK is the _Engine_. The Engine keeps a list of workflows and activities hosted in a particular worker host, and a mapping of active workflow (and activity) instances and their respective local execution states. The Engine contains a message loop that polls Core for work items, such as workflow activations, activity tasks, etc. Based on that, the engine invokes and manages user-provided workflow / activity implementations.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW (not that you have to match anything here), in Go we call the thing that keeps that list the "registry". We also maintain an abstraction called "environment" a bit higher than "engine" here wrt message loop so that we can replace with a "test environment" (or here it can be a "test engine").

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I gotta think about it. :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having thought about it: I adjusted the terminology to use "registry". As far as test server: depending on the final architecture, that may be abstracted away by Core. Let's discuss it later.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️

**Alternative considerations.**
It must be noted that other language SDKs take a variety of approaches to the multi-threading restrictions. This is required asynchronous and parallel programming is handled differently across runtimes. For example, the Java SDK permits "pseudo-parallelism": It permits workflows to invoke a subset of threading API. The Java SDK takes over the scheduling of such threads and ensures that such scheduling is deterministic.

In .NET such approach is feasible, but we made an explicit decision not to provide such features in version 1. Two main reasons drove this decision:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this decision. Threads should be abstracted and not user-facing in workflows and, if Java had a construct for yielding a thread, they wouldn't use them everywhere (i.e. waiting on project loom). Yes we can provide tunable thread pool counts that tasks may run across, but from an execution POV, everything will always appear single threaded unless they're trying to do something with thread-local storage or thread ID which they should not.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree but for the reasons I stated above.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we all agree on this aspect. :)

✔️


**Environment interactions:**

**SDK offers deterministic alternatives to frequently used non-deterministic APIs.**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is addressed later in the review, but can something be added about the options to prevent use of non-deterministic APIs from workflow executions? I am not familiar enough with .Net, but I am very curious the options there (can you restrict some APIs at runtime by thread? what sandbox options exist? do we need to consider a multi-process model?).

Of course neither Go nor Java tackle this today, but we really want to.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. We should touch on that in the discussion, even if we implement it later.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The primary reason I bring it up now is because it can affect worker/workflow instantiation. For instance, if there is some kind of .Net sandbox construct that means they have to pass in workflow code instead of just class type, we can incorporate that. TS has to do the same.

I also just so happen to find this the most interesting part of any SDK development. I have found about a dozen ways to hack this in Go (and I know exactly how I'd do it w/ advanced JVM constructs).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @cretz, this should be one of the core discussions in this proposal.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to next doc version.

✔️

The user-facing foundation of the SDK is the host. It is configured in a manner that closely follows the conventions for configuring an ASP.NET Core application host ([basic example](../src/Samples/Basic/Temporal.Samples.DesignProcess/Part1_1_TypicalWorkflow.cs#L73-L89) | [more examples](../src/Samples/Basic/Temporal.Samples.DesignProcess/Part4_1_BasicWorkflowUsage.cs#L67-L202) | [another example](../src/Samples/Basic/Temporal.Samples.DesignProcess/Part4_2_BasicWorkflowUsage_MultipleWorkers.cs#L108-L158)).

Users provide workflow implementations and activity implementations in one of two ways:
- Pattern based: Entry points of workflow and activity implementations follow specific, well known patterns. Developers use attributes to provide metadata about such entry point to the host. The host automatically loads, and executes workflows and activities. This approach allows writing workflows (and activities) with hardly any scaffolding.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by pattern? Just task queue string and workflow "type" string right?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean to say - entry point APIs have one of several permitted shapes / signatures and developers use attributes to mark them. I'll clarify that.

```cs
public static void Main(string[] args)
{
IHost appHost = Host.CreateDefaultBuilder(args)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "Host" instead of just using the word "Worker"? Why have all these extra lines and closures and "Moniker" and such instead of just creating a worker and registering workflows and/or activities with it?

var worker = new Worker("SomeTaskQueue");
worker.RegisterWorkflow<SomeWorkflow>();
worker.Run();

That reads much clearer to me. I think it's very important to be as simple as possible from a library user POV. That example may be too simple (may want options class or builder pattern or whatever for worker options), but the idea is there.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASP.NET "host" is how .NET web apps are started app.
If we follow that approach, we permit dependency injection, configuration from different source, and allowing workers and exposing remote, app specific APs, all at the same time and using a de-facto standard pattern.

Doing what you suggest above is, indeed, shorter. But once we want to tackle the above-mentioned scenarios it would get longer, would it not?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have to see what those scenarios look like. At its root, a worker connects to a server over gRPC and "runs" and "does things". If this "host" does a lot more things, maybe then it's the host piece that can leverage a worker instead of the other way around.

As with all the rest of this, take the simplest approach first, and then complicate it with additional utilities when you add use cases. Don't complicate the simplest approach just because of some potentially unused features.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host is the .NET abstraction for the thing in the app that

  • reads configuration (from files of code) and provides it to components that require it,
  • sets up and runs the DI container
  • instantanes, starts and provides the thing that handles the requests
  • etc....
    In the case of Temporal "thing that handles the requests" is the worker. And the "requests" here are items from the Task queue.

From what I have heard from other folks (please correct me if I am wrong), people write apps that so more than execute workflows. For example, if you were to write an app that executes workflows and also exposes some rest APIs, then you would use the same ASP.NET host in the same way, only you would set it up with two "things that handle the requests": the Temporal worker and the HTTP web requests pipeline.

Does this clarify things?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It clarifies a bit. The worker needs to be the smallest, lightest weight thing possible and if they want configuration files, DI, hosts, and all that mess, they can have them. We shouldn't force that on them.

From what I have heard from other folks (please correct me if I am wrong), people write apps that so more than execute workflows. For example, if you were to write an app that executes workflows and also exposes some rest APIs, then you would use the same ASP.NET host in the same way, only you would set it up with two "things that handle the requests": the Temporal worker and the HTTP web requests pipeline.

I think it's important not to develop this with the assumption of how one may use it. They may use it as part of their host-based app or in ASP.Net or whatever. Or they may use it as some embedded mono thing at a really low level. Who knows. Make it as small and simple as possible and if there is value for providing extras on top, we can add things for that.

{
// Use the ASP.Net Core host initialization pattern:
// 'UseTemporalWorkerHost' will configure all the temporal defaults
// and also apply all the file-based application config files.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File based? I think file-based configuration is out of scope for the SDKs at the moment.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very common for .NET web-API apps. Reading config from files is handled by the built-in API. Consuming it does not depend on where it originated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @cretz this is premature, let's nail the basics first


serviceCollection.AddWorkflowWithAttributes<SayHelloWorkflow>();

serviceCollection.AddActivity<SpeechRequest>("SpeakAGreeting", Speak.GreetingAsync);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we support AddActivities<Speak>() that automatically adds public methods? Or maybe we can require opt-in with an attribute on each activity method?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


serviceCollection.AddWorkflowWithAttributes<SayGreetingWorkflow>();

serviceCollection.AddActivity<SpeechRequest>(RemoteApiNames.Activities.SpeakGreeting, Speak.GreetingAsync);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just for demonstration purposes that the activity name is different than the method name? From an implementer POV (and therefore from a best-practices/example POV), I'd probably name them the same.


// Workflow interfaces:

// These ifaces are used by the client (see 'Part2_2_IfaceBasedClient').
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmm, I wonder if we can marry a dynamic proxy invoker approach and combined with an implicit context, you don't need this custom interface if you already have the workflow type, but instead can just make a var myClient = new client.WorkflowClient<ShoppingCart>() or something. Then you can var cartRun = await myClient.Start() and var result = await cartRun.GetProductsAsync() or something.

I think Java's approach shows a lot of promise over all of this manual typing (but I understand if there are limitations in .Net).

// @ToDo: If we want to support returning these from workflow APIs, we must either implement IDataValue or find some other solution.
public sealed class TryGetResult<T>
{
private readonly bool _isSuccess;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What minimum version of C# are we allowed to target? Can't you just public bool IsSuccess { get; init; }? (I see a lot of C# features, e.g. var, properties, etc that are not leveraged making the code quite wordy)

@chronoguy
Copy link
Owner Author

Thanks for the feedback, @bergundy.

  • I'd avoid inventing new APIs that the other SDKs don't have

Agree. There is a balance between taking an opportunity to improve things in an area where we are not restricted by legacy decisions and keeping things consistent. Other SDKs are definitely a good guide. Did you have any particular APIs in mind that should look more similar to other SDKs in your opiniuon?

  • The markdown format is far more convenient for commenting than the sample code, let's add more content there

Sure. Can do that.

  • I'd avoid using strings for referencing workflows / activities / signals / queries, it's error prone and should only be done in the fully dynamic case

There has been a clear trend towards less code scaffolding and and more pattern-based coding in .NET in recent years. If course, if there is a specific opportunity to reduce potential for errors, we should. Do you have anything particular in mind?

  • Cancellation could use a dedicated section.
    • How do you create sub cancellation tokens?
    • Is there an equivalent to
    • Do all operations (sleep / calling an activity) accept a cancellation token? If one is not provided do they use the current workflows cancellation token (how? with no implicit context)?

Sure. I'll add some details on that.

{
string ActivityTypeName { get; }

Task<PayloadsCollection> RunAsync(PayloadsCollection input, WorkflowActivityContext workflowCtx);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mentioned once elsewhere, but is it standard to postfix these names with Async in .NET? The type signature makes that clear, and it seems just Run would work fine here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but important, I believe this interface should be generic over the input type.

In your later implementation example:

public class SpeakAGreetingActivity : IBasicActivity
        {
            /// <summary>Optionally customize the name under which this activity is registered:</summary>
            public string ActivityTypeName { get { return "SpeakAGreeting1"; } }

            public Task<PayloadsCollection> RunAsync(PayloadsCollection input, WorkflowActivityContext activityCtx)
            {
                string greetingText = activityCtx.GetSerializer(input).Deserialize<string>(input) ?? "<null>";
                Console.WriteLine($"[{ActivityTypeName}] {greetingText}");
                return Task.FromResult<PayloadsCollection>(null);
            }
        }

You deserialize inside the activity. Users shouldn't have to do this, since the first line of basically every activity is going to be exactly that. This should be done for the user, and then the actual type they care about should be passed in to their activity function.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we'd end up with something like public interface IActivity<T> where T: IDataValue (assuming we do use IDataValue)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re Async - see pointers in the equivalent thread.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re generic iface:
The idea is the same layering as with workflows: the lowest level of abstraction provides access to the raw PayloadsCollection. A higher level of abstraction is generic and required an IDavaValue parameter. We can add an IActivity. Initially I have not done it because I allowed any method with the right signature. It is not required to implement a partikular iface. I have some examples, I'll centralize them in the doc.


// =========== Basic activity abstractions ===========

public interface IBasicActivity

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Basic activity? IMO this should just be IActivity unless we expect some kind of Advanced activity, but I'm not sure what that'd be.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also consider a design similar to Java where multiple activities can be colocated in the same class.

Comment on lines 33 to 37
private async Task SpeakGreetingAsync(string text, WorkflowContext workflowCtx)
{
PayloadsCollection greetingPayload = workflowCtx.WorkflowImplementationConfig.DefaultPayloadSerializer.Serialize(text);
await workflowCtx.Orchestrator.Activities.ExecuteAsync("SpeakAGreeting1", greetingPayload);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit the same as my other comment - users shouldn't need to explicitly serialize inputs before calling activities

Comment on lines +73 to +76
public static class Speak
{
public static Task GreetingAsync(SpeechRequest input, WorkflowActivityContext activityCtx)
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this one looks very much like what I'd expect, without the manual serialization on either side, but I'm not clear on how this gets turned into an IBasicActivity implementer. Can you elaborate on that a bit?

Copy link
Owner Author

@chronoguy chronoguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants