Skip to content

[fix] Fix the SERIAL_DISCARD execution_type not working#10258

Merged
caishunfeng merged 2 commits intoapache:devfrom
cadl:fix-serial-discard
May 30, 2022
Merged

[fix] Fix the SERIAL_DISCARD execution_type not working#10258
caishunfeng merged 2 commits intoapache:devfrom
cadl:fix-serial-discard

Conversation

@cadl
Copy link
Contributor

@cadl cadl commented May 26, 2022

Purpose of the pull request

I found the SERIAL_DISCARD execution_type always stops the process instance, even if there were no running process instances.

After diving into the code, It seems like some logic error. Before the fix, the ProcessServiceImpl will stop the current process_instance, if running_instances is empty.

Now make some fixes, if running_instances is not empty, mark the current process_instance as stop (discard it). if running_instances is empty, submit the current process_instance.

Brief change log

fix SERIAL_DISCARD not working

Verify this pull request

@cadl cadl changed the title fix the SERIAL_DISCARD execution_type not working [fix] Fix the SERIAL_DISCARD execution_type not working May 26, 2022
@SbloodyS SbloodyS added bug Something isn't working first time contributor First-time contributor backend labels May 27, 2022
@SbloodyS SbloodyS added this to the 3.0.0-beta-2 milestone May 27, 2022
@cadl
Copy link
Contributor Author

cadl commented May 27, 2022

Hi @SbloodyS

Already changed unit test. Please re-run ci workflows 🙏

@SbloodyS
Copy link
Member

Hi @SbloodyS

Already changed unit test. Please re-run ci workflows 🙏

I rerun it.

@codecov-commenter
Copy link

Codecov Report

Merging #10258 (2be97eb) into dev (4c1ef0a) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##                dev   #10258      +/-   ##
============================================
- Coverage     41.25%   41.24%   -0.02%     
+ Complexity     4760     4758       -2     
============================================
  Files           854      854              
  Lines         34579    34582       +3     
  Branches       3821     3821              
============================================
- Hits          14265    14262       -3     
- Misses        18944    18948       +4     
- Partials       1370     1372       +2     
Impacted Files Coverage Δ
...nscheduler/service/process/ProcessServiceImpl.java 30.62% <50.00%> (-0.08%) ⬇️
...er/master/dispatch/host/assign/RandomSelector.java 77.77% <0.00%> (-5.56%) ⬇️
...dolphinscheduler/remote/future/ResponseFuture.java 81.96% <0.00%> (-1.64%) ⬇️
...r/plugin/task/sqoop/parameter/SqoopParameters.java 53.33% <0.00%> (-1.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c1ef0a...2be97eb. Read the comment docs.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

66.7% 66.7% Coverage
0.0% 0.0% Duplication

@SbloodyS SbloodyS requested a review from caishunfeng May 27, 2022 06:56
@SbloodyS
Copy link
Member

@caishunfeng Do you have time take a look?

Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM, @cadl thanks for your contribution.

@caishunfeng caishunfeng merged commit 13af2ad into apache:dev May 30, 2022
@cadl cadl deleted the fix-serial-discard branch May 30, 2022 01:55
@devosend devosend modified the milestones: 3.0.0-beta-2, 3.1.0-alpha Jun 16, 2022
ITBOX-ITBOY pushed a commit to ITBOX-ITBOY/dolphinscheduler that referenced this pull request Jul 8, 2022
* fix always stop process_instance when execution_type is SERIAL_DISCARD

* test: no running_process, submit it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend bug Something isn't working first time contributor First-time contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants