Workflow replayer, docs, time-skipping test server, rename ActivityContext, core update, and more #48

cretz · 2023-04-17T20:23:14Z

What was changed

Added workflow docs to README
Updated core (and needed regenerated pieces)
Update CI to build on older Linux (fixes [Bug] NuGet package is built with too-new of a glibc version #36)
Fix construction of data converter (fixes [Bug] DataConverter should not eagerly instantiate payload/failure converter from type #41)
Properly implement time-skipping test server
Implement workflow replayer
Rename ActivityContext to ActivityExecutionContext

Sorry so much was included. It was originally small, but difficulties mounted during PR. Can ignore generated code of course.

Fixes temporalio#36

Fixes temporalio#41

cretz · 2023-04-17T20:27:04Z

tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs

@@ -2363,7 +2343,7 @@ public class PostPatchWorkflow : PatchWorkflowBase
        }
    }

-    [Fact]
+    [Fact(Skip = "TODO(cretz): This is currently not throwing expected non-determinism errors in core")]


@Sushisource - this is the test that, on the code below that runs:

var exc = await Assert.ThrowsAsync<WorkflowQueryFailedException>( () => QueryWorkflowAsync(prePatchID));

This is not throwing the non-determinism error one might expect even with latest core. This is testing that using query to replay pre-patch code on a post-patch workflow will cause non-determinism (activity type change).

OK, weird. I'll check this out - what's the easiest way for me to run just this test? I haven't learned much of the tooling for this yet.

Download the dotnet binary, clone this repo recursively, then in this dir run dotnet build (which runs Rust build, so you'll need protoc and such), remove the Skip message from the attribute, and then you can do something like:

dotnet run --project tests/Temporalio.Tests -- -verbose -method "*.ExecuteWorkflowAsync_Patched_ProperlyHandled"

That should be enough for you to see. Feel free to comment out many parts of this multi-step test. Feel free to hit me up with any snags.

Turns out this is because the version of server this SDK is using for tests doesn't support the sdk metadata and hence internal flags feature

👍 I will change the skip message to be more clear. I have opened #50. Semi-relatedly, I have also opened temporalio/sdk-java#1746.

src/Temporalio/Converters/DataConverter.cs

bergundy

Did not do an in-depth review but overall LGTM.

bergundy · 2023-04-18T19:43:49Z

.github/workflows/package.yml

-        if: ${{ matrix.os != 'ubuntu-arm' }}
+      - name: Install protoc (non-Linux)
+        # Does not work on Linux
+        if: ${{ matrix.os != 'ubuntu-latest' && matrix.os != 'ubuntu-arm' }}


nit: use contains(matrix.os, 'ubuntu').

bergundy · 2023-04-18T19:53:56Z

src/Temporalio/Converters/DataConverter.cs

+        /// <param name="payloadConverterType">Payload converter type. This must be a type and not
+        /// an instance so it can be serialized across a sandbox boundary.</param>
+        /// <param name="failureConverterType">Failure converter type. This must be a type and not
+        /// an instance so it can be serialized across a sandbox boundary.</param>


Do you want to keep using types instead of instances?
Would it make sense to require a factory here so users can pass in config to their converters (given that it's mostly codecs that need to be configurable)?

It's kinda annoying for users, but I like that even though I know this won't ever be sandboxed probably, it still forces them to think about how these are isolated. Because they can't state share in their payload or failure converters which is a good thing anyways (well, unless they use statics).

bergundy · 2023-04-18T19:54:52Z

src/Temporalio/Converters/DataConverter.cs

+        /// <param name="payloadCodec">Optional payload codec.</param>
+        public DataConverter(
+            Type payloadConverterType,
+            Type failureConverterType,


Can we make the failure converter optional?

It is effectively optional if they do DataConverter.Default with { PayloadConverterType = typeof(MyPayloadConverter) } which the README suggests.

bergundy · 2023-04-18T20:06:16Z

src/Temporalio/Worker/WorkflowReplayer.cs

+        /// <returns>Results of the replay runs.</returns>
+        public async IAsyncEnumerable<WorkflowReplayResult> ReplayWorkflowsAsync(
+            IAsyncEnumerable<WorkflowHistory> histories,
+            bool throwOnReplayFailure = false,


Why do you need this flag?
I removed it in TS since the user can decide to throw while iterating throw the results.

@Sushisource advocated for it on the Python side. Same for single workflow replay. Are you suggesting remove it from both, or have single workflow replay throw but multi-workflow replay not throw? I don't mind either way, I just want to be consistent.

Suggesting that single replay throws and multiple (async iterator) doesn't.
We can give the option for the multi replay method that replays synchronously.

Suggesting that single replay throws and multiple (async iterator) doesn't.

That's already the case by default. Are you saying both need to disallow changing the default? Or just one have the option of changing the default?

We can give the option for the multi replay method that replays synchronously.

They all replay synchronously in this sense. There is no concurrent replay.

I don't think we need the option in the async iterator method.

They all replay synchronously in this sense. There is no concurrent replay.

I was talking about the multi replay method that returns task<iterator>.
I'm of course familiar with how the replayer works.

I don't think we need the option in the async iterator method.

This argues for inconsistency. Why is a foreach ok to throw but not a await foreach?

Because in the task<iterator> method you can't get the results one by one, and it's nice to be able to fail fast.
In the async iterator method, the user has full control and can abort whenever they like.

Because in the task<iterator> method you can't get the results one by one

You can. The reason for the async iterator in this case is because I allow async iteration input. You can have either:

foreach (var result in await replayer.ReplayWorkflowsAsync(syncHistIter)) { // Process 1 by t1 }

or

await foreach (var result in replayer.ReplayWorkflowsAsync(asyncHyncHistIter)) { // Process 1 by 1 }

And technically the latter doesn't work on all .NET versions we support.

bergundy · 2023-04-18T20:07:00Z

src/Temporalio/Worker/WorkflowReplayer.cs

+            (await ReplayWorkflowsAsync(new[] { history }, throwOnReplayFailure).ConfigureAwait(false)).Single();
+
+        /// <summary>
+        /// Replay multiple workflows from the given histories.


Might want to direct users to the async API if they're on a new enough version of .NET.

I think it's clear enough when looking at API docs. And the iterable passed in is clear enough I think.

bergundy · 2023-04-18T20:15:05Z

src/Temporalio/WorkflowHistory.cs

+                }
+            }
+
+            private static void CollectPathsToFix(


Yeah, but I should probably move it to compile time code generator. But I figured lazy on first use is ok to start with.

bergundy · 2023-04-18T20:17:11Z

tests/Temporalio.Tests/Testing/WorkflowEnvironmentTests.cs

+public class WorkflowEnvironmentTests : TestBase
+{
+    // Time-skipping test server only runs on x86/x64
+    public sealed class OnlyIntelFactAttribute : FactAttribute


nit: it's not only intel, amd has this arch too but 🤷

"Intel" is a common term for the instruction set, even though it's AMD's 64-bit extensions. I just used the word they are using to describe it at https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.architecture. This is test only code anyways. I could changed to OnlyIntelBasedFactAttrbiute or something.

bergundy · 2023-04-18T20:21:34Z

tests/Temporalio.Tests/WorkflowEnvironment.cs

@@ -34,6 +34,7 @@ public async Task InitializeAsync()
                },
            },
        });
+        // env = new(await Temporalio.Client.TemporalClient.ConnectAsync(new("localhost:7233")));


Intentionally so, because I come here and run against local server so I have a UI (I used it to download those JSONs for the replayer tests). But I can make a comment to that effect.

…rging with existing payload.

cretz added 8 commits April 13, 2023 08:37

README update to include workflow docs

0313b25

Use older Linux containers to build on Linux

149926c

Fixes temporalio#36

Update SDK core

d4c1a71

Properly initialize payload/failure converter lazily

a32f2f1

Fixes temporalio#41

Time-skipping workflow environment

d71f663

Workflow replayer

2ee7e14

Update core

22800a0

Rename ActivityContext to ActivityExecutionContext

0e8fab9

cretz requested a review from a team April 17, 2023 20:23

cretz commented Apr 17, 2023

View reviewed changes

Fix child wait during Ci tests

e42f0ce

cretz mentioned this pull request Apr 18, 2023

When you planning to Release your product #49

Closed

cretz commented Apr 18, 2023

View reviewed changes

src/Temporalio/Converters/DataConverter.cs Outdated Show resolved Hide resolved

bergundy approved these changes Apr 18, 2023

View reviewed changes

cretz added 2 commits April 19, 2023 08:03

Minor fixes

fcecf55

Fix codec helper to make sure we convert from lazy iterable before me…

00b23a9

…rging with existing payload.

cretz merged commit 3408d01 into temporalio:main Apr 19, 2023

cretz deleted the workflows-more-5 branch April 19, 2023 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow replayer, docs, time-skipping test server, rename ActivityContext, core update, and more #48

Workflow replayer, docs, time-skipping test server, rename ActivityContext, core update, and more #48

cretz commented Apr 17, 2023

cretz Apr 17, 2023

Sushisource Apr 17, 2023

cretz Apr 17, 2023 •

edited

Loading

Sushisource Apr 18, 2023

cretz Apr 19, 2023

bergundy left a comment

bergundy Apr 18, 2023

bergundy Apr 18, 2023

cretz Apr 18, 2023 •

edited

Loading

bergundy Apr 18, 2023

cretz Apr 18, 2023

bergundy Apr 18, 2023

cretz Apr 18, 2023

bergundy Apr 19, 2023

cretz Apr 20, 2023 •

edited

Loading

bergundy Apr 20, 2023

cretz Apr 20, 2023

bergundy Apr 20, 2023

cretz Apr 20, 2023 •

edited

Loading

bergundy Apr 18, 2023

cretz Apr 18, 2023

bergundy Apr 18, 2023

cretz Apr 18, 2023

bergundy Apr 18, 2023

cretz Apr 18, 2023

bergundy Apr 18, 2023

cretz Apr 18, 2023

Workflow replayer, docs, time-skipping test server, rename ActivityContext, core update, and more #48

Workflow replayer, docs, time-skipping test server, rename ActivityContext, core update, and more #48

Conversation

cretz commented Apr 17, 2023

What was changed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergundy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Apr 17, 2023 •

edited

Loading

cretz Apr 18, 2023 •

edited

Loading

cretz Apr 20, 2023 •

edited

Loading

cretz Apr 20, 2023 •

edited

Loading