Command reordering in Core #315
Conversation
| // 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.
nit: (there might be a few other places in the repo)
| // In a previous version of .NET, if this was a successful activation completion with | |
| // In a previous version of .NET SDK, if this was a successful activation completion with |
There was a problem hiding this comment.
A bit pedantic for a code comment, but I understand the ambiguity to the reader. Will change.
There was a problem hiding this comment.
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.
Very true, I definitely missed that prefacing. My bad (and fixed anyways).
| // | ||
| // 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. | ||
|
|
There was a problem hiding this comment.
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.
I can change the variable name to lastCompletionCommandIndex if that helps
There was a problem hiding this comment.
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.
Updated var name, yeah "the completion in question" is hopefully implied now
| // 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.
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:
-
This release of .NET will also be the first .NET release that has the new core command reordering code.
-
So, for example, let
Cbe a completion command andNbe a non-completion, and consider a workflow being processed today by a .NET worker on the recently released code. Suppose the raw lang sequence isN C1 N C2 N. The .NET flag is set, so .NET will change that toN C1 N N C2. And core will change that toN C1, since it's old core. -
Now, if we release .NET workers based on this PR, the same thing will happen on replay, because the .NET flag will be set, and the new core internal flag will not be set, so both .NET and core will do their legacy behavior.
There was a problem hiding this comment.
Now consider a workflow born after this PR is released. Again the raw commands collected in lang are N C1 N C2 N. lang does not do anything to them, since lang's flag is not set, and core receives them unchanged. Core then does its new behavior, and changes them to N N N C1, and sets core's flag, so that this will also be done on replay.
There was a problem hiding this comment.
Yes, everything in your statements is correct
What was changed
Checklist