-
Notifications
You must be signed in to change notification settings - Fork 33
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
Command reordering in Core #315
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
+1 −1 | Cargo.toml | |
+42 −15 | client/src/lib.rs | |
+10 −6 | client/src/raw.rs | |
+4 −3 | core/Cargo.toml | |
+3 −3 | core/src/abstractions.rs | |
+1 −1 | core/src/core_tests/determinism.rs | |
+87 −21 | core/src/core_tests/workflow_tasks.rs | |
+17 −7 | core/src/internal_flags.rs | |
+1 −10 | core/src/pollers/poll_buffer.rs | |
+3 −3 | core/src/test_help/mod.rs | |
+1 −4 | core/src/worker/activities/local_activities.rs | |
+1 −4 | core/src/worker/mod.rs | |
+7 −1 | core/src/worker/workflow/machines/workflow_machines.rs | |
+214 −14 | core/src/worker/workflow/managed_run.rs | |
+1 −11 | core/src/worker/workflow/mod.rs | |
+8 −0 | sdk-core-protos/src/history_builder.rs | |
+131 −5 | tests/integ_tests/client_tests.rs |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -68,7 +68,6 @@ internal class WorkflowInstance : TaskScheduler, IWorkflowInstance, IWorkflowCon | |||||
private readonly Action<WorkflowInstance> onTaskStarting; | ||||||
private readonly Action<WorkflowInstance, Exception?> onTaskCompleted; | ||||||
private readonly IReadOnlyCollection<Type>? workerLevelFailureExceptionTypes; | ||||||
private readonly bool disableCompletionCommandReordering; | ||||||
private readonly Handlers inProgressHandlers = new(); | ||||||
private WorkflowActivationCompletion? completion; | ||||||
// Will be set to null after last use (i.e. when workflow actually started) | ||||||
|
@@ -190,7 +189,6 @@ public WorkflowInstance(WorkflowInstanceDetails details) | |||||
Random = new(details.Start.RandomnessSeed); | ||||||
TracingEventsEnabled = !details.DisableTracingEvents; | ||||||
workerLevelFailureExceptionTypes = details.WorkerLevelFailureExceptionTypes; | ||||||
disableCompletionCommandReordering = details.DisableCompletionCommandReordering; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
|
@@ -580,8 +578,8 @@ public WorkflowActivationCompletion Activate(WorkflowActivation act) | |||||
} | ||||||
} | ||||||
|
||||||
// Maybe apply workflow completion command reordering logic | ||||||
ApplyCompletionCommandReordering(act, completion, out var workflowComplete); | ||||||
// Maybe apply legacy workflow completion command reordering logic | ||||||
ApplyLegacyCompletionCommandReordering(act, completion, out var workflowComplete); | ||||||
|
||||||
// Log warnings if we have completed | ||||||
if (workflowComplete && !IsReplaying) | ||||||
|
@@ -1425,20 +1423,11 @@ private string GetStackTrace() | |||||
}).Where(s => !string.IsNullOrEmpty(s)).Select(s => $"Task waiting at:\n{s}")); | ||||||
} | ||||||
|
||||||
private void ApplyCompletionCommandReordering( | ||||||
private void ApplyLegacyCompletionCommandReordering( | ||||||
WorkflowActivation act, | ||||||
WorkflowActivationCompletion completion, | ||||||
out bool workflowComplete) | ||||||
{ | ||||||
// In earlier versions of the SDK we allowed commands to be sent after workflow | ||||||
// completion. These ended up being removed effectively making the result of the | ||||||
// workflow function mean any other later coroutine commands be ignored. To match | ||||||
// Go/Java, we are now going to move workflow completion to the end so that | ||||||
// same-task-post-completion commands are still accounted for. | ||||||
// | ||||||
// Note this only applies for successful activations that don't have completion | ||||||
// reordering disabled and that are either not replaying or have the flag set. | ||||||
|
||||||
// Find the index of the completion command | ||||||
var completionCommandIndex = -1; | ||||||
if (completion.Successful != null) | ||||||
|
@@ -1459,21 +1448,22 @@ private void ApplyCompletionCommandReordering( | |||||
} | ||||||
workflowComplete = completionCommandIndex >= 0; | ||||||
|
||||||
// This only applies for successful activations that have a completion not at the end, | ||||||
// don't have completion reordering disabled, and that are either not replaying or have | ||||||
// the flag set. | ||||||
// In a previous version of .NET, if this was a successful activation completion with | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: (there might be a few other places in the repo)
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit pedantic for a code comment, but I understand the ambiguity to the reader. Will change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a reviewer prefixes their comment with "nit", you can't complain it's pedantic :) Universal law of code review! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very true, I definitely missed that prefacing. My bad (and fixed anyways). |
||||||
// a completion command not at the end, we'd reorder it to move at the end. However, | ||||||
// this logic has now moved to core and become more robust. Therefore, we only apply | ||||||
// this logic if we're replaying and flag is present so that workflows/histories that | ||||||
// were created after this .NET flag but before the core flag still work. | ||||||
if (completion.Successful == null || | ||||||
completionCommandIndex == -1 || | ||||||
completionCommandIndex == completion.Successful.Commands.Count - 1 || | ||||||
disableCompletionCommandReordering || | ||||||
(IsReplaying && !act.AvailableInternalFlags.Contains( | ||||||
(uint)WorkflowLogicFlag.ReorderWorkflowCompletion))) | ||||||
!IsReplaying || | ||||||
!act.AvailableInternalFlags.Contains((uint)WorkflowLogicFlag.ReorderWorkflowCompletion)) | ||||||
{ | ||||||
return; | ||||||
} | ||||||
|
||||||
// Now we know the completion is in the wrong spot and we're on a newer SDK, so set the | ||||||
// SDK flag and move it | ||||||
// Now we know that we're replaying w/ the flag set and the completion in the wrong | ||||||
// spot, so set the SDK flag and move it | ||||||
completion.Successful.UsedInternalFlags.Add((uint)WorkflowLogicFlag.ReorderWorkflowCompletion); | ||||||
var compCmd = completion.Successful.Commands[completionCommandIndex]; | ||||||
completion.Successful.Commands.RemoveAt(completionCommandIndex); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So just to try to recap this logic for myself and anyone else. It would require a fairly complicated diagram to set it all out, but here's one story:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now consider a workflow born after this PR is released. Again the raw commands collected in lang are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, everything in your statements is correct |
||||||
|
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 would be worth leaving a comment for any future readers explaining that the code below assumes there is only one completion command. Otherwise having "the completion" in comments could confuse readers, making them forget that there could be multiple.
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 can change the variable name to
lastCompletionCommandIndex
if that helpsThere 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.
Sure yes, and ideally "the last completion" in comments rather than "the completion" but, that's prefixed by "nit"!
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.
Updated var name, yeah "the completion in question" is hopefully implied now