Copilot commented on code in PR #61989:
URL: https://github.com/apache/doris/pull/61989#discussion_r3019861432
##########
be/src/exec/pipeline/pipeline_task.cpp:
##########
@@ -1063,7 +1063,12 @@ Status PipelineTask::_state_transition(State new_state) {
}
_task_profile->add_info_string("TaskState", _to_string(new_state));
_task_profile->add_info_string("BlockedByDependency", _blocked_dep ?
_blocked_dep->name() : "");
- if (!LEGAL_STATE_TRANSITION[(int)new_state].contains(_exec_state)) {
+ bool legal = LEGAL_STATE_TRANSITION[(int)new_state].contains(_exec_state);
+ // When _wake_up_early is true, a BLOCKED task can skip RUNNABLE and go
directly to FINISHED.
+ if (!legal && _wake_up_early && _exec_state == State::BLOCKED && new_state
== State::FINISHED) {
+ legal = true;
+ }
+ if (!legal) {
Review Comment:
Allowing BLOCKED -> FINISHED in _state_transition means a task can be
closed/finalized while it is still registered in Dependency::_blocked_task
(because Dependency::set_ready() is what drains that list). If
unblock_all_dependencies()/Dependency::set_ready() races and runs after
close_task has already transitioned the task to FINISHED/FINALIZED,
Dependency::set_ready() will call PipelineTask::wake_up(), which will attempt
FINISHED/FINALIZED -> RUNNABLE and throw; since set_ready() doesn’t catch
per-task errors, one failing wake_up can abort waking the remaining blocked
tasks for that dependency. Consider making wake_up() a no-op when the task is
already FINISHED/FINALIZED (especially when _wake_up_early is set), or make
Dependency::set_ready() robust to wake_up failures so one finished task can’t
prevent waking others.
##########
be/test/exec/pipeline/pipeline_task_test.cpp:
##########
@@ -479,6 +479,8 @@ TEST_F(PipelineTaskTest, TEST_STATE_TRANSITION) {
EXPECT_EQ(task->_exec_state, PipelineTask::State::RUNNABLE);
EXPECT_GT(task->_execution_dependencies.size(), 1);
}
+ // Test static LEGAL_STATE_TRANSITION table (with _wake_up_early = false)
Review Comment:
Minor: the comment says “static LEGAL_STATE_TRANSITION table”, but
LEGAL_STATE_TRANSITION is a per-instance const member (not static). Consider
rewording to avoid confusion about where the transitions are defined.
```suggestion
// Test LEGAL_STATE_TRANSITION table for this task instance (with
_wake_up_early = false)
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
