You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "ZhongJinHacker (via GitHub)" <gi...@apache.org> on 2024/03/19 16:03:09 UTC
[PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
ZhongJinHacker opened a new pull request, #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741
<!--Thanks very much for contributing to Apache DolphinScheduler, we are happy that you want to help us improve DolphinScheduler! -->
## Purpose of the pull request
This pull request to fix #15740 , fix the method return incorrect value problem.
please check the issue out for details
<!--(For example: This pull request adds checkstyle plugin).-->
## Brief change log
<!--*(for example:)*
- *Add maven-checkstyle-plugin to root pom.xml*
-->
## Verify this pull request
<!--*(Please pick either of the following options)*-->
This pull request is code cleanup without any test coverage.
*(or)*
This pull request is already covered by existing tests, such as *(please describe tests)*.
(or)
This change added tests and can be verified as follows:
<!--*(example:)*
- *Added dolphinscheduler-dao tests for end-to-end.*
- *Added CronUtilsTest to verify the change.*
- *Manually verified the change by testing locally.* -->
(or)
If your pull request contain incompatible change, you should also add it to `docs/docs/en/guide/upgrede/incompatible.md`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #15741: [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value
URL: https://github.com/apache/dolphinscheduler/pull/15741
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "rickchengx (via GitHub)" <gi...@apache.org>.
rickchengx commented on code in PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#discussion_r1538560909
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteRunnable.java:
##########
@@ -908,7 +908,7 @@ private boolean executeTask(TaskInstance taskInstance) {
// todo: remove this method
if (!processService.submitTask(workflowInstance, taskInstance)) {
log.error("Submit standby task: {} failed", taskInstance.getName());
- return true;
+ return false;
Review Comment:
It seems that it should return false if it submits to DB failed, not sure if there are other considerations.
but there is also a todo here. cc @ruanwenjun
will we remove this method or fix this logic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#issuecomment-2272390995
This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#issuecomment-2008560167
Right now, pause task will also use this method to submit, and should return false, this change will influence the whole process. Is there a case to reproduce this bug?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#discussion_r1545721563
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteRunnable.java:
##########
@@ -908,7 +908,7 @@ private boolean executeTask(TaskInstance taskInstance) {
// todo: remove this method
if (!processService.submitTask(workflowInstance, taskInstance)) {
log.error("Submit standby task: {} failed", taskInstance.getName());
- return true;
+ return false;
Review Comment:
It's better not to change this logic unless we find a bug related to this issue. When the workflow instance is READY_STOP, it will also submit the task, but will return false here to avoid dispatch the task.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#issuecomment-2257235864
This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "ZhongJinHacker (via GitHub)" <gi...@apache.org>.
ZhongJinHacker commented on PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#issuecomment-2009570063
to executeTask method's logic, I can understand the logic intention. Only executeTask success will return true, and if write the taskInstance to db fail or throw Exception will return false.
But in the follow code:
it is incorrect. the log ("Submit standby task: {} failed")also can express this. when when taskInstance write to db unsuccessfully.`processService.submitTask` will return false, it let into the code block. and return `true` . this incorrect boolean value will cause the out layer caller who call `executeTask` method think the task is executed successfully, but in the fact, the task do not write into database's table
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "ZhongJinHacker (via GitHub)" <gi...@apache.org>.
ZhongJinHacker commented on PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#issuecomment-2009586823
if `taskInstanceDao.submitTaskInstanceToDB` do not success, the taskinstance will do not submit to db.
and return false.here is correct. But its caller `executeTask` will return true in last version code, it will cause this problem
you can use my unit test to reproduce this bug, or close the db or simulate the db Fault when call submitTask in debug mode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bug] [dolphinscheduler-master] fix WorkflowExecuteRunnable's executeTask method return incorrect value [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #15741:
URL: https://github.com/apache/dolphinscheduler/pull/15741#discussion_r1545721563
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteRunnable.java:
##########
@@ -908,7 +908,7 @@ private boolean executeTask(TaskInstance taskInstance) {
// todo: remove this method
if (!processService.submitTask(workflowInstance, taskInstance)) {
log.error("Submit standby task: {} failed", taskInstance.getName());
- return true;
+ return false;
Review Comment:
It's better not to change this logic unless we find a bug related to this issue, when the workflow instance is READY_STOP, it will also submit the task, but will return false here to avoid dispatch the task.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org