SEPURI-SAI-KRISHNA opened a new pull request, #18539:
URL: https://github.com/apache/dolphinscheduler/pull/18539
<!--Thanks very much for contributing to Apache DolphinScheduler, we are
happy that you want to help us improve DolphinScheduler! -->
## Was this PR generated or assisted by AI?
YES. The faulty expression was located and the fix and unit tests were
drafted with AI
assistance (Claude Code); the reasoning, the failover impact analysis and
the final code
were reviewed and verified by me.
## Purpose of the pull request
Closes #18538.
`TaskRetryLifecycleEvent#of` computes the delay before a failed task is
retried. The
intent is to retry at `endTime + retryInterval`, so the remaining delay is
`retryInterval - (now - endTime)`. The expression instead evaluated to
`retryInterval + (now - endTime)` — the elapsed time was added rather than
subtracted:
```java
final long remainingTime =
TimeUnit.MINUTES.toMillis(delayTime) + System.currentTimeMillis() -
taskInstance.getEndTime().getTime();
```
In the common path `now - endTime` is a few milliseconds, so the error is
invisible. It
matters when the retry event is created long after the task actually ended —
most
notably on **master failover**: `WorkflowFailoverCommandHandler` rebuilds
the execution
graph from the existing task instances, so a task left in `FAILURE` with
retries
remaining keeps its old `endTime`; `TaskFailureStateAction#onStartEvent`
then republishes
the failure event with that stale `endTime`, and the retry gets postponed by
the whole
outage duration on top of the configured interval instead of firing
immediately.
## Brief change log
- `TaskRetryLifecycleEvent#of`: subtract the elapsed time since `endTime`
from the retry
interval, and clamp the result to `0` so an already-overdue retry is
triggered
immediately.
- Renamed the local `delayTime` to `retryInterval` — it holds the configured
retry
interval and shadowed the inherited `AbstractDelayEvent#delayTime` field,
which is what
made the wrong expression easy to miss.
- Added `TaskRetryLifecycleEventTest`.
## Verify this pull request
This change added tests and can be verified as follows:
- Added `TaskRetryLifecycleEventTest` with three cases:
- retry interval **not** elapsed → the event is delayed for the remaining
interval;
- retry interval **already** elapsed (the failover case, `endTime` two
hours in the
past) → the event is ready immediately. This case fails on `dev` with a
delay of
~2h05m;
- retry times exhausted → `IllegalStateException`, guarding the existing
`checkState`.
```bash
./mvnw -pl dolphinscheduler-master -am clean test \
-Dtest=TaskRetryLifecycleEventTest \
-Dsurefire.failIfNoSpecifiedTests=false
```
Verified locally:
- The new tests fail on `dev` and pass with this change. On `dev` the
failover case reports
a delay of `7499999` ms (~2h05m) where `0` is expected, and the normal
case reports
`330016` ms where at most `270000` ms is expected.
- The full `dolphinscheduler-master` suite was run (98 tests). The only
non-passing test was
`WorkflowStartTimeoutTestCase#testStartWorkflow_withTimeoutWarnFailedTask`,
which is
unrelated timing flakiness under the 4-way parallel forks: its fixture
defines no
`failRetryTimes`, so `maxRetryTimes` is `0`, `isTaskInstanceCanRetry()` is
false and
`TaskRetryLifecycleEvent#of` is never reached. The class passes 5/5 when
re-run in
isolation.
## Pull Request Notice
[Pull Request
Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
If your pull request contains incompatible change, you should also add it to
`docs/docs/en/guide/upgrade/incompatible.md`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
