Skip to content

WorkflowOptions support for configuring max number of concurrent tasks. #287

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

Merged
merged 2 commits into from
Apr 7, 2019

Conversation

eddie3716
Copy link
Contributor

No description provided.

@SebastienLagrange
Copy link

Hello,
I am very happy to see that you implemented this change that I was just thinking on !
I implement workflows that contains lot of steps that actually are waiting for external resources. And I start hundreds of workflows at the same time. The current implementation limiting the number of tasks to the number of CPU is very limiting the performance to process all the hundreds of workflows... and I see my CPU is zero all the time because the steps actually call REST services that are on other server so the workflow engine is just waiting all the time. In that case it would possible to start 50 workflows at the same time even with my 2 CPU, and that would really improve the time to process all the hundreds of workflows...
daniel do you think this modification is good, will you accept the pullrequest and make this new option available from your master package ?
Thank you all !

@@ -67,6 +70,16 @@ public void UseErrorRetryInterval(TimeSpan interval)
{
ErrorRetryInterval = interval;
}

public void UseMaxConcurrentItems(int? maxConcurrentItems)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should make this explicitly the max number of concurrent workflows rather than items?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me make that change you've suggested. Most consumers of your API may not ever dig into the internals, and thus not know what maxConcurrentItems really means, but framing it as "max number of concurrent workflows" is more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of modifying EventConsumer directly, I'll move the change to WorkflowConsumer...that way the other child consumers can keep their default behavior the way you intended.

@@ -13,7 +13,7 @@ internal class EventConsumer : QueueConsumer, IBackgroundTask
private readonly IPersistenceProvider _persistenceStore;
private readonly IDistributedLockProvider _lockProvider;
private readonly IDateTimeProvider _datetimeProvider;
protected override int MaxConcurrentItems => 2;
protected override int MaxConcurrentItems => WorkflowOptions.MinimumNumberOfConcurrentItems;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense to me... is this a typo?

Copy link
Contributor Author

@eddie3716 eddie3716 Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was intentional, because MaxConcurrentItems as a function of the number 2 was showing in two places, the EventConsumer and QueueConsumer. And then I added a 3rd place it where it shows up in the WorkflowOptions class. I was concerned the number 2 was becoming a magic number duplicated in too many locations, and so an explicitly named constant helps to maintain the context of what the value 2 is for. This change doesn't add a ton of value, so I can revert it if its problematic.

@danielgerlag danielgerlag merged commit 8119284 into danielgerlag:master Apr 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants