Drop all post-terminal commands & sort activation jobs #502
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sushisource
requested review from
bergundy,
cretz and
Spikhalskiy
as code owners
cretz
reviewed
Mar 6, 2023
Sushisource
changed the title
cretz
reviewed
Mar 7, 2023
| /// Sorts jobs in an activation to be in the order lang expects: | ||
| /// `patches -> signals -> other -> queries` | ||
| fn sort_act_jobs(wfa: &mut WorkflowActivation) { | ||
| wfa.jobs.sort_by(|j1, j2| { |
There was a problem hiding this comment.
Can you confirm this is a "stable" sort (meaning equal things aren't reordered)?
core/src/worker/workflow/mod.rs
Outdated
| (workflow_activation_job::Variant::NotifyHasPatch(_), _) => Ordering::Less, | ||
| (workflow_activation_job::Variant::SignalWorkflow(_), _) => Ordering::Less, | ||
| (workflow_activation_job::Variant::QueryWorkflow(_), _) => Ordering::Greater, | ||
| _ => Ordering::Equal, |
There was a problem hiding this comment.
Can you confirm just for my reading that this case should never be hit because of the discriminant(j1v) == discriminant(j2v) check above? (fine to remain for exhaustion, I just want to confirm that the match clause never compares equal elements)
core/src/worker/workflow/mod.rs
Outdated
| } | ||
| match (j1v, j2v) { | ||
| (workflow_activation_job::Variant::NotifyHasPatch(_), _) => Ordering::Less, | ||
| (workflow_activation_job::Variant::SignalWorkflow(_), _) => Ordering::Less, |
There was a problem hiding this comment.
What if j1v is SignalWorkflow and j2v is NotifyHasPatch? I think you should give integer ordinals to each job and then cmp them.
There was a problem hiding this comment.
Yeah, that's a reasonable way to do it. Changed.
Sushisource
force-pushed
the
discard-post-term-commands
branch
from
4f167bd to
260a568
Compare
cretz
approved these changes
Mar 9, 2023
Sushisource
added a commit
to Sushisource/sdk-core
that referenced
this pull request
Mar 17, 2023
* Drop all post-terminal commands * Sort outgoing jobs in order lang expects
Sushisource
added a commit
to Sushisource/sdk-core
that referenced
this pull request
May 12, 2023
* Drop all post-terminal commands * Sort outgoing jobs in order lang expects
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was changed
Drop & warn about commands following a terminal workflow command.
Also ensure jobs are sorted in the order langs are expecting anyways before sending them out.
Why?
We'll end up blowing up server-side right now, which is a bit annoying for a user to fix when we can do this instead - and not require lang layer to do it.
De-dupe lang job-sorting work and move it into core.
Checklist
Closes [Feature Request] Discard post-terminal non-query commands #497 and closes [Feature Request] Sort activation jobs by desired execution order #498
How was this tested:
Added UTs. Existing tests.
Any docs updates needed?