Doris-Breakwater commented on issue #69509: URL: https://github.com/apache/doris/issues/69509#issuecomment-6359011582
Breakwater-GitHub-Analysis-Slot: slot_66312f675387 **Triage: source-confirmed error-propagation defect in the BE Thrift external-scan path; recommend high-priority correctness handling.** The reported production cancellation trigger and the attribution of the missing rows remain unverified. The issue currently has no labels, assignee, or milestone; suggested routing is BE execution/external-scan maintainers, with connector maintainers involved in end-to-end validation. **Verified from source** I checked the exact reported tag, `2.1.11-rc01` → `97b77e6cda6743e2bb4fcc6af45c613f12310461`, the available `branch-3.0` and `branch-4.0` snapshots (`4090f6c40d22` and `8a9961723ea4`, matching their remote heads when checked), and the relevant files at the remote master commit `0d777cd0791553da6f271cb7a62811aa165832fc`. This was read-only source analysis; I did not build BE or execute the proposed C++ test. 1. **The consumer race is real.** In 2.1.11, `fetch_result()` retains a shared queue reference, checks its status, then blocks. Neither a subsequently dequeued sentinel nor `blocking_get() == false` causes another status check. Both can return `OK` with `eos=true`. The same function body exists in the inspected 3.0, 4.0, and master sources. `BackendService::get_next()` propagates a non-OK fetch status, so the failure is lost before that RPC response is formed. See [2.1.11 fetch_result](https://github.com/apache/doris/blob/97b77e6cda6743e2bb4fcc6af45c613f12310461/be/src/runtime/result_queue_mgr.cpp#L48-L76) and [RPC caller](https://github.com/apache/doris/blob/97b77e6cda6743e2bb4fcc6af45c613f12310461/be/src/service/backend_service.cpp#L860-L911). 2. **The version split is supported.** The 2.1.11 external-scan entry constructs `TExecPlanFragmentParams`; its executor publishes errors after `open_vectorized_internal()` and during cancellation. Its scratch-sink close still publishes a sentinel without using `exec_status`. In the inspected 3.0/4.0/master pipeline paths, external scan constructs `TPipelineFragmentParams`, and the scratch-sink local-state close also publishes the sentinel without recording the failure. A full-tree search of the local 3.0/4.0/master snapshots found no calls to `update_queue_status()`, only its declaration and definition. Master’s `PipelineTask::close()` passes `exec_status` into sink close, providing a concrete propagation point. See [legacy executor](https://github.com/apache/doris/blob/97b77e6cda6743e2bb4fcc6af45c613f12310461/be/src/runtime/plan_fragment_executor.cpp#L272-L305), [legacy cancellation](https://github.com/apache/doris/blob/97b77e6cda6743e2bb4fcc6af45c613f12310461/be/src/runtime/p lan_fragment_executor.cpp#L553-L575), and [master sink close](https://github.com/apache/doris/blob/0d777cd0791553da6f271cb7a62811aa165832fc/be/src/exec/operator/memory_scratch_sink_operator.cpp#L53-L64). This establishes the silent-EOF mechanism when failure reaches that close path; it does not prove that every cancellation in every deployment ends silently. 3. **Shutdown is a distinct uncovered exit.** `ResultQueueMgr::cancel()` shuts down the queue and erases its map entry without setting a failure. An in-flight fetch keeps the queue alive through its shared pointer. The blocking queue returns false once shut down and empty, which fetch translates into successful EOF. A later fetch instead fails the map lookup. Also, shutdown does not discard already queued batches: those can still be dequeued. See [manager cancellation](https://github.com/apache/doris/blob/97b77e6cda6743e2bb4fcc6af45c613f12310461/be/src/runtime/result_queue_mgr.cpp#L91-L103) and [blocking queue semantics](https://github.com/apache/doris/blob/97b77e6cda6743e2bb4fcc6af45c613f12310461/be/src/util/blocking_queue.hpp#L49-L80). **Relationship to #67604 and recommended fix scope** [PR #67604](https://github.com/apache/doris/pull/67604) was still **open and unmerged** when checked; I inspected head `2ed3b9cc431d7c993acb0fd2e3609ae6098df9f2`. It publishes failure from sink close and rechecks status **inside the sentinel branch**. It does not change the false-return/shutdown branch or publish a cancellation status before shutdown. Keeping this issue as the correctness/backport tracker is therefore justified. The fix should publish the original terminal failure before waking consumers, preserve the first error, recheck status after dequeue before returning a batch or EOF, and return non-OK for shutdown without successful completion. Cancellation should record its status on the queue before shutdown/map removal. Preserve ordinary successful EOF and repeated EOF behavior. The existing first-error behavior in `RecordBatchQueue::update_status()` should be retained. Backports need branch-specific validation; the legacy status publisher makes the 2.1 consumer change smaller, but a sentinel-only hunk does not cover shutdown. **Validation and missing evidence** - Replace the proposed sleep-based synchronization with a test hook/barrier proving the fetch passed its initial status check before the failure is published. A sleep alone can let a test pass through the initial check without exercising the race. Cover failure + sentinel, manager cancel while blocked, shutdown with queued data, preservation of the original error, clean/repeated EOF, and bounded completion without hangs. Validate both the legacy and pipeline paths and assert a non-OK Thrift response at the connector boundary. - Supply the Spark and Doris connector artifact versions, effective read options, and the complete minimal DDL/data-generation/query/job commands, including predicate pushdown, partitioning, retries, and timeout settings. Verify baseline and connector reads use the same stable table snapshot. - For one failing run, correlate query ID, fragment-instance ID, scan-context ID, tablet, and Spark task attempt with timestamps. Capture BE cancellation reason and preceding warnings, FE logs if relevant, and connector `get_next` status/eos/offset/batch-row counts through the final response. Memory-GC cancellation logs support that reproduction, but do not recover the trigger of the earlier production incident. - Distinguish scan-context GC from memory-GC cancellation. `get_next()` marks the context active with `last_access_time=-1`, and scan-context GC normally skips that state. Explicit concurrent close can reach shutdown, but idle-context GC should not be assumed to have interrupted an active fetch without evidence. See [context cleanup](https://github.com/apache/doris/blob/97b77e6cda6743e2bb4fcc6af45c613f12310461/be/src/runtime/external_scan_context_mgr.cpp#L90-L153). Until a tested fix is available, independently reconcile critical external-scan outputs before accepting or publishing them. Retry success or a larger timeout cannot establish completeness when the server can acknowledge a failed scan as normal EOF. -- 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]
