ruanwenjun commented on PR #18722: URL: https://github.com/apache/dolphinscheduler/pull/18722#issuecomment-6282744889
Could we simplify `workflowFinish()` around the current lifecycle-event dispatch path, which does not have an enclosing transaction, instead of introducing `WorkflowFinishTransactionSynchronization`? The flow could be: 1. Copy the shared `WorkflowInstance`. 2. Inside `transactionTemplate.execute()`, update the copy's final state/end time, persist it, and delete the serial command when applicable. 3. After `execute()` returns successfully, update the shared instance, record the completion metric, and publish the finalize event. Keep the metric exception isolated so it cannot prevent finalize publication. The database-operation logs can stay inside `execute()`, which also avoids returning a `serialCommandDeleted` flag just for logging. Their wording should describe operations executed within the transaction, rather than claim that the transaction has committed. Any final completion log should remain after the transaction returns. For this scope, I do not think an additional `isActualTransactionActive()` guard is necessary; we can document the existing transaction boundary. This would remove the synchronization scan, custom synchronization class, and `completionAttempted` flag while retaining rollback/retry coverage for the current event-dispatch path. This is deliberately a narrower contract, not an equivalent refactor: it would not retain the additional guarantees for an enclosing `REQUIRED` transaction or for another `afterCommit` callback throwing after the database has committed. Could we scope the implementation and tests accordingly, while retaining the rollback/retry and real-event-bus serial-queue recovery tests? -- 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]
