[fix](arrow-flight) Let GetFlightInfo pass the session layer's Flight status through again - #68221
Merged
morningman merged 1 commit intoSep 20, 2026
Conversation
… status through again apache#67883 made DorisFlightSqlProducer.getFlightInfoStatement rethrow a FlightRuntimeException as is, so that the status the session layer chose reaches the client: UNAVAILABLE from the session's command lock and UNAUTHENTICATED from a closed session (apache#67900, apache#67966), and whatever a refused session is answered with. apache#67820 rewrote that catch block into a catch-all that lets only its two incremental-window errors (by doris-error-code metadata) through and wraps every other FlightRuntimeException as "INTERNAL: get flight info statement failed, <message>" -- the wrapping apache#67883 had removed. The other entry points of the producer (setSessionOptions, streamMetadata) still let the status through and say they do "as in getFlightInfoStatement". Restore the FlightRuntimeException passthrough ahead of the catch-all. The window errors are FlightRuntimeExceptions built by queryFailure, so the metadata special case is subsumed and goes. The unit tests apache#67820 added for the wrapping now assert the passthrough; the one for a non-Flight failure keeps asserting the INTERNAL wrapper. Found through apache#68101, whose test_connection_quota asserts the RESOURCE_EXHAUSTED the connection pool answers a refused Flight session with: the pipelines compile a PR merged into master, and every run after apache#67820 landed received the wrapped INTERNAL instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27148 ms |
Contributor
TPC-DS: Total hot run time: 153893 ms |
Contributor
ClickBench: Total hot run time: 23.91 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
FE UT Coverage ReportIncrement line coverage |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review opinion: Approve. No blocking or non-blocking correctness finding survived the complete static review.
Critical checkpoint conclusions:
- Goal and proof: the patch restores passthrough of an existing
FlightRuntimeException, so the session/command layer status, description, metadata, and cause reach the client unchanged. The tests assert object identity for ordinary Flight statuses and metadata-bearing business errors, while retaining a negative control that non-Flight failures are wrapped asINTERNAL. - Scope and clarity: the production change is limited to catch classification plus the directly corresponding unit expectations; the obsolete
ErrorCodedependency and metadata allowlist are removed. - Concurrency and lifecycle: no locking or state lifecycle is changed. The command-lock
CANCELLED/UNAVAILABLEand torn-down-sessionUNAUTHENTICATEDpaths retain their intended meaning; query-body failures still run deferred-query cancellation/finalization beforequeryFailureconstructs the outgoing status, andcallCommandstill restores thread-local state and unlocks infinally. - Compatibility and parallel paths: this restores the behavior that other producer entry points and prepared-statement paths already follow. Incremental-window
UNAVAILABLEmetadata remains intact, ordinary query failures remainINTERNAL, and no FE-BE protocol, serialization, storage, configuration, persistence, transaction, or data-write compatibility surface changes. - Tests and observability: the boundary-focused assertions compose with existing real command-lock, teardown, and window-error tests. The pending connection-pool PR owns the real
RESOURCE_EXHAUSTEDrefusal and its integration coverage. Expected client statuses are no longer hidden by a redundant wrapper; no material performance or observability regression was found. - User focus: no additional focus points were supplied.
This was a static-only review as required by the runner instructions; I did not run builds or tests. Current CI reports FE UT and CheckStyle passing, but those are CI evidence rather than independent execution here.
Convergence: the main scan plus both normal reviewers and the separate risk-focused reviewer covered both changed files and their upstream/downstream paths. All three reviewers returned NO_NEW_VALUABLE_FINDINGS in Round 1, all initial risks were resolved with code evidence, and no inline comments were proposed.
Reviewed exact head: 06952c59513fcfa6dffe575801cd7020294fd778 (base 979e053f0802fbc48f83dafe7f713c02968415a8).
924060929
approved these changes
Sep 20, 2026
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #67883, #67820, #67966, #68101
Problem Summary:
DorisFlightSqlProducer.getFlightInfoStatementused to rethrow aFlightRuntimeExceptionas is (#67883), so the status the session layer chose reaches the client:UNAVAILABLEfrom the session's command lock andUNAUTHENTICATEDfrom a closed session (#67900, #67966), and whatever a refused session is answered with. #67820 rewrote that catch block into a catch-all that lets only its two incremental-window errors through (bydoris-error-codemetadata) and wraps every otherFlightRuntimeExceptionasINTERNAL: get flight info statement failed, <message>-- the wrapping #67883 had removed. The producer's other entry points (setSessionOptions,streamMetadata) still let the status through and say they do it "as in getFlightInfoStatement".This PR restores the passthrough ahead of the catch-all. The window errors are
FlightRuntimeExceptions built byqueryFailure, so #67820's metadata special case is subsumed and removed; a non-Flight failure is still wrapped asINTERNALwith the same message.Found through #68101: its
test_connection_quotaasserts theRESOURCE_EXHAUSTEDthe connection pool answers a refused Flight session with, and since the pipelines compile a PR merged into master, every run after #67820 landed got the wrappedINTERNALinstead.Release note
None
Check List (For Author)
DorisFlightSqlProducerTest-- the wrapping tests of [fix](binlog) Track committed TSO and fence uncertain commits for bounded incremental reads #67820 now assert the passthrough (testGetFlightInfoPassesOtherFlightErrorsThrough,testGetFlightInfoPassesOtherBusinessErrorsThrough);testGetFlightInfoPreservesBothWindowErrorsandtestGetFlightInfoWrapsNonFlightErrorsunchanged in intent.arrow_flight_sql_p0/test_connection_quotaof [refactor](arrow-flight) One connection pool for MySQL connections and Arrow Flight SQL sessions #68101 is the end-to-end check, once that PR is rebased onto this.Check List (For Reviewer who merge this PR)