SEPURI-SAI-KRISHNA commented on PR #18544: URL: https://github.com/apache/dolphinscheduler/pull/18544#issuecomment-5261342776
@SbloodyS > As I said before, this is not the root cause. If there is no dirty data, this problem will not appear. I think we're disagreeing about which data counts as dirty, so I made the reproduction match the engine exactly rather than keep arguing. The test now writes the two rows the way `RetryTaskInstanceFactory` writes them: the superseded attempt gets `flag = NO`, the new attempt gets `flag = YES`. ```java // A failed attempt and the retry which replaced it, both finishing within the same second. On MySQL // t_ds_task_instance.end_time is a datetime without fractional seconds, so the two attempts end up // with exactly the same end_time. This is exactly the state RetryTaskInstanceFactory leaves behind: // the superseded attempt is flagged invalid and the new attempt is the only valid one. insertTaskInstance(EXTRACT_TASK, TaskExecutionStatus.FAILURE, sameEndTime, Flag.NO); insertTaskInstance(EXTRACT_TASK, TaskExecutionStatus.SUCCESS, sameEndTime, Flag.YES); ``` There is now exactly one *valid* task instance for that task code, which is the invariant you're describing. On `dev` the query still returns both rows: ``` TaskInstanceDaoImplTest.queryLastTaskInstanceListIntervalInWorkflowInstanceWhenAttemptsShareTheSameEndTime:86 expected: <1> but was: <2> ``` No row in that fixture is dirty. Both are written by the engine on purpose, the superseded one is correctly invalidated, and the query returns both anyway because it never looks at `flag` and because `end_time` is not unique. The old attempt also can't just be deleted, the retry history is what the task instance list shows. If your point is that the query should be honouring the `flag = YES` invariant instead of picking the newest row, I'm glad to do it that way. Both of these turn the test green: ```sql -- (a) current PR: deterministic newest attempt select max(id) as max_id from t_ds_task_instance ... group by task_code ``` ```sql -- (b) rely on the engine invariant instead ... and flag = 1 ``` I chose (a) because it holds even if the invariant is ever violated, and because the singular sibling in the same mapper, `findLastTaskInstance`, already breaks the tie the same way with `order by end_time desc limit 1`. Happy to switch to (b), or apply both, whichever you prefer. And if you do think a second finished row for one task code shouldn't exist at all, could you point me at the factory you consider wrong? `RetryTaskInstanceFactory`, `FailoverTaskInstanceFactory` and `FailedRecoverTaskInstanceFactory` all deliberately `insert` a new row instead of updating the old one, so from the code this looks like the intended design. If it isn't, that's a much larger bug than this one and I'd rather raise it separately than leave `DependentExecute` throwing in the meantime. -- 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]
