[Fix-16978][Master] Fix AbstractDelayEvent compare method is incorrect

[reele]

Purpose of the pull request

Fix AbstractDelayEventBus block events issue fix #16978.

Brief change log

1. Fix AbstractDelayEvent compare method is incorrect
2. Modify TaskFailureStateAction to allow killEventAction when task instance can retry.

Verify this pull request

Stop a workflow with failed task when waiting to retry.

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

[ruanwenjun]

Do we need to do this change? or this is only your test code?

[reele]

it's not test code, before this pr, the kill event always triggered after retry event, so the kill or start(retry) action always fire on submitted state

this code use to finish the workflow's Topology graph.

[ruanwenjun]

Ok, I got you.
Please use

if (taskExecutionRunnable.isTaskInstanceCanRetry()
                && workflowExecutionGraph.isTaskExecutionRunnableActive(taskExecutionRunnable)) {

to replace

if (taskExecutionRunnable.isTaskInstanceCanRetry()
        // && workflowExecutionGraph.isTaskExecutionRunnableActive(taskExecutionRunnable)
        ) 

You can add a new private method for this, this logic also exist in killedEventAction

private boolean isTaskRetrying(
            final IWorkflowExecutionRunnable workflowExecutionRunnable,
            final ITaskExecutionRunnable taskExecutionRunnable) {
        final IWorkflowExecutionGraph workflowExecutionGraph = taskExecutionRunnable.getWorkflowExecutionGraph();
        return taskExecutionRunnable.isTaskInstanceCanRetry()
                && workflowExecutionGraph.isTaskExecutionRunnableActive(taskExecutionRunnable);
    }

[reele]

You can add a new private method for this, this logic also exist in killedEventAction

sure.

but i need test workflowExecutionGraph.isTaskExecutionRunnableActive(taskExecutionRunnable) more for some case

[reele]

i'm a bit confused about this:
when task is failed and going to waiting retry, this task was not removed from WorkflowExecutionGraph.activeTaskExecutionRunnable, activeTaskExecutionRunnable.add() in isTaskExecutionRunnableActive will always return false, this result prevented the execution of if-block.

or WorkflowExecutionGraph.isTaskExecutionRunnableActive should call activeTaskExecutionRunnable.contains instead of activeTaskExecutionRunnable.add() ?

[ruanwenjun]

Yes, this is another bug, we should use contains rather than add here. You can fix this in this PR or submib another pr.

[ruanwenjun]

BTW, it's better to add a judge method isTaskRetrying (ITaskExecutionRunnable taskExecutionRunnable) at IWorkflowExecutionGraph.

public boolean isTaskExecutionRunnableRetrying(final ITaskExecutionRunnable taskExecutionRunnable) {
        if (!taskExecutionRunnable.isTaskInstanceInitialized()) {
            return false;
        }
        final TaskInstance taskInstance = taskExecutionRunnable.getTaskInstance();
        return taskInstance.getState() == TaskExecutionStatus.FAILURE && taskExecutionRunnable.isTaskInstanceCanRetry() && isTaskExecutionRunnableActive(taskExecutionRunnable);
    }

[reele]

ok!

[ruanwenjun]

BTW, we are missing test cases in WorkflowInstanceStopTestCase, it's better to add a case kill a workflow which contains retry task.

[reele]

BTW, we are missing test cases in WorkflowInstanceStopTestCase, it's better to add a case kill a workflow which contains retry task.

sure, i'll do that.