[Improvement-17157][Master] Support setting max.concurrent.workflow.instances #17159

tusaryan · 2025-05-01T14:49:20Z

Purpose of the pull request

This Pull Request implements a new configuration option max.concurrent.workflow.instances in the master server, allowing administrators to set a limit on the number of concurrently running workflow instances on a single master. This addresses the issue of potential overload when multiple workflows failover to a single master, as described in #17157.

close #17157

Brief change log

Added a new configuration property master.server-load-protection.max-concurrent-workflow-instances in application.yaml under the server-load-protection section. This property is optional and defaults to 2147483647 (no limit) if not set. To set specific limit, Eg:
master.server-load-protection.max-concurrent-workflow-instances=100 #Set your desired limit here
Replace 100 with any integer value you want as the limit.
Modified MasterServerLoadProtection.java to read and utilize the new configuration property.
Changed from using WorkflowCacheRepository to IWorkflowRepository for better abstraction and testability.
Implemented logic in MasterServerLoadProtection to check the current count of running workflow instances against the configured limit.
Updated the isOverload method in MasterServerLoadProtection to return true if the concurrent workflow instance count exceeds the configured maximum, marking the master as busy.
Updated MasterServerLoadProtectionConfig.java to properly inject IWorkflowRepository into the MasterServerLoadProtection instance.
Updated MasterServerLoadProtectionTest.java to include a test case verifying the new functionality where the master is marked as overloaded when the concurrent workflow instance count exceeds the configured limit.

Verify this pull request

This change added tests and can be verified as follows:

Updated MasterServerLoadProtectionTest.java with a new test method (isOverloadWithMaxConcurrentWorkflowInstances) to specifically test the behavior when the number of running workflow instances reaches or exceeds the configured max.concurrent.workflow.instances limit.
Explanation of test verification::
- When the workflow count (5) is less than the max limit (10), the master is not overloaded
- When the workflow count (5) equals the max limit (5), the master is marked as overloaded
- When the workflow count (5) exceeds the max limit (3), the master is marked as overloaded

fix issue: [Improvement][Master] Support set max.concurrent.workflow.instances in master #17157

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

sonarqubecloud · 2025-05-05T10:38:04Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ruanwenjun

LGTM, and it's better to expose the activeWorkflowInstanceCount in MasterHeartBeat.

...c/main/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtection.java

tusaryan · 2025-05-07T21:11:17Z

Can anyone review the changes again

...c/main/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtection.java

ruanwenjun · 2025-05-08T03:57:33Z

.../java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtectionConfig.java

+    @PostConstruct
+    public void init() {
+        MasterServerLoadProtection serverLoadProtection = masterConfig.getServerLoadProtection();
+        serverLoadProtection.setWorkflowRepository(workflowRepository);


It's better to inject at the constructor.

Is this resolved? Why mark this as resolved?

...c/main/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtection.java

Copilot

Pull Request Overview

This PR introduces a new configuration option "max.concurrent.workflow.instances" to limit the number of concurrently running workflow instances on a single master. Key changes include adding a new property in the YAML configuration, updating MasterServerLoadProtection to use IWorkflowRepository for counting workflow instances, and incorporating tests to verify overload behavior.

Added test cases in MasterServerLoadProtectionTest.java to validate overload conditions.
Updated MasterServerLoadProtection.java to enforce the new concurrent workflow limit.
Modified MasterServerLoadProtectionConfig.java to inject the IWorkflowRepository dependency.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
dolphinscheduler-master/src/test/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtectionTest.java	Added test cases for the overload behavior with the new configuration option.
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtectionConfig.java	Updated to inject IWorkflowRepository into the load protection component.
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtection.java	Integrated the new "max.concurrent.workflow.instances" property checking and logging for overload conditions.

Comments suppressed due to low confidence (1)

dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtection.java:52

[nitpick] The log message contains a minor typo with 'over then' which could be corrected to 'exceeds' for better clarity.

log.info("OverLoad: the workflow instance count: {} is over then the maxConcurrentWorkflowInstances {}", currentWorkflowInstanceCount, maxConcurrentWorkflowInstances);

ruanwenjun · 2025-05-10T04:48:53Z

Please solve the code check style by mvn spotless:apply

tusaryan · 2025-05-10T08:37:27Z

I wanted to let you know that I'm currently traveling. Because of this, I might be a bit delayed in addressing the feedback on this pull request. I will definitely get to it as soon as things settle down for me.

tusaryan · 2025-05-16T06:37:54Z

Please solve the code check style by mvn spotless:apply

Code style issues have been fixed by running mvn spotless:apply. Please let me know if there's anything else I can do!

ruanwenjun

LGTM

ruanwenjun · 2025-05-27T01:36:11Z

.../java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtectionConfig.java

+    @Bean
+    public MasterServerLoadProtection masterServerLoadProtection(
+                                                                 IWorkflowRepository workflowRepository,
+                                                                 @Value("${master.server-load-protection.max-concurrent-workflow-instances:2147483647}") int maxConcurrentWorkflowInstances) {


We need to add this config in doc, and you need to set other fields in MasterServerLoadProtection, e.g. maxSystemCpuUsagePercentageThresholds, maxJvmCpuUsagePercentageThresholds.

We need to add this config in doc, and you need to set other fields in MasterServerLoadProtection, e.g. maxSystemCpuUsagePercentageThresholds, maxJvmCpuUsagePercentageThresholds.

Thanks for the feedback! I’ll get to it as soon as possible.

Sorry for the delay. I will update it within 1-2 days.

apply mvn spotless:apply

@bean

- Refactored MasterServerLoadProtection to require IWorkflowRepository and maxConcurrentWorkflowInstances via constructor injection. - Updated MasterServerLoadProtectionConfig to provide these dependencies using Spring @bean and @value with a default of 2147483647. - Removed direct instantiation of MasterServerLoadProtection in MasterConfig to avoid missing constructor arguments.

tusaryan requested review from caishunfeng, SbloodyS and ruanwenjun as code owners May 1, 2025 14:49

github-actions bot added backend test labels May 1, 2025

github-actions bot assigned tusaryan May 1, 2025

tusaryan marked this pull request as draft May 1, 2025 14:50

tusaryan marked this pull request as ready for review May 1, 2025 14:50

SbloodyS changed the title ~~[Fix-17157][Master] Support setting max.concurrent.workflow.instances~~ [Improvement-17157][Master] Support setting max.concurrent.workflow.instances May 5, 2025

SbloodyS added the improvement make more easy to user or prompt friendly label May 5, 2025

SbloodyS added this to the 3.3.1 milestone May 5, 2025

ruanwenjun reviewed May 6, 2025

View reviewed changes

tusaryan requested a review from ruanwenjun May 7, 2025 21:29

ruanwenjun reviewed May 8, 2025

View reviewed changes

tusaryan requested a review from ruanwenjun May 8, 2025 08:53

yingh0ng reviewed May 9, 2025

View reviewed changes

...c/main/java/org/apache/dolphinscheduler/server/master/config/MasterServerLoadProtection.java Show resolved Hide resolved

nielifeng requested a review from Copilot May 9, 2025 09:44

Copilot AI reviewed May 9, 2025

View reviewed changes

ruanwenjun force-pushed the improvement/17157-master-concurrent-limit branch from 0e05e2b to 8b5361f Compare May 16, 2025 10:24

ruanwenjun approved these changes May 22, 2025

View reviewed changes

ruanwenjun force-pushed the improvement/17157-master-concurrent-limit branch from bd4d946 to 4fdac71 Compare May 27, 2025 01:27

ruanwenjun reviewed May 27, 2025

View reviewed changes

tusaryan and others added 3 commits June 9, 2025 09:34

[Fix-17157][Master] Support setting max.concurrent.workflow.instances

693fec7

Update MasterServerLoadProtection.java

7937ce4

Update MasterServerLoadProtectionTest.java

897031a

tusaryan and others added 8 commits June 9, 2025 09:34

Update MasterServerLoadProtectionConfig.java

8b2c48c

Update MasterServerLoadProtection.java

013d9f3

Update MasterServerLoadProtectionConfig.java

20cd0bf

Update MasterServerLoadProtection.java

2f3f694

Update MasterServerLoadProtectionConfig.java

9475b0b

apply mvn spotless:apply

fix typo in MasterServerLoadProtection

4b27191

Apply spotless formatting

3f0ec17

ruanwenjun force-pushed the improvement/17157-master-concurrent-limit branch from 4fdac71 to 3f0ec17 Compare June 9, 2025 01:34

[Improvement-17157][Master] Support setting max.concurrent.workflow.instances #17159

Are you sure you want to change the base?

[Improvement-17157][Master] Support setting max.concurrent.workflow.instances #17159

Uh oh!

Conversation

tusaryan commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of the pull request

Brief change log

Verify this pull request

Pull Request Notice

Uh oh!

sonarqubecloud bot commented May 5, 2025

Quality Gate passed

Uh oh!

ruanwenjun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tusaryan commented May 7, 2025

Uh oh!

Uh oh!

ruanwenjun May 8, 2025

Choose a reason for hiding this comment

Uh oh!

ruanwenjun May 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

ruanwenjun commented May 10, 2025

Uh oh!

tusaryan commented May 10, 2025

Uh oh!

tusaryan commented May 16, 2025

Uh oh!

ruanwenjun left a comment

Choose a reason for hiding this comment

Uh oh!

ruanwenjun May 27, 2025

Choose a reason for hiding this comment

Uh oh!

tusaryan May 27, 2025

Choose a reason for hiding this comment

Uh oh!

tusaryan Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tusaryan commented May 1, 2025 •

edited

Loading