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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
