Skip to content
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

[fix-#13045] fix endless loop #15092

Merged
merged 4 commits into from
Nov 8, 2023
Merged

[fix-#13045] fix endless loop #15092

merged 4 commits into from
Nov 8, 2023

Conversation

fuchanghai
Copy link
Member

Purpose of the pull request

This PR is complementary to issue #13045

PR #13051

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

@fuchanghai
Copy link
Member Author

@zhuangchong PTAL. cc @huage1994

processInstance.getState(), workflowStateEvent.getStatus());

if (workflowExecuteRunnable.processComplementData()) {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Will this patch fix the endless loop when completing data?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will this patch fix the endless loop when completing data?

yes ,in #13051 ,forgot to add the corresponding event processing class, resulting in the status of the process instance not being updated

Copy link
Member

Choose a reason for hiding this comment

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

I'm still a little confused here. If a workflow instance fails to get submitted in data complement mode, in line 45, we directly return true without doing anything else to it. What will the state of the workflow instance become? Failure or just hanging there?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still a little confused here. If a workflow instance fails to get submitted in data complement mode, in line 45, we directly return true without doing anything else to it. What will the state of the workflow instance become? Failure or just hanging there?

You are right, whatever type, as long as an error occurs during the submission phase, the status should be updated to failure

@EricGao888 EricGao888 added bug Something isn't working 3.2.1 labels Oct 30, 2023
@EricGao888 EricGao888 added this to the 3.2.1 milestone Oct 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Merging #15092 (9989718) into dev (5e135ba) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 9989718 differs from pull request most recent head f651677. Consider uploading reports for the commit f651677 to get more accurate results

@@             Coverage Diff              @@
##                dev   #15092      +/-   ##
============================================
- Coverage     38.19%   38.17%   -0.02%     
+ Complexity     4686     4682       -4     
============================================
  Files          1280     1281       +1     
  Lines         45325    45347      +22     
  Branches       4938     4941       +3     
============================================
  Hits          17312    17312              
- Misses        26126    26148      +22     
  Partials       1887     1887              
Files Coverage Δ
...server/master/event/WorkflowStartEventHandler.java 0.00% <ø> (ø)
...ter/event/WorkflowSubmitFailStateEventHandler.java 0.00% <0.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@fuchanghai
Copy link
Member Author

@huage1994 PTAL cc @ruanwenjun

Copy link
Member

@EricGao888 EricGao888 left a comment

Choose a reason for hiding this comment

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

BTW, you need to change the log information here:

} else if (WorkflowStartStatus.FAILED == workflowStartStatus) {
log.error(
"Failed to submit the workflow instance, will resend the workflow start event: {}",
workflowEvent);
WorkflowStateEvent stateEvent = WorkflowStateEvent.builder()
.processInstanceId(processInstance.getId())
.type(StateEventType.PROCESS_SUBMIT_FAILED)
.status(WorkflowExecutionStatus.FAILURE)
.build();
workflowExecuteRunnable.addStateEvent(stateEvent);

If DS fails to submit a workflow instance, it no more resends the start event.

@fuchanghai
Copy link
Member Author

BTW, you need to change the log information here:

} else if (WorkflowStartStatus.FAILED == workflowStartStatus) {
log.error(
"Failed to submit the workflow instance, will resend the workflow start event: {}",
workflowEvent);
WorkflowStateEvent stateEvent = WorkflowStateEvent.builder()
.processInstanceId(processInstance.getId())
.type(StateEventType.PROCESS_SUBMIT_FAILED)
.status(WorkflowExecutionStatus.FAILURE)
.build();
workflowExecuteRunnable.addStateEvent(stateEvent);

If DS fails to submit a workflow instance, it no more resends the start event.

done

Copy link
Contributor

@huage1994 huage1994 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Nov 8, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
22.7% 22.7% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@EricGao888 EricGao888 merged commit 2622685 into apache:dev Nov 8, 2023
52 of 53 checks passed
@fuchanghai fuchanghai deleted the fix-##13045 branch November 30, 2023 01:17
@zhuangchong zhuangchong added the 3.1.x for 3.1.x version label Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.1.x for 3.1.x version 3.2.1 backend bug Something isn't working ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants