SEPURI-SAI-KRISHNA commented on PR #18544:
URL:
https://github.com/apache/dolphinscheduler/pull/18544#issuecomment-5261161869
Thanks for the review. I agree there should only ever be one *running* task
instance per task code, but the rows this query returns are not running
instances, and the duplicates are not dirty data.
**The query only looks at finished attempts.** `findLastTaskInstances`
filters `state != 8` (`NEED_FAULT_TOLERANCE`) and matches on `end_time`, so a
running instance (`end_time is null`) can never be selected. Its only caller
reaches it after the depended-on workflow has already finished:
```java
// DependentExecute#calculateResultForTasks
if (workflowInstance.getState().isSuccess()) {
...
taskInstanceDao.queryLastTaskInstanceListIntervalInWorkflowInstance(workflowInstance.getId(),
...)
```
So there is no running instance involved in this path at all.
**Multiple finished rows per task code are created by the engine on
purpose.** Retry, failover and failed-recover each `insert` a brand new
`t_ds_task_instance` row with the same `task_code` / `workflow_instance_id` and
flip the previous row to `flag = NO`:
```java
// RetryTaskInstanceFactory#createTaskInstance
final TaskInstance taskInstance = cloneTaskInstance(needRetryTaskInstance);
taskInstance.setId(null);
...
taskInstanceDao.insert(taskInstance);
needRetryTaskInstance.setFlag(Flag.NO);
taskInstanceDao.updateById(needRetryTaskInstance);
```
`FailoverTaskInstanceFactory` and `FailedRecoverTaskInstanceFactory` do the
same. That is exactly why this query exists and is called `findLast…` — it is
supposed to pick the newest of several legitimate attempts.
**The defect is the tie-break, not the data.** The query groups by
`task_code` taking `max(end_time)`, then joins back on equality:
```sql
on instance.workflow_instance_id = t_max.workflow_instance_id
and instance.task_code = t_max.task_code
and instance.end_time = t_max.max_end_time
```
`end_time` is not unique. On MySQL `t_ds_task_instance.end_time` is a plain
`datetime`, second precision, no fractional part:
```sql
`end_time` datetime DEFAULT NULL COMMENT 'task end time',
```
So a short task that fails and is retried (or failed over) inside the same
second produces two attempts with byte-identical `end_time`, both of them equal
to the max, and the join emits both. `DependentExecute` then does:
```java
.collect(Collectors.toMap(TaskInstance::getTaskCode,
TaskInstance::getState));
```
`Collectors.toMap` has no merge function, so the dependent task fails the
whole workflow with `IllegalStateException: Duplicate key`. It is a genuine
plan-level ambiguity, not corrupted rows, the same two rows are perfectly valid
to every other query.
The singular sibling in the same mapper already handles this correctly and
can't return duplicates:
```sql
<select id="findLastTaskInstance" ...>
... order by end_time desc limit 1
```
The plural version is simply inconsistent with it.
**Why `max(id)`.** Attempts are only ever created from an already-terminated
attempt (failover marks the old row `NEED_FAULT_TOLERANCE`, which this query
excludes), so the auto-increment id is a strictly monotonic attempt order and
`max(id)` is deterministic where `max(end_time)` is not. It selects the same
row as before whenever `end_time` values differ, it only makes the tied case
well-defined.
If you'd rather keep `end_time` as the primary ordering and use the id only
as a tie-break, I'm happy to change it to `max(id)` restricted to the rows
holding `max(end_time)`; the fix is one query either way. And if you still
think the duplicate rows themselves are the bug, I'd like to understand which
factory you consider wrong to insert a second finished row — from the code
above it looks intentional.
The added test reproduces it: it fails on `dev` with `expected: <1> but was:
<2>` and passes with this change.
--
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]
