HappenLee commented on PR #67644: URL: https://github.com/apache/doris/pull/67644#issuecomment-5976265648
Reviewed head fbb6e1138d7c8d476f70daeb7c1127abd30a4e2e. The added `ctx->on_failure(st)` is correct for MySQL/JDBC: it serializes the failure status and runs the RPC completion callback before returning. A few follow-ups would improve this fix: 1. **Please cover the Arrow Flight / ADBC result-fetch path too.** In `PInternalService::fetch_arrow_data()` (`be/src/service/internal_service.cpp`, lines 702–705), a failed `find_buffer()` still only logs and returns. If the result buffer has been removed before a remote fetch, `GetArrowResultBatchCtx`'s default destructor does not run `done`, so `ArrowFlightBatchRemoteReader::_fetch_data()` waits for the RPC timeout rather than receiving the lookup error. Please also call `ctx->on_failure(st)` in that branch. This is an existing parallel-path omission, not a regression introduced by the new line. 2. **Please add focused tests for missing/removed buffers.** Existing `GetResultBatchCtxTest` tests `on_failure()` directly, but does not prove that either service entry point invokes it when lookup fails. Exercise `fetch_data` and `fetch_arrow_data` with a nonexistent or cancelled buffer, and assert a non-OK response and exactly one completion callback. The current incremental coverage report is 0/1 for this change. 3. **Consider making the lookup error protocol-neutral.** `ResultBufferMgr::find_buffer()` currently returns `no arrow schema for this query, maybe query has been canceled`, which this fix also exposes to MySQL/JDBC clients. A result-buffer-unavailable message retaining the query/instance ID would be clearer; the original cancellation reason is not retained by this lookup. Before merging, please also investigate/re-run the failed BE UT, NonConcurrent Regression, and coverage checks. I could not inspect the TeamCity failure details because the API returned 401, so I am not attributing those failures to this change. This review traced the code paths; no build or runtime test was run locally. -- 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]
