-
Notifications
You must be signed in to change notification settings - Fork 21
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
add sample for workflow update #40
Conversation
@cretz I am trying to implement an example for workflow update, in test environment the [WorkflowUpdate] method never gets executed. this is the stacktrace
I might be doing something wrong, any idea? thank you |
I wonder if this is an issue with the time skipping test server. I will set aside some time to debug that, but see if it works with |
|
Can you check whether all uses of |
@cretz sorry I didn't come back before,
I am not really sure what you mean with "all uses". Tested
and it works. Is there anything else I can test? |
No, that confirms that the time-skipping test server does not currently work with update but does with other things. Use |
@cretz sorry forgot about this.. I am working on it again. For this example, I am implementing a UI wizard. The user submits a screen, the workflow processes the screen (maybe execute some activities) and returns the id of the next screen. I have a small issue, implementation issue, nothing to do with dotnet, If I remove this line https://github.com/temporalio/samples-dotnet/pull/40/files#diff-6717af88ce41823890a56df6172522c7503b0829944f308df6cf29549e1404c1R25 the workflow completes before the update handler returns Event WorkflowExecutionUpdateCompleted is missing Am I doing something wrong? |
9f88621
to
e0fbdcb
Compare
@antmendoza - hrmm, I would expect instant update completion (as failure since workflow completed) upon workflow completion. Can you bring this up internally and tag me (e.g. start discussion with server team)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PR is ready @cretz ,
thank you
tests/TimeSkippingServerFact.cs
Outdated
/// </remarks> | ||
public sealed class TimeSkippingServerFactAttribute : FactAttribute | ||
{ | ||
public TimeSkippingServerFactAttribute() | ||
{ | ||
if (RuntimeInformation.ProcessArchitecture != Architecture.X86 && | ||
RuntimeInformation.ProcessArchitecture != Architecture.X64) | ||
var processArchitecture = RuntimeInformation.ProcessArchitecture; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to change this file to have some tests working on my laptop, let me know if it makes sense @cretz or you want me to revert it back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this will fail CI. The problem is your macOS ARM does run fine, but Linux ARM does not. You should revert it back and not use [TimeSkippingServerFact]
on your tests since you're not using the time-skipping test server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, added several more comments, but the only two that really matter are the time skipping fact attribute update and the sln file conflict.
tests/TimeSkippingServerFact.cs
Outdated
/// </remarks> | ||
public sealed class TimeSkippingServerFactAttribute : FactAttribute | ||
{ | ||
public TimeSkippingServerFactAttribute() | ||
{ | ||
if (RuntimeInformation.ProcessArchitecture != Architecture.X86 && | ||
RuntimeInformation.ProcessArchitecture != Architecture.X64) | ||
var processArchitecture = RuntimeInformation.ProcessArchitecture; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this will fail CI. The problem is your macOS ARM does run fine, but Linux ARM does not. You should revert it back and not use [TimeSkippingServerFact]
on your tests since you're not using the time-skipping test server.
TemporalioSamples.sln
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is conflicting with main
. Since it's kinda an opaque file, you may have to merge, accept main
's version, and then dotnet add
this project again
src/WorkflowUpdate/UiRequest.cs
Outdated
@@ -0,0 +1,3 @@ | |||
namespace TemporalioSamples.WorkflowUpdate; | |||
|
|||
public record UiRequest(string RequestId, ScreenId ScreenId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could have also just made this and ScreenId
as inner classes of the workflow itself, but this is fine too
[WorkflowQuery] | ||
public ScreenId GetCurrentScreen() | ||
{ | ||
return screen; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be clearer to make this a property and remove the screen
field (queries as property getters is a neat .NET feature).
[WorkflowQuery] | |
public ScreenId GetCurrentScreen() | |
{ | |
return screen; | |
} | |
[WorkflowQuery] | |
public ScreenId CurrentScreen { get; private set; } = ScreenId.Screen1; |
await Workflow.WaitConditionAsync(() => !requests.Any()); | ||
updateInProgress = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're counting on one in-flight update at a time, might as well not use a queue, but instead just have a private UiRequest? pendingRequest
field that you set.
} | ||
|
||
// Ensure update method has completed | ||
await Workflow.WaitConditionAsync(() => !updateInProgress); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well do the per-update wait in the loop for each update
} | |
// Ensure update method has completed | |
await Workflow.WaitConditionAsync(() => !updateInProgress); | |
// Ensure update method has completed | |
await Workflow.WaitConditionAsync(() => !updateInProgress); | |
} |
return screen; | ||
} | ||
|
||
private bool IsLastScreen() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably inline this single-use one-liner into the while loop
private bool updateInProgress; | ||
|
||
[WorkflowRun] | ||
public async Task RunAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it may be clearer to have the update handler have its own logic and if it needs to have a homemade lock to prevent other updates from running using a bool field like you have, so be it, and the run function just be public Task RunAsync() => Workflow.WaitConditionAsync(() => CurrentScreen == ScreenId.End)
, but what you have here is fine too.
@cretz thanks, I've implemented the suggested changes. The code looks much clearer now
I think, feel free to add more comments if something can be improved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just minor suggestions, not required
public async Task RunAsync() | ||
{ | ||
await Workflow.WaitConditionAsync(() => currentScreen == ScreenId.End); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public async Task RunAsync() | |
{ | |
await Workflow.WaitConditionAsync(() => currentScreen == ScreenId.End); | |
} | |
public Task RunAsync() => Workflow.WaitConditionAsync(() => currentScreen == ScreenId.End); |
Can shorten to this if you want, but don't have to
return currentScreen; | ||
} | ||
|
||
private void SetNextScreen(UiRequest currentRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make this a separate method? It's just a one-liner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the code easier (for me) to read than having...
updateInProgress = true;
// Activities can be scheduled here
currentScreen = request.ScreenId switch
{
ScreenId.Screen1 => ScreenId.Screen2,
ScreenId.Screen2 => ScreenId.End,
_ => currentScreen,
};
updateInProgress = false;
wdyt?
{ | ||
} | ||
|
||
[TimeSkippingServerFact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this attribute since you're not using time-skipping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but it does not work? what should I put instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok [Fact] seems to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry, [Fact]
required for all tests (the [TimeSkippingServerFact]
is just a special kind)
}); | ||
} | ||
|
||
[TimeSkippingServerFact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this attribute since you're not using time-skipping
private ScreenId currentScreen = ScreenId.Screen1; | ||
|
||
[WorkflowRun] | ||
public async Task RunAsync() => await Workflow.WaitConditionAsync(() => currentScreen == ScreenId.End); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public async Task RunAsync() => await Workflow.WaitConditionAsync(() => currentScreen == ScreenId.End); | |
public Task RunAsync() => Workflow.WaitConditionAsync(() => currentScreen == ScreenId.End); |
Probably could be this, but what you have is good too
Looks great, merge whenever. |
What was changed
Why?
Checklist
Closes
How was this tested: