Update Core #376
Update Core #376
Conversation
Makes it easier to run this without local tooling, which is a massive pain on linux
With temp workaround for job ordering
Sushisource
force-pushed
the
core-update
branch
from
fc1e49e to
18e7bcc
Compare
Sushisource
force-pushed
the
core-update
branch
from
045dbce to
95428ea
Compare
| @@ -1227,7 +1227,7 @@ await ExecuteWorkerAsync<MiscHelpersWorkflow>( | |||
| // Now query after close to check that is replaying worked | |||
| var isReplayingValues = await Env.Client.GetWorkflowHandle<MiscHelpersWorkflow>( | |||
| workflowId).QueryAsync(wf => wf.GetEventsForIsReplaying()); | |||
| Assert.Equal(new[] { false, true }, isReplayingValues); | |||
| Assert.Equal(new[] { false, true, true }, isReplayingValues); | |||
There was a problem hiding this comment.
This is also due to the evict-on-finish change
There was a problem hiding this comment.
Took me a sec to grok what was happening here. But now I see that on line 1214 above we issue a post-complete query which now causes full replay where it didn't before
Sushisource
force-pushed
the
core-update
branch
from
95428ea to
6b33906
Compare
| @@ -538,8 +538,25 @@ public WorkflowActivationCompletion Activate(WorkflowActivation act) | |||
| { | |||
| // We must set the sync context to null so work isn't posted there | |||
| SynchronizationContext.SetSynchronizationContext(null); | |||
| // TODO: Temporary workaround in lieu of https://github.com/temporalio/sdk-dotnet/issues/375 | |||
| var sortedJobs = act.Jobs.OrderBy(j => | |||
There was a problem hiding this comment.
I am a bit surprised we didn't order before. But I assume this order by matches previous Core versions before this update? Meaning we feel fairly confident that replays will work the same on .NET SDK upgrade here?
There was a problem hiding this comment.
Yes, this doesn't change anything. It matches the previous Core ordering
| @@ -1715,6 +1732,7 @@ public override Task DelayAsync(DelayAsyncInput input) | |||
| { | |||
| Seq = seq, | |||
| StartToFireTimeout = Google.Protobuf.WellKnownTypes.Duration.FromTimeSpan(delay), | |||
| Summary = instance.PayloadConverter.ToPayload(string.Empty), | |||
There was a problem hiding this comment.
| Summary = instance.PayloadConverter.ToPayload(string.Empty), |
I think we want this to be null instead of a full payload (and we will add if summary is present as part of #359). Is there some kind of check Core-side forcing us to set this?
There was a problem hiding this comment.
No, I was confused by something going on in one of the tests and forgot to clean this up
| @@ -1227,7 +1227,7 @@ await ExecuteWorkerAsync<MiscHelpersWorkflow>( | |||
| // Now query after close to check that is replaying worked | |||
| var isReplayingValues = await Env.Client.GetWorkflowHandle<MiscHelpersWorkflow>( | |||
| workflowId).QueryAsync(wf => wf.GetEventsForIsReplaying()); | |||
| Assert.Equal(new[] { false, true }, isReplayingValues); | |||
| Assert.Equal(new[] { false, true, true }, isReplayingValues); | |||
There was a problem hiding this comment.
Took me a sec to grok what was happening here. But now I see that on line 1214 above we issue a post-complete query which now causes full replay where it didn't before
Updating core to separate those changes from the PR for slot suppliers #372
Which I will rebase after this is merged