github-actions[bot] commented on PR #61768:
URL: https://github.com/apache/doris/pull/61768#issuecomment-4132623850
## Code Review Summary
**Changes reviewed:** Merging two loops into one in
`Pipeline::make_all_runnable()`, and removing the redundant
`PipelineTask::terminate()` call from `PipelineTask::execute()`'s deferred
lambda.
### Critical Checkpoint Conclusions
- **Goal and correctness:** The PR removes a redundant
`PipelineTask::terminate()` call from `execute()` and simplifies the loop in
`make_all_runnable()`. Both changes are correct.
- **Concurrency analysis:**
- The per-task ordering guarantee (`set_wake_up_early` before `terminate`)
described in the comment at lines 490-505 of `pipeline_task.cpp` is preserved
in the single-loop version, because for each individual task,
`set_wake_up_early()` is still called before `terminate()`.
- Cross-task effects via shared dependencies (`_execution_dependencies`,
`_memory_sufficient_dependency`) do not affect the race condition because
`_is_pending_finish()` only checks per-task `_finish_dependencies`.
- All code paths that set `_wake_up_early=true` (`make_all_runnable`,
`stop_if_finished`) call `PipelineTask::terminate()` immediately after, so the
removed call in `execute()` was indeed redundant (idempotent double-call).
- **Parallel code paths:** `do_revoke_memory()` at line 763 still calls
`terminate()` in its deferred lambda when `_wake_up_early` is true. This is a
minor inconsistency with this PR's change but is not incorrect — it serves as
an idempotent safety net in the spill path.
- **Lifecycle / memory / error handling:** No issues —
`PipelineTask::terminate()` is guarded by `!is_finalized()` and the
`_dependency_lock` mutex, so late or redundant calls are safe.
- **Test coverage:** Existing regression tests (`test_terminate.groovy`,
`test_slow_close.groovy`) cover the wake-up-early and make_all_runnable paths
with debug points. No new test is needed for this simplification.
- **Incompatible changes:** None — this is a pure refactoring with no
behavioral change.
- **Configuration / observability:** Not applicable.
**Verdict:** No issues found. The changes are safe and the simplification is
clean.
--
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]
