You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "ThreeFire1 (via GitHub)" <gi...@apache.org> on 2026/10/15 03:49:49 UTC
[PR] [Fix-18707][Master] Keep task success retryable after persistence failure [dolphinscheduler]
ThreeFire1 opened a new pull request, #18708:
URL: https://github.com/apache/dolphinscheduler/pull/18708
## Was this PR generated or assisted by AI?
YES. I used AI assistance for the investigation, code changes, regression tests, and PR description. I reviewed the issue description, the proposed fix, and the reported before-and-after test results.
## Purpose of the pull request
Fixes #18707.
When persisting a task success result throws, the shared task instance is already marked `SUCCESS`. A retried success event is then skipped and can be acknowledged while the database still contains `RUNNING_EXECUTION`.
The old `TaskResultEventHandler` handled this class of failure after #10866 (for #10854) by restoring the task instance when persistence threw. Source comparison shows that this protection was not retained in the state-machine success path introduced in #16327. This PR restores the same retry guarantee by persisting a copy before updating the shared instance, rather than restoring it after an exception.
## Brief change log
- Persist the success state, end time, and merged output variables using a copy of the task instance, then update the shared instance after the DAO call returns normally.
- Keep the persisted success state when sending the ACK fails; a repeated report can be acknowledged without persisting or publishing the topology event again.
- Add regression tests for persistence failure and recovery, repeated reports, output variables, event ordering, ACK send failure, and an uncertain commit result.
- Add a Master integration test with the real DAO and an isolated H2 database, verifying that retrying a failed success update lets a serial workflow finish.
The DAO's boolean return handling is unchanged. This patch does not add database read-back checks or broaden retry exception handling.
## Verify this pull request
This change added `TaskSuccessPersistenceRecoveryTest`. It uses the real state actions and success handler, a mocked DAO with an independent persisted snapshot, and the real event bus for the retry case. The ACK failure case uses the real `TaskExecutorClient` with a mocked transport delegator.
`TaskSuccessPersistenceRecoveryTestCase` starts a serial workflow A → B through the existing Master integration test framework. A DAO spy throws once before A's first success update; all other calls use the real DAO. The test verifies the database state before failure, two attempts to persist A's success, exactly two successful task instances, workflow completion, and release of Master resources. No manual event replay or Master restart is used.
With the unmodified production code and a clean build, this test fails: the retry skips A's success event and the workflow remains `RUNNING_EXECUTION`. With the fix, the integration test passes.
Run the focused tests with:
```sh
./mvnw -B -ntp -pl dolphinscheduler-master -am \
-Dtest=TaskSuccessPersistenceRecoveryTest,TaskSuccessLifecycleEventHandlerTest,TaskForceSuccessStateActionTest,TaskSuccessPersistenceRecoveryTestCase \
-Dsurefire.failIfNoSpecifiedTests=false clean test
```
A clean local build on JDK 17 passed all 17 selected tests in both configured Surefire executions: 10 new recovery tests, 1 new Master integration test, 5 existing success handler tests, and 1 existing forced-success test. The 115-module dependency reactor completed successfully with Spotless checks and JaCoCo instrumentation enabled. `spotless:apply` was also run before testing.
The integration test uses real H2 persistence with an injected DAO exception; it does not exercise a real database outage, database-specific exception translation, or a deployed cluster. Local testing was limited to the 17 tests listed above; the full project test suite was not run.
## Pull Request Notice
[Pull Request Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.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] [Fix-18707][Master] Keep task success retryable after persistence failure [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS merged PR #18708:
URL: https://github.com/apache/dolphinscheduler/pull/18708
--
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] [Fix-18707][Master] Keep task success retryable after persistence failure [dolphinscheduler]
Posted by "ThreeFire1 (via GitHub)" <gi...@apache.org>.
ThreeFire1 commented on PR #18708:
URL: https://github.com/apache/dolphinscheduler/pull/18708#issuecomment-6188397123
> Other persistent events need to be handled in the same way. A utility method could be created here to handle them uniformly; however, to ensure recovery in the event of a database operation failure, all relevant sections of the program must be checked.
Thanks for the feedback! The local changes now cover the other task lifecycle updates through a shared helper that persists a copy before updating the shared instance.
I’m checking the related recovery paths and test coverage before pushing the update here.
--
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] [Fix-18707][Master] Keep task success retryable after persistence failure [dolphinscheduler]
Posted by "ThreeFire1 (via GitHub)" <gi...@apache.org>.
ThreeFire1 commented on PR #18708:
URL: https://github.com/apache/dolphinscheduler/pull/18708#issuecomment-6192597468
@ruanwenjun Updated this PR to use a shared helper across the nine task lifecycle update paths. The tests cover failed database updates followed by retry and recovery, repeated reports, ACK failures, output variables, and task-group resource release. The Master/H2 tests also verify that workflows complete or reach the expected stopped/failed state after the injected update failure. These tests pass on JDK 8 and 11.
The broader review also found existing transaction-related recovery issues in workflow completion and Retry/Failover instance creation. These need changes beyond the current helper. Would you prefer to address them in separate follow-up PRs?
--
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] [Fix-18707][Master] Keep task success retryable after persistence failure [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on PR #18708:
URL: https://github.com/apache/dolphinscheduler/pull/18708#issuecomment-6207435164
> @ruanwenjun Updated this PR to use a shared helper across the nine task lifecycle update paths. The tests cover failed database updates followed by retry and recovery, repeated reports, ACK failures, output variables, and task-group resource release. The Master/H2 tests also verify that workflows complete or reach the expected stopped/failed state after the injected update failure. These tests pass on JDK 8 and 11.
>
> The broader review also found existing transaction-related recovery issues in workflow completion and Retry/Failover instance creation. These need changes beyond the current helper. Would you prefer to address them in separate follow-up PRs?
@ThreeFire1 Thanks for finding out this., use a separate PR LGTM.
--
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] [Fix-18707][Master] Keep task success retryable after persistence failure [dolphinscheduler]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #18708:
URL: https://github.com/apache/dolphinscheduler/pull/18708#issuecomment-6183966010
Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.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] [Fix-18707][Master] Keep task success retryable after persistence failure [dolphinscheduler]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #18708:
URL: https://github.com/apache/dolphinscheduler/pull/18708#issuecomment-6210324780
Awesome work, congrats on your first merged 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