add foreach.RunParallel option #597
Conversation
| } | ||
|
|
||
| if (context.PersistenceData is ControlPersistenceData) | ||
| if (context.PersistenceData is IteratorPersistenceData persistanceData && persistanceData?.ChildrenActive == true) |
There was a problem hiding this comment.
We need to remain backwards compatible... there may be live workflows out there with ForEach steps that have ControlPersistenceData persisted against them, so we still need to support that.
There was a problem hiding this comment.
you are right! I'll check instead for ControlPersistenceData and only for IteratorPersistenceData if .RunParallel is set to False
There was a problem hiding this comment.
You could also test what the type is, then we simplify by not maintaining 2 persistence types moving forward.
There was a problem hiding this comment.
I hope my latest push matches what you had in mind... I did not want to overcomplicate the code so I kept the two separated.
| @@ -306,6 +306,25 @@ private WorkflowStep IterateParents(int id, string name) | |||
| return stepBuilder; | |||
| } | |||
|
|
|||
| public IContainerStepBuilder<TData, Foreach, Foreach> ForEach(Expression<Func<TData, IEnumerable>> collection, Expression<Func<TData, bool>> runParallel) | |||
There was a problem hiding this comment.
I wonder if we should add runParallel as an optional parameter to the existing methods? Thoughts?
There was a problem hiding this comment.
of course that would be a nice global feature but I am not sure how would work in a .While (that does not actually pass any Context.Item) - that would require more refactoring and could be done in a separate PR.
Also I was thinking of somehow passing the .Index property (like in a for loop instead a foreach) - sometimes might be useful to know which item index is actually being processed (i.e. first, last, somewhere in between...); but that would require a separate .Context.ItemOrder property which I did not want to add to the base class.
| WaitForWorkflowToComplete(workflowId, TimeSpan.FromSeconds(30)); | ||
|
|
||
| GetStatus(workflowId).Should().Be(WorkflowStatus.Complete); | ||
| GetData(workflowId).Counter.Should().Be(3); |
There was a problem hiding this comment.
Can we assert that it was sync?
There was a problem hiding this comment.
only way I could find to test it is with the Sleep and Counter - the test will fail is .runParallel is set to True. Do you have any better ideas how to actually Assert that?
I've seen multiple questions about the ForEach parallel execution and in my use case the TData object defines whether it should run in parallel or synchronous (think a chain of emails for approvals), so I've played around Decide and ForEach vs While but I found it much easier to simply extend the existing ForEach Primitive with a .RunParallel boolean (by default set to True to keep it backward compatible). I think it's a nice addition to this otherwise great library and maybe beneficial to others as well.
I've added the Mongo serialization (only one tested since that's what I use - could not get the other DB Provider tests to run), a new Sample and a link in the readme.
I hope this satisfies your PR requirements and I do hope to see this soon released (obviously I am open to any feedback!)
Thanks in advance